Shopify / theme-tools

Everything developer experience for Shopify themes
https://shopify.dev/docs/themes
MIT License
48 stars 14 forks source link

LiquidHTMLSyntaxError false positive with dynamic attributes #335

Open alibosworth opened 3 months ago

alibosworth commented 3 months ago

Describe the bug It seems like liquid with dynamic attribute name is considered an error

Source


<img {{ src_attr }}="{{ image | image_url: width: fallback_width }}" />
Screenshot 2024-03-28 at 5 33 02 PM

If i change the attribute name to a fixed string, it doesn't error (img src="...")

Expected behaviour Be able to dynamically set html attribute names in liquid

Actual behaviour Theme-check flags it as an error state

Debugging information -Current Shopify CLI version: 3.58.0

charlespwd commented 3 months ago

I wonder if it trips because img is a void element (you don't need the self closing thing). IIRC those were supposed to be allowed.

https://github.com/Shopify/theme-tools/blob/main/packages/prettier-plugin-liquid/src/test/html-attributes/index.liquid#L97-L99

We indeed don't have a unit test for "attribute name only" though. That wouldn't be a bad thing to add!

Thanks for reporting!

charlespwd commented 3 months ago

Confirmed: image

charlespwd commented 3 months ago

Ah found it: image

The liquidNode rule takes precedence over the attribute ones. And both start with the same rule since an attribute name is (text | liquidDrop)*. So what happens here is that the liquidNode rule is "completed" and doesn't try to parse the = as though it was an attribute. Since the rule is completed, then it starts to try parsing another attribute or a closing tag character. Can't, therefore throws.

It might be simply a matter of kicking liquidNode down the list. But I'll have to think about it a bit more.