Shopify / theme-check

The Ultimate Shopify Theme Linter
Other
337 stars 95 forks source link

`UndefinedObject` incorrectly flagging image data-attributes #539

Closed ConduciveMammal closed 2 years ago

ConduciveMammal commented 2 years ago

Using the new image_tag filters, theme-check is flagging data-attributes as UndefinedObject.

{{ media | image_url: width: 900 | image_tag: sizes: '180, 360, 540, 720, 980, 1080, 1296, 1512, 1728, 2048', loading: 'lazy', preload: true, class: 'product-single__photo__img js-pswp-img', data-pswp-src: master_url, data-pswp-height: media.height, data-pswp-width: media.width, data-media-id: media.id }}

I thought they might need quoting, although the docs don't specify this, but they also render as expected on the frontend without quotes.

Quoting causes another issue: Expected end_of_string but found colon.

image

Working frontend example image

Documentation sans data-attributes https://shopify.dev/api/liquid/filters/html-filters#additional-attributes

charlespwd commented 2 years ago

Hmm there must be a discrepancy between how we parse the arguments somewhere. Definitely a bug.

charlespwd commented 2 years ago

Some notes (to self or someone else):

=> #<Liquid::Variable:0x0000000140358e28
 @filters=
  [["image_url", [], {"width"=>900}],
   ["image_tag",
    [#<Liquid::VariableLookup:0x0000000140379150 @command_flags=0, @lookups=["master_url"], @name="data-pswp-src">,
     #<Liquid::VariableLookup:0x000000014038aea0 @command_flags=0, @lookups=["media", "height"], @name="data-pswp-height">,
     #<Liquid::VariableLookup:0x00000001403905a8 @command_flags=0, @lookups=["media", "width"], @name="data-pswp-width">,
     #<Liquid::VariableLookup:0x00000001403900f8 @command_flags=0, @lookups=["media", "id"], @name="data-media-id">],
    {"sizes"=>"180, 360, 540, 720, 980, 1080, 1296, 1512, 1728, 2048", "loading"=>"lazy", "preload"=>true, "class"=>"product-single__photo__img js-pswp-img"}]],
charlespwd commented 2 years ago

Wow. Really questioned my sanity with this one.

Turns out it's indeed a liquid bug! It works in production because of a difference between the liquid-c and the liquid implementation of how we parse filters. Liquid-C will parse tag arguments with dashes in them as tag names, whereas Liquid won't because of the \w regex.

I'm making a PR to liquid to fix this difference.

ConduciveMammal commented 2 years ago

Damn, that's crazy!

Perhaps a more JS/React style would be preferred? {% render 'snippet', dataPswpHeight: 15 %}

charlespwd commented 2 years ago

Nono, we handle them in liquid-c and I think that's a feature. I'll just fix liquid :D

charlespwd commented 2 years ago

(Blocked b/c waiting for a new release of liquid)