Skip to content
This repository has been archived by the owner on Jul 27, 2024. It is now read-only.

HTML/Liquid replacement logic error surfaced in language server. #441

Closed
charlespwd opened this issue Sep 9, 2021 · 5 comments · Fixed by #444
Closed

HTML/Liquid replacement logic error surfaced in language server. #441

charlespwd opened this issue Sep 9, 2021 · 5 comments · Fixed by #444
Assignees
Labels
bug Something isn't working p:high Priority (or Impact): High

Comments

@charlespwd
Copy link
Contributor

Checking c:/Users/ScottJ/Documents/ShopifyDev/Gales/GalesWIP
undefined method `[]' for nil:NilClass
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_node.rb:72:in `block in replace_placeholders'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_node.rb:71:in `gsub'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_node.rb:71:in `replace_placeholders'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_node.rb:60:in `markup'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/checks.rb:51:in `rescue in call_check_method'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/checks.rb:29:in `call_check_method'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/checks.rb:11:in `block in call'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/checks.rb:10:in `each'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/checks.rb:10:in `call'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_visitor.rb:51:in `call_checks'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_visitor.rb:41:in `visit'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_visitor.rb:43:in `block in visit'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_visitor.rb:43:in `each'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_visitor.rb:43:in `visit'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/html_visitor.rb:17:in `visit_template'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/analyzer.rb:40:in `block (2 levels) in analyze_theme'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/analyzer.rb:38:in `each'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/analyzer.rb:38:in `block in analyze_theme'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check.rb:57:in `with_liquid_c_disabled'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/analyzer.rb:37:in `analyze_theme'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/language_server/handler.rb:144:in `block in analyze_and_send_offenses'
C:/Ruby30-x64/lib/ruby/3.0.0/benchmark.rb:293:in `measure'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/language_server/handler.rb:143:in `analyze_and_send_offenses'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/language_server/handler.rb:61:in `on_text_document_did_open'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/language_server/server.rb:105:in `process_request'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/language_server/server.rb:45:in `block in listen'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/language_server/server.rb:44:in `loop'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/language_server/server.rb:44:in `listen'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/lib/theme_check/language_server.rb:27:in `start'
C:/Ruby30-x64/lib/ruby/gems/3.0.0/gems/theme-check-1.5.2/exe/theme-check-language-server:10:in `<top (required)>'
C:/Ruby30-x64/bin/theme-check-language-server:23:in `load'
C:/Ruby30-x64/bin/theme-check-language-server:23:in `<main>'
[Info  - 12:38:20 PM] Connection to server got closed. Server will restart.
[Error - 12:38:20 PM] Request textDocument/documentLink failed.
Error: Connection got disposed.
	at Object.dispose (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:35828)
	at Object.dispose (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:116000)
	at w.handleConnectionClosed (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:116213)
	at w.handleConnectionClosed (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:176601)
	at t (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:114302)
	at r.invoke (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:37441)
	at o.fire (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:38202)
	at Y (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:25086)
	at r.invoke (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:37441)
	at o.fire (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:38202)
	at m.fireClose (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:46083)
	at Socket.<anonymous> (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:47668)
	at Socket.emit (events.js:327:22)
	at Socket.EventEmitter.emit (domain.js:467:12)
	at Pipe.<anonymous> (net.js:673:12)
[Error - 12:38:20 PM] Request textDocument/documentLink failed.
Error: Connection got disposed.
	at Object.dispose (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:35828)
	at Object.dispose (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:116000)
	at w.handleConnectionClosed (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:116213)
	at w.handleConnectionClosed (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:176601)
	at t (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:114302)
	at r.invoke (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:37441)
	at o.fire (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:38202)
	at Y (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:25086)
	at r.invoke (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:37441)
	at o.fire (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:38202)
	at m.fireClose (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:46083)
	at Socket.<anonymous> (c:\Users\ScottJ\.vscode\extensions\shopify.theme-check-vscode-1.1.5\dist\extension.js:1:47668)
	at Socket.emit (events.js:327:22)
	at Socket.EventEmitter.emit (domain.js:467:12)
	at Pipe.<anonymous> (net.js:673:12)
[Error - 12:38:20 PM] Request textDocument/documentLink failed.
Error: Connection got disposed.
@charlespwd charlespwd added the bug Something isn't working label Sep 9, 2021
@charlespwd charlespwd added the p:high Priority (or Impact): High label Sep 9, 2021
@charlespwd charlespwd self-assigned this Sep 9, 2021
@charlespwd
Copy link
Contributor Author

cc @furnaceX. Looks like the following line didn't work:

string.gsub(LIQUID_TAG) do |match|
key = /\d+/.match(match)[0]
@placeholder_values[key.to_i]
end

I'd have to look into why we're not replacing it properly. Do you happen to have a theme you could send me? Or know which file is causing the issue?

(You can send it privately to [email protected])

@furnaceX
Copy link

furnaceX commented Sep 16, 2021 via email

@charlespwd
Copy link
Contributor Author

Hi Scott,

When I'm looking at Liquid files in VS Code, I see the liquid linting
errors. But I'm not getting the linting of the <script> and <style>
blocks. I can temporarily change the file type to HTML and get better
linting of those sections. I've messed around with my config settings but
can't seem to get the script and style blocks to lint when I have the file
set as liquid type.

Do you mean syntax highlighting or linting? How are you linting javascript/styles in liquid files? Wouldn't the liquid tags and variables would break the CSS/JS parsers?

Or are you writing them without liquid in them and expect them to lint properly? In that case I would suggest writing your JS/CSS in separate files in the assets folder.

One minor bug I notice is that when you have the squiggly red error
underline, if you add some lines above, the error location doesn't adjust
with the additional lines:

I think that's expected behaviour. The linter reruns on file save. Otherwise we'd be eating your CPU like no there's no tomorrow.

@furnaceX
Copy link

furnaceX commented Sep 16, 2021 via email

@charlespwd
Copy link
Contributor Author

Unfortunately that doesn't work. Here's an example that would break an HTML parser:

<script src="{{ "theme.js" | asset_url }}" defer></script>

This would parse the script tag like this:

{
  type: "script",
  attributes: {
    "src": "{{ ",
    "theme.js": "| asset_url }}",
  },
},

Another example:

<img srcset="{% if "string" > x %}...{% endif %}">

The HTML parser would close the tag after the "string" part (because of the greater than sign.)

Inside Theme-Check, we go to great lengths just to avoid bugs like these and properly lint HTML errors properly.

As for the Syntax Highlighting, that's indeed an area that needs work. But so long as you are using JavaScript or CSS inside liquid files, you'd have similar problems. Since {{ }} would break both the JavaScript and CSS parser.

Which is why I suggest moving that content over inside the assets folder and treat them as real JavaScript and CSS files.

If you must use liquid for CSS variables and such, I'd leave only the definition of variables inside the liquid files.

Here's an example

<!doctype html>
<html>
<head>
  ...
  {% comment %}
    Here's how you would have liquid settings for CSS,
    while still using actual CSS files that you can
    lint/syntax highlight properly.

    1. We define the CSS variables
  {% endcomment %}
  <style>
    :root {
      --footer-border-color: {{ settings.footer_border_color }};
    }
  </style>

  {% comment %}
    2. We use those variables in a separate file in assets/theme.css
  {% endcomment %}
  {{ 'theme.css' | asset_url | stylesheet_tag }}

  {% comment %}
    Similarly for scripts, we're going to use variables defined on window.
  {% endcomment %}
  <script src="{{ 'theme.js' | asset_url }}" defer></script>
</head>
<body>
  ...

  {% comment %}
    At the end of the body, we'll add our inline script tag with the variables in it.
  {% endcomment %}
  <script id="my-theme-variables" type="application/json">
    window.routes = {
      cart_add_url: '{{ routes.cart_add_url }}',
    }
  </script>
</body>
</html>
// theme.css
#right-arrow g {
  fill: var(--footer-border-color);
}
// theme.js

// Notice how this file is not liquid, therefore you can do everything you want with it:
//   * lint (eslint, tslint, etc.)
//   * transpile (babel, typescript, webpack, etc.)
//   * reformat (prettier)
function init() {
  // Do whatever you want with the config.
  console.log(window.routes);
}

init();

You'll also notice this is exactly how Shopify/dawn does it:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working p:high Priority (or Impact): High
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants