diff --git a/.changeset/sweet-steaks-greet.md b/.changeset/sweet-steaks-greet.md new file mode 100644 index 000000000..ccb8d9f61 --- /dev/null +++ b/.changeset/sweet-steaks-greet.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme-check-common': minor +--- + +Updated the `Undefined-object` theme check to skip over the new `snippet` tags diff --git a/packages/theme-check-common/src/checks/undefined-object/index.spec.ts b/packages/theme-check-common/src/checks/undefined-object/index.spec.ts index d44ff5582..b500ae87f 100644 --- a/packages/theme-check-common/src/checks/undefined-object/index.spec.ts +++ b/packages/theme-check-common/src/checks/undefined-object/index.spec.ts @@ -459,4 +459,110 @@ 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); + }); + + describe('Scope-aware checking for inline snippets', () => { + it('No doc tags anywhere - should skip all checks', async () => { + const sourceCode = ` + {{ undefined_in_parent }} + + {% snippet inline_one %} + {{ undefined_in_inline }} + {% endsnippet %} + `; + + const filePath = 'snippets/file.liquid'; + const offenses = await runLiquidCheck(UndefinedObject, sourceCode, filePath); + + expect(offenses).toHaveLength(0); + }); + + it('Doc tag in parent only - should check parent, skip inline snippets', async () => { + const sourceCode = ` + {% doc %} + @param {string} parent_param + {% enddoc %} + + {{ parent_param }} + {{ undefined_in_parent }} + + {% snippet inline_one %} + {{ undefined_in_inline }} + {% endsnippet %} + `; + + const filePath = 'snippets/file.liquid'; + const offenses = await runLiquidCheck(UndefinedObject, sourceCode, filePath); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toBe("Unknown object 'undefined_in_parent' used."); + }); + + it('Doc tag in inline snippet only - should skip parent, check inline snippet', async () => { + const sourceCode = ` + {{ undefined_in_parent }} + + {% snippet inline_one %} + {% doc %} + @param {string} inline_param + {% enddoc %} + + {{ inline_param }} + {{ undefined_in_inline }} + {% endsnippet %} + `; + + const filePath = 'snippets/file.liquid'; + const offenses = await runLiquidCheck(UndefinedObject, sourceCode, filePath); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toBe("Unknown object 'undefined_in_inline' used."); + }); + + it('Doc tags in both - should check both, but skip inline snippets without docs', async () => { + const sourceCode = ` + {% doc %} + @param {string} parent_param + {% enddoc %} + + {{ parent_param }} + {{ undefined_in_parent }} + + {% snippet inline_one %} + {% doc %} + @param {string} inline_param + {% enddoc %} + + {{ inline_param }} + {{ undefined_in_inline }} + {% endsnippet %} + + {% snippet inline_two %} + {{ anything }} + {% endsnippet %} + `; + + const filePath = 'snippets/file.liquid'; + const offenses = await runLiquidCheck(UndefinedObject, sourceCode, filePath); + + expect(offenses).toHaveLength(2); + expect(offenses.map((o) => o.message)).toEqual([ + "Unknown object 'undefined_in_parent' used.", + "Unknown object 'undefined_in_inline' used.", + ]); + }); + }); }); diff --git a/packages/theme-check-common/src/checks/undefined-object/index.ts b/packages/theme-check-common/src/checks/undefined-object/index.ts index f9d322a8b..c70f80789 100644 --- a/packages/theme-check-common/src/checks/undefined-object/index.ts +++ b/packages/theme-check-common/src/checks/undefined-object/index.ts @@ -7,6 +7,7 @@ import { LiquidTagDecrement, LiquidTagFor, LiquidTagIncrement, + LiquidTagSnippet, LiquidTagTablerow, LiquidVariableLookup, NamedTags, @@ -16,7 +17,7 @@ import { import { LiquidCheckDefinition, Severity, SourceCodeType, ThemeDocset } from '../../types'; import { isError, last } from '../../utils'; import { hasLiquidDoc } from '../../liquid-doc/liquidDoc'; -import { isWithinRawTagThatDoesNotParseItsContents } from '../utils'; +import { isWithinRawTagThatDoesNotParseItsContents, findInlineSnippetAncestor } from '../utils'; type Scope = { start?: number; end?: number }; @@ -38,13 +39,14 @@ export const UndefinedObject: LiquidCheckDefinition = { create(context) { const relativePath = context.toRelativePath(context.file.uri); const ast = context.file.ast; + const isSnippetFile = relativePath.startsWith('snippets/'); if (isError(ast)) return {}; /** * Skip this check when a snippet does not have the presence of doc tags. */ - if (relativePath.startsWith('snippets/') && !hasLiquidDoc(ast)) return {}; + if (isSnippetFile && !hasLiquidDoc(ast)) return {}; /** * Skip this check when definitions for global objects are unavailable. @@ -56,7 +58,9 @@ export const UndefinedObject: LiquidCheckDefinition = { const themeDocset = context.themeDocset; const scopedVariables: Map = new Map(); const fileScopedVariables: Set = new Set(); - const variables: LiquidVariableLookup[] = []; + const variables: Array<{ node: LiquidVariableLookup; inlineSnippet: LiquidTag | null }> = []; + const inlineSnippetsWithDocTags: Set = new Set(); + let snippetFileHasDocTag = false; function indexVariableScope(variableName: string | null, scope: Scope) { if (!variableName) return; @@ -66,9 +70,27 @@ export const UndefinedObject: LiquidCheckDefinition = { } return { - async LiquidDocParamNode(node: LiquidDocParamNode) { + async LiquidRawTag(node, ancestors) { + if (!isSnippetFile || node.name !== 'doc') return; + + const parent = last(ancestors); + if (isLiquidTag(parent) && isLiquidTagSnippet(parent)) { + inlineSnippetsWithDocTags.add(parent.position.start); + } else { + snippetFileHasDocTag = true; + } + }, + + async LiquidDocParamNode(node: LiquidDocParamNode, ancestors: LiquidHtmlNode[]) { const paramName = node.paramName?.value; - if (paramName) { + if (!paramName) return; + const snippetAncestor = findInlineSnippetAncestor(ancestors); + if (snippetAncestor) { + indexVariableScope(paramName, { + start: snippetAncestor.blockStartPosition.end, + end: snippetAncestor.blockEndPosition?.start, + }); + } else { fileScopedVariables.add(paramName); } }, @@ -134,6 +156,10 @@ export const UndefinedObject: LiquidCheckDefinition = { end: node.blockEndPosition?.start, }); } + + if (isLiquidTagSnippet(node) && node.markup.name) { + fileScopedVariables.add(node.markup.name); + } }, async VariableLookup(node, ancestors) { @@ -142,7 +168,10 @@ export const UndefinedObject: LiquidCheckDefinition = { const parent = last(ancestors); if (isLiquidTag(parent) && isLiquidTagCapture(parent)) return; - variables.push(node); + if (isLiquidTag(parent) && isLiquidTagSnippet(parent)) return; + + const inlineSnippet = findInlineSnippetAncestor(ancestors); + variables.push({ node, inlineSnippet }); }, async onCodePathEnd() { @@ -150,9 +179,22 @@ export const UndefinedObject: LiquidCheckDefinition = { objects.forEach((obj) => fileScopedVariables.add(obj.name)); - variables.forEach((variable) => { + variables.forEach(({ node: variable, inlineSnippet }) => { if (!variable.name) return; + // For snippet files: only check variables if they're in a scope with a doc tag. + if (isSnippetFile) { + if (inlineSnippet) { + if (!inlineSnippetsWithDocTags.has(inlineSnippet.position.start)) { + return; + } + } else { + if (!snippetFileHasDocTag) { + return; + } + } + } + const isVariableDefined = isDefined( variable.name, variable.position, @@ -268,6 +310,10 @@ function isLiquidTagCapture(node: LiquidTag): node is LiquidTagCapture { return node.name === NamedTags.capture; } +function isLiquidTagSnippet(node: LiquidTag): node is LiquidTagSnippet { + return node.name === NamedTags.snippet; +} + function isLiquidTagAssign(node: LiquidTag): node is LiquidTagAssign { return node.name === NamedTags.assign && typeof node.markup !== 'string'; } diff --git a/packages/theme-check-common/src/checks/utils.ts b/packages/theme-check-common/src/checks/utils.ts index 73a89500c..6f6daabd9 100644 --- a/packages/theme-check-common/src/checks/utils.ts +++ b/packages/theme-check-common/src/checks/utils.ts @@ -13,6 +13,7 @@ import { LiquidTagTablerow, LiquidTag, LoopNamedTags, + NamedTags, } from '@shopify/liquid-html-parser'; import { LiquidHtmlNodeOfType as NodeOfType } from '../types'; @@ -104,6 +105,16 @@ export function isLoopLiquidTag(tag: LiquidTag): tag is LiquidTagFor | LiquidTag return LoopNamedTags.includes(tag.name as any); } +export function findInlineSnippetAncestor(ancestors: LiquidHtmlNode[]) { + for (let i = ancestors.length - 1; i >= 0; i--) { + const ancestor = ancestors[i]; + if (ancestor.type === NodeTypes.LiquidTag && ancestor.name === NamedTags.snippet) { + return ancestor; + } + } + return null; +} + const RawTagsThatDoNotParseTheirContents = ['raw', 'stylesheet', 'javascript', 'schema']; function isRawTagThatDoesNotParseItsContent(node: LiquidHtmlNode) {