Investigate bringing back UndefinedObjects for snippets

[macournoyer]

It was disabled by default in Shopify/theme-check#160 because of issues with optional parameters.

Possible there's a better way to make this all work out. But I can't think of any at the moment.

Also see Shopify/theme-check#134 for another attempt at fixing it.

[charlespwd]

Did we consider using something like a magic comment that allows certain variables?

Something similar to ESlint's /* global var1, var2 */ (example).

We could then make UnusedVariable make sure that those globals are used, and it would fix the UndefinedObject problem. We'd also need to document how to fix that intuitively.

Could be something like:

snippets/tags.liquid

{% comment %}theme-check variables tags, potatoes{% endcomment %}
{% comment %}
  Here we tell theme-check, and developers, that this snippets expects the tags and potatoes variables to be passed to render.

  e.g.

  {% render 'this-snippet', potato: product, tags: product.tags %}
{% endcomment %}
<ul>
  {% for tag in tags %}
    <li>{% comment %}do stuff with tag{% endcomment %}</li>
  {% endfor %}
  {% for potato in potatoes %}
    <li>{% comment %}do stuff with potato{% endcomment %}</li>
  {% endfor %}
</ul>

[macournoyer]

Did we consider using something like a magic comment that allows certain variables?

Kind of: https://github.com/Shopify/theme-check/pull/134#discussion_r568861008.

But excluding snippets was a simpler, safer, short-term solution.

I don't understand your code example here.

[charlespwd]

You'd have a magic comment that adds "global" objects to the current file. In this case, tags is not a global variable, so with exclude_snippets: false, the check would say that this variable doesn't exist.

What I'm proposing is that instead of checking if tags is a valid global variable, it would check if it's a global variable or one defined by the magical comment.

This way, we wouldn't "disable this check for those variables", we would specify "here are variables that should be defined globally when using this snippet."

Which would allow us to do the following:

• Not flag code presented above as an error
• Tell you if the argument is missing when using render
• Tell you if the variable is used or not and flag the useless comment if the variable isn't used.

(P.S. I updated the code example)

[macournoyer]

That make sense to me!

However, I'm unclear on the syntax. Is {% comment %}theme-check variables tags, potatoes{% endcomment %} the magic comment to define global variables?

[charlespwd]

Yup! We'd only allow those in snippet files.

[macournoyer]

I think it'd be more uniform w/ theme-check-disable comments if it was:

{% comment %}theme-check-variables tags, potatoes{% endcomment %}

(Adding the dash)

[t-kelly]

Came to the repo to write an issue on this -- happy to see you're ahead of me. This feature would a lifesaver as we create more and more reusable snippets. 💯

[charlespwd]

As I found out in Shopify/theme-check#527, the following liquid passes the check:

{% assign class = class | default: '' %}

Do we still want the comment syntax or do we tell partners to add the API of their snippets as code at the top of their snippets?

{% liquid
  # required args
  assign product = product
  assign feature = feature

  # optional args
  assign class = class | default: 'product-highlight'
%}

[macournoyer]

Since this is a Theme Check concern, I feel it should be a comment.

In my mind, there's no reason to change the code just to please Theme Check, it should be the other way around where Theme Check adapts to the code best practices.

What do you think?

[charlespwd]

We have different proposals around this. This issue is a downstream effect that I don't think we need to keep track of.