allow UndefinedObject check to work on snippets

[EvilGenius13]

What are you adding in this PR?

Closes: https://github.com/Shopify/developer-tools-team/issues/668

The UndefinedObject check would not look at snippet files. Now that we have liquiddoc support for them, we can amend the check to work on snippets.

Testing

• Pull down the branch
• Build the branch
• Hit F5 to start the development server
• Try adding a variable in a snippet file such as {{ mage }} and make sure it's caught as UndefinedObject
• Add a doc block (mini template here)

{% doc %}
@param {string} mage
{% enddoc %}

• Watch the check disappear

Before you deploy

• This PR includes a new checks or changes the configuration of a check
  • I included a minor bump changeset
  • I ran yarn build and committed the updated configuration files
  • I've made a PR to update the shopify.dev theme check docs if applicable (link PR here).

[charlespwd]

cc @macournoyer !! YEARS LATER, WE FINALLY DID IT! 😂

[jamesmengo]

Functionally looks good!

One thing we can consider: **How will this impact existing themes? **

When this gets released, users on the newest version of theme-tools will get warnings on all snippets. This raises 2 potential concerns:

LiquidDoc is not released yet so users can't resolve the warning on their saved theme

• Users have no way of resolving this aside from disabling the check.
• This is a temporary issue, but that also means we should hold off on shipping this unless we change the approach

Users are forced to adopt LiquidDoc

• This might require a little bit of product input. I'm just dumping my thoughts here, but let's find some time to sync on this, and please feel free to correct any incorrect assumptions I've made
• Similarly to JS doc, I see Liquid doc as an optional additional layer that can provide tooling support but I wouldn't want users to be forced into documenting it.
• Maybe we can only enable this check on snippets when the doc header is present.
• I haven't really thought through the implications of how this might work for blocks yet, though I imagine it could be similar.

Admittedly, I'm not intimately familiar with this specific check, so please feel free to share your thoughts!

[macournoyer]

OMG!! How far we've come 👏

[charlespwd]

@jamesmengo, tough one. Some snippets might not have args so it might feel extra verbose for a dev to add the {% doc %}. They might still want to know that they are using a variable that is undefined though.

Maybe we need a setting? Maybe we need an auto-fix that adds the {% doc %} when there isn't one with the @param for that variable?

Maybe a strict flag?

UndefinedObject:
  enabled: true
  strict: true

[aswamy]

Could we keep this check disabled for snippets if you don't have doc tag? Or will this make this check too unpredictable?

i.e.

    if (relativePath.startsWith('snippets/') && !liquidDocNotSet) {
      return {};
    }

[karreiro]

Thank you for this PR, @EvilGenius13!

I'd be happy to sync on this as well! I believe we could:

1. Adopt Alok's approach and keep this check disabled for snippets/blocks without the {% doc %} tag
2. Add a new check that lints snippets/blocks that don't have the {% doc %} tag but have undefined objects (this would be helpful, as users could choose to disable only this particular check if they prefer) It would also be great to include a fixer that automatically adds {% doc %}\n{% enddoc %}
3. Add a new fixer to include any missing parameters in the {% doc %}

[EvilGenius13]

hasLiquidDoc is meant as a helper to detect the presence of doc tags but without building the arrays of what's inside the nodes. This should be a bit more performant if we're simply trying to check if `{% doc %} {% enddoc %} are in a file

[aswamy]

Im okay with you using extractDocDefinition as well to check if liquidDoc is defined; but i can see the benefit of keeping this light as possible. 👍

[EvilGenius13]

Thanks for everyone talking about the options we have! For now we have added an extra check in the UndefinedObject to look for doc tags if it's in a snippet. If there are none, we will just skip the check. I will open a separate issue about adding another check and laying out another area to discuss options.

[aswamy]

Im okay with you using extractDocDefinition as well to check if liquidDoc is defined; but i can see the benefit of keeping this light as possible. 👍

[karreiro]

Thank you, @EvilGenius13!