Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shoplando wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6189068
Updated the undefined-object theme check to skip over inline-snippets
shoplando Oct 9, 2025
3274d57
update the check so that it's scope aware
shoplando Oct 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/sweet-steaks-greet.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/theme-check-common': minor
---

Updated the `Undefined-object` theme check to skip over the new `snippet` tags
14 changes: 14 additions & 0 deletions packages/theme-check-common/src/checks/undefined-object/index.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,4 +459,18 @@ describe('Module: UndefinedObject', () => {
assert(offenses.length == 0);
expect(offenses).to.be.empty;
});

it('should not report an offense for inline snippet names in snippet and render tags', async () => {
const sourceCode = `
{% snippet my_inline_snippet %}
{% echo 'hello' %}
{% endsnippet %}

{% render my_inline_snippet %}
`;

const offenses = await runLiquidCheck(UndefinedObject, sourceCode);

expect(offenses).toHaveLength(0);
});
});
4 changes: 4 additions & 0 deletions packages/theme-check-common/src/checks/undefined-object/index.ts
View file
Open in desktop
Copy link
Copy Markdown
Contributor

@aswamy aswamy Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@shoplando shoplando Oct 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ export const UndefinedObject: LiquidCheckDefinition = {
const parent = last(ancestors);
if (isLiquidTag(parent) && isLiquidTagCapture(parent)) return;

if (parent?.type === NodeTypes.RenderMarkup && parent.snippet === node) return;

if (isLiquidTag(parent) && parent.name === 'snippet' && parent.markup === node) return;
Copy link
Copy Markdown
Contributor

@aswamy aswamy Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@shoplando shoplando Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@EvilGenius13 EvilGenius13 Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@shoplando shoplando Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


variables.push(node);
},

Expand Down