Shopify / theme-check

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

Dynamic metafield keys throw Unidentified Objects error #576

Closed eballeste closed 2 years ago

eballeste commented 2 years ago

I'm not 100% sure if this is the correct way of doing what I am trying to do but I made a custom app that stores metafields on different resource endpoints and am trying to output those metafields dynamically, based on the current resource page.

In an app embed block rendered in the head I have the following:

{%- liquid
  assign resource = request.page_type 
  assign meta_value = [resource].metafields.namespace.key
  echo meta_value
-%}

the code works fine, outputs the correct value in the different types of resource pages, and throws no errors in the theme's code editor, however, theme-check throws a 'resource' is never used warning for the first line and an Unidentified Object error for the second line.

Screen Shot 2022-04-26 at 2 48 56 AM
charlespwd commented 2 years ago

The second line is invalid syntax which might be the reason our parser is not picking up the used assign.

If that works in prod, it would be because of the lax parser (which is permissive). But the linter uses strict parsing so I'm pretty sure that if you remove the square brackets, this error will go away.

eballeste commented 2 years ago

gotcha, yeah, the errors do go away but the code stops working, nothing is outputted.

I sort of thought that it was a stretch but decided to try with the brackets and got really excited when It actually worked. could you explain what syntax rule is being broken? why would would it work in production but considered bad form by the linter?

is there a better alternative to calling a metafield where the first resource key is dynamic or would I have to build out a large case/when tree where each metafield is spelled out per resource?? (I have many fields, my example is reduced for explanation purposes)

{%- liquid
  case resource
    when 'page':
       assign key_value = page.metafields.custom.key
       ...
    when 'article':
       assign key_value = article.metafields.custom.key
       ...
    when ...
 -%}
charlespwd commented 2 years ago

I sort of thought that it was a stretch but decided to try with the brackets and got really excited when It actually worked. could you explain what syntax rule is being broken? why would would it work in production but considered bad form by the linter?

Thanks for the explanation! The fact that it does work seems like a happy bug? A JS equivalent would be to do window[var] or global[var] but it doesn't look like this is something we support in the strict parser.

I'll have to investigate why this works. Might be a valid use case actually!

charlespwd commented 2 years ago

I asked around. Apparently it's an intended feature! Sorry for the confusion. I'll reopen this and flag it as a bug.