Updated the undefined-object theme check to skip over inline-snippets

[shoplando]

What are you adding in this PR?

Disabled the "Undefined Object" theme check for the new snippet tag to prevent false positives.
This change also includes making the check scope-aware based on inline snippets' doc tag parameters.

Behaviour before change:

Behaviour after change:

Before you deploy

• I included a minor bump changeset
• My feature is backward compatible

[shoplando]

• Added doc tag support logic inside snippet tag #1072 : 2 dependent PRs (#1068 , #1074 )
• Updated the undefined-object theme check to skip over inline-snippets #1071 👈 (View in Graphite)
• introduced snippet tag in ohm #1063 : 1 other dependent PR (#1069 )
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[aswamy]

Is this supposed to be a temporary fix or something instead of just adding it our list of valid liquid tags?

[shoplando]

I already added snippet to our list of valid liquid tags (in packages/liquid-html-parser/src/types.ts) but it still warns me 🤔

[EvilGenius13]

Is this because we load the tags from a different repo?

[shoplando]

I think that repo is used for the documentation, autocomplete and validation of the tag itself. And also the capture tag is using a similar approach by skipping over the newly defined object in the capture tag so that the theme check won't warn you about it (I might just use the same format as the capture case)

[aswamy]

TIL Cool didn't even know we had a variable scope like this

[aswamy]

What's the parent.markup === node part supposed to catch?

[shoplando]

That's an oversight on my part. I think I added that before creating the isLiquidTagSnippet() function and forgot to remove it. Thanks!

[aswamy]

So i think you have the right logic, but i think you need to modify the logic on line 48:

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

This might not make too much sense, but here is the logic:

• There exists real world snippets (especially old ones) that DONT have doc tag
• The code above ensures that we only have UndefinedObject errors on snippets IF they have doc tag because technically, you can pass ANYTHING into snippets, and without a doc tag we don't really know which one is Defined, and which one isn't
• But by introducing inline snippets, we have a slight problem: having a doc tag inside the inline snippets don't mean the top-level snippet file has one too...

So we need a robust solution that works for the following situations:

• What happens when you have no doc tags in the parent snippet AND the inline snippets
• What happens when you have doc tag in the parent, but not in the inline snippets
• What happens when you have doc tag in the inline snippets, but not in the parent
• What happens when you have doc tag in both

I think we need to layout each situation before full implementation of this

[shoplando]

Ok so basically we would want:

• no checks at all if both the snippet file and inline snippet don't have any doc tags
• yes checks when there is a doc tag at its respective scope, for example when there is a doc tag in the main snippet file and none inside the inline snippet:

{%- comment -%} snippets/my-snippet.liquid {%- endcomment -%}

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

{{ parent_param }}         <- ✅ Should NOT warn
{{ undefined_in_parent }}  <- ❓ Should this warn? YES

{% snippet inline_one %}
  {{ undefined_in_inline }} <- ❓ Should this warn? NO
{% endsnippet %}

And when the doc tag is inside the inline snippet, the check will be activated only on the inside of the inline snippet, and not outside of it.
I'll implement this right now.