[Bug]: theme-check-disable not working on assets/*.js.liquid

[ivansimplistic]

Please confirm that you have:

• Searched existing issues to see if your issue is a duplicate. (If you’ve found a duplicate issue, feel free to add additional information in a comment on it.)
• Reproduced the issue in the latest CLI version.

In which of these areas are you experiencing a problem?

Theme

Expected behavior

I'm adding {% # theme-check-disable %} to files in the assets folder, but this tag doesn't work or it is being ignored at all. The file names are like something**.js.liquid**

Actual behavior

When I run shopify theme check I get LiquidHTMLSyntaxError errors on this files, even if the are no true errors. As the theme-check-disable tag doesn't work, the only workaround I found was to ignore all .js.liquid files for the LiquidHTMLSyntaxError check.

This happens on Windows and Mac.

Additionally, the vscode extension is not returning any issues on .js.liquid files, not even if I add something that really breaks liquid like an {% if %} without its closing tag, or use invalid liquid filters, or whatever.
This happens on Windons, I don't know if also on Mac.

Verbose output

assets/auto_addons.js.liquid                                                
                                                                              
                                                                              
  [error]: LiquidHTMLSyntaxError                                              
  SyntaxError: expected ">" or "/>"                                           
                                                                              
  58  equal = equal && item[p][i]===query[p][i];

Reproduction steps

1. run: shopify theme check
2. see the results

Operating System

Windows 11, Mac

Shopify CLI version (check your project's package.json if you're not sure)

3.70

Shell

windows cmd, windows powershell, mac terminal

Node version (run node -v if you're not sure)

v20.17

What language and version are you using in your application?

No response

[ivansimplistic]

Actually, I'm also trying {% # theme-check-disable %} in footer.liquid and it still throws LiquidHTMLSyntaxError issues.

I tried {% # theme-check-disable DeprecatedTag %} somewhere else and that worked, ignoring DeprecatedTag issues.

[aswamy]

Hey @ivansimplistic. I was just testing it out and does seem like you're correct that items in assets/**.js.liquid files don't allow {% # theme-check-disable %} or {% comment %}theme-check-disable LiquidHTMLSyntaxError{% endcomment %}.

I was still able to disable theme checks in footer.liquid. While i look into the issue for files in the asset folder, could you give more details on what you did to disable theme check on footer.liquid file?

[charlespwd]

I thought those were ignored by default, but it seems like that isn't the case.

In the meantime you should be able to mitigate by adding a assets/*.js.liquidentry to your .theme-check.yml's ignore

ignore:
  - 'assets/*.js.liquid'

[ivansimplistic]

Yes, I'm already ignoring those and 17 other liquid files among snippets and sections,
Like this:
LiquidHTMLSyntaxError
ignore:
- assets/*.js.liquid
- sections/footer.liquid
- sections/media_feed.liquid
...

It seems that the check LiquidHTMLSyntaxError is not being disabled by {% # theme-check-disable %}, something is not working as expected.

[charlespwd]

Ah I think it's because we need to be able to parse the file in order to see the disable comment. The disable relies on the parser (not regexes) to disable. But LiquidHTMLSyntaxError is the one that checks that the parser works. If we can't parse, we can't disable.

I wonder if other linters do the same or rely on regexes to do the disable?

Maybe we should use a Liquid AST instead of LiquidHTMLAST?

[ivansimplistic]

That's way beyond my knowledge ;)

Anyway the things that triggers LiquidHTMLSyntaxError in our code are pieces of code like this: (very short example)

<{% if useLink %}a href="/" {% else %}div {% endif %} class="body-text">some text </{% if useLink %}a{% else %}div{% endif %}>

which prints
<a href="/" class="body-text">some text</a>
or
<div class="body-text">some text</div>

The same issue is reported by theme check on the Dawn theme.

If that other issue can be solved, then we wouldn't have to ignore most of those 17 liquid files.

[charlespwd]

Yeah the if statement HTML element name is something the parser doesn't support. We need to make the name or the attributes as compound liquid + text, but the if that spans name & attributes makes it impossible to parse as a node.

That said, we do support having dangling tags inside if statements. So you could rewrite it this way and have no issue:

{% if useLink %}
  <a href="/" class="body-text">
{% else %}
  <div class="body-text">
{% endif %}
  some text
{% if useLink %}
  </a>
{% else %}
  </div>
{% endif %}

We also support Liquid variable output in tag names:

{% liquid
  if useLink
    assign tag = 'a'
  else
    assign tag = 'div'
  endif
%}

<{{ tag }} class="body-text">
  some text
</{{ tag }}>

[ivansimplistic]

Thanks.
Due to other element attributes that depend on the tag I ended using {{ '<' }}...{{ '>' }}...{{ '</' }}...{{ '>' }} as a workaround.

However now that I could re-enable LiquidHTMLSyntaxError check for those .liquid files, theme check throws VariantName and DeprecatedTag issues on those files.

Ideally, ignoring a check for a file shouldn't disable other checks.