Shopify / theme-check

The Ultimate Shopify Theme Linter
Other
333 stars 96 forks source link

SyntaxError when using an if statement after an opening HTML tag delimiter #757

Closed nicklocicero closed 4 months ago

nicklocicero commented 4 months ago

Describe the bug In Dawn theme, out of the box, there is an erroneous SyntaxError where an if statement starts after an opening delimiter <.

<{% if section.settings.sticky_header_type != 'none' %}sticky-header data-sticky-type="{{ section.settings.sticky_header_type }}"{% else %}div{% endif %} 
image

Expected I believe this should be allowed because it is using logic in a straightforward way to dynamically choose the right element. The if statement is valid liquid code.

Actual I get the SyntaxError, but the liquid code is functioning fine for Dawn out of the box. Most likely it is used this way on thousands of stores since this is how it's coded out of the box.

Stack trace

SyntaxError: expected a letter, "{{", "wbr" (case-insensitive), "track" (case-insensitive), "source" (case-insensitive), "param" (case-insensitive), "meta" (case-insensitive), "link" (case-insensitive), "keygen" (case-insensitive), "input" (case-insensitive), "img" (case-insensitive), "hr" (case-insensitive), "embed" (case-insensitive), "command" (case-insensitive), "col" (case-insensitive), "br" (case-insensitive), "base" (case-insensitive), "area" (case-insensitive), "svg", "style", or "script"theme-check(LiquidHTMLSyntaxError)

Debugging information

Additional context I tried looking into the theme check code, but I don't have enough Ruby experience to find where the regex or code is that checks for this kind of error.

charlespwd commented 4 months ago

Unfortunately this is an issue that we don't have a fix for. In the LiquidHTML parser, we build an AST of Liquid & HTML nodes together. By doing the if statement after the <, there would be no way to represent an HTML element in a sane way (mostly because it would be rather difficult to find the closing tag).

Example:

<{% if cond %}details attr1 attr2{% else %}h2{% endif %}>

{% unless cond %}</h2>{% endif %}

{% if cond %}</details>{% endif %}

While this is technically valid Liquid. We can't turn that into something that makes sense such as

{
  type: HtmlElement
  name: ...
  children: [
    ...
  ]
}

The way we allow you to do this right now without breaking things is to do the opening tag/closing tag inside if statements.

This is allowed:

{% if cond %}
  <details attr1 attr2>
{% else %}
  <h2>
{% endif %}

{% unless cond %}
  </h2>
{% endif %}

{% if cond %}
  </details>
{% endif %}

It also makes things easier to indent, pretty-print, etc.