diff --git a/packages/theme-check-common/src/checks/index.ts b/packages/theme-check-common/src/checks/index.ts index 251e365cc..1f1d5529e 100644 --- a/packages/theme-check-common/src/checks/index.ts +++ b/packages/theme-check-common/src/checks/index.ts @@ -1,5 +1,6 @@ import { ConfigTarget, JSONCheckDefinition, LiquidCheckDefinition } from '../types'; +import { AppBlockMissingSchema } from './app-block-missing-schema'; import { AppBlockValidTags } from './app-block-valid-tags'; import { AssetPreload } from './asset-preload'; import { AssetSizeAppBlockCSS } from './asset-size-app-block-css'; @@ -11,9 +12,9 @@ import { CaptureOnContentForBlock } from './capture-on-content-for-block'; import { CdnPreconnect } from './cdn-preconnect'; import { ContentForHeaderModification } from './content-for-header-modification'; import { DeprecateBgsizes } from './deprecate-bgsizes'; -import { DeprecateLazysizes } from './deprecate-lazysizes'; import { DeprecatedFilter } from './deprecated-filter'; import { DeprecatedTag } from './deprecated-tag'; +import { DeprecateLazysizes } from './deprecate-lazysizes'; import { EmptyBlockContent } from './empty-block-content'; import { ImgWidthAndHeight } from './img-width-and-height'; import { JSONMissingBlock } from './json-missing-block'; @@ -25,18 +26,19 @@ import { MissingAsset } from './missing-asset'; import { MissingTemplate } from './missing-template'; import { PaginationSize } from './pagination-size'; import { ParserBlockingScript } from './parser-blocking-script'; -import { SchemaPresetsBlockOrder } from './schema-presets-block-order'; -import { SchemaPresetsStaticBlocks } from './schema-presets-static-blocks'; import { RemoteAsset } from './remote-asset'; import { RequiredLayoutThemeObject } from './required-layout-theme-object'; +import { SchemaPresetsBlockOrder } from './schema-presets-block-order'; +import { SchemaPresetsStaticBlocks } from './schema-presets-static-blocks'; import { TranslationKeyExists } from './translation-key-exists'; import { UnclosedHTMLElement } from './unclosed-html-element'; import { UndefinedObject } from './undefined-object'; +import { UniqueSettingIds } from './unique-settings-id'; import { UniqueStaticBlockId } from './unique-static-block-id'; import { UnknownFilter } from './unknown-filter'; import { UnusedAssign } from './unused-assign'; -import { ValidContentForArguments } from './valid-content-for-arguments'; import { ValidBlockTarget } from './valid-block-target'; +import { ValidContentForArguments } from './valid-content-for-arguments'; import { ValidHTMLTranslation } from './valid-html-translation'; import { ValidJSON } from './valid-json'; import { ValidLocalBlocks } from './valid-local-blocks'; @@ -47,10 +49,10 @@ import { ValidSettingsKey } from './valid-settings-key'; import { ValidStaticBlockType } from './valid-static-block-type'; import { ValidVisibleIf, ValidVisibleIfSettingsSchema } from './valid-visible-if'; import { VariableName } from './variable-name'; -import { AppBlockMissingSchema } from './app-block-missing-schema'; -import { UniqueSettingIds } from './unique-settings-id'; +import { VisibleIfUsage } from './visible-if-usage'; export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ + AppBlockMissingSchema, AppBlockValidTags, AssetPreload, AssetSizeAppBlockCSS, @@ -74,13 +76,12 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ MatchingTranslations, MissingAsset, MissingTemplate, - AppBlockMissingSchema, PaginationSize, ParserBlockingScript, - SchemaPresetsBlockOrder, - SchemaPresetsStaticBlocks, RemoteAsset, RequiredLayoutThemeObject, + SchemaPresetsBlockOrder, + SchemaPresetsStaticBlocks, TranslationKeyExists, UnclosedHTMLElement, UndefinedObject, @@ -89,18 +90,19 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ UnknownFilter, UnusedAssign, ValidBlockTarget, - ValidHTMLTranslation, ValidContentForArguments, + ValidHTMLTranslation, ValidJSON, ValidLocalBlocks, + ValidRenderSnippetParams, ValidSchema, + ValidSchemaName, ValidSettingsKey, ValidStaticBlockType, ValidVisibleIf, ValidVisibleIfSettingsSchema, VariableName, - ValidRenderSnippetParams, - ValidSchemaName, + VisibleIfUsage, ]; /** diff --git a/packages/theme-check-common/src/checks/schema-presets-static-blocks/index.spec.ts b/packages/theme-check-common/src/checks/schema-presets-static-blocks/index.spec.ts index 7a5449904..1b144062e 100644 --- a/packages/theme-check-common/src/checks/schema-presets-static-blocks/index.spec.ts +++ b/packages/theme-check-common/src/checks/schema-presets-static-blocks/index.spec.ts @@ -67,7 +67,6 @@ describe('Module: SchemaPresetsStaticBlocks', () => { const offenses = await runLiquidCheck(SchemaPresetsStaticBlocks, sourceCode, DEFAULT_FILE_NAME); expect(offenses).toHaveLength(1); - console.log(offenses); expect(offenses[0].message).toEqual( 'Static block block-2 is missing a corresponding content_for "block" tag.', ); @@ -134,7 +133,6 @@ describe('Module: SchemaPresetsStaticBlocks', () => { const offenses = await runLiquidCheck(SchemaPresetsStaticBlocks, sourceCode, DEFAULT_FILE_NAME); expect(offenses).toHaveLength(1); - console.log(offenses); expect(offenses[0].message).toEqual( 'Static block block-2 is missing a corresponding content_for "block" tag.', ); diff --git a/packages/theme-check-common/src/checks/valid-block-target/index.ts b/packages/theme-check-common/src/checks/valid-block-target/index.ts index ed98095e4..0393c39d7 100644 --- a/packages/theme-check-common/src/checks/valid-block-target/index.ts +++ b/packages/theme-check-common/src/checks/valid-block-target/index.ts @@ -14,7 +14,7 @@ import { isInvalidPresetBlock, validateNestedBlocks, validateBlockFileExistence, - reportWarning, + reportOnJsonNode, } from '../../utils'; type BlockNodeWithPath = { node: Section.Block | ThemeBlock.Block | Preset.Block; @@ -61,7 +61,7 @@ export const ValidBlockTarget: LiquidCheckDefinition = { const exists = await validateBlockFileExistence(node.type, context); if (!exists) { errorsInRootLevelBlocks = true; - reportWarning( + reportOnJsonNode( `Theme block 'blocks/${node.type}.liquid' does not exist.`, offset, typeNode, @@ -87,7 +87,7 @@ export const ValidBlockTarget: LiquidCheckDefinition = { const errorMessage = isPrivateBlockType ? `Theme block type "${node.type}" is a private block so it must be explicitly allowed in "blocks" at the root of this schema.` : `Theme block type "${node.type}" must be allowed in "blocks" at the root of this schema.`; - reportWarning(errorMessage, offset, typeNode, context); + reportOnJsonNode(errorMessage, offset, typeNode, context); } if ('blocks' in node && node.blocks) { @@ -110,7 +110,7 @@ export const ValidBlockTarget: LiquidCheckDefinition = { const typeNode = nodeAtPath(ast, path)! as LiteralNode; const exists = await validateBlockFileExistence(node.type, context); if (!exists) { - reportWarning( + reportOnJsonNode( `Theme block 'blocks/${node.type}.liquid' does not exist.`, offset, typeNode, diff --git a/packages/theme-check-common/src/checks/valid-local-blocks/index.ts b/packages/theme-check-common/src/checks/valid-local-blocks/index.ts index 446dee4c2..901e5dda0 100644 --- a/packages/theme-check-common/src/checks/valid-local-blocks/index.ts +++ b/packages/theme-check-common/src/checks/valid-local-blocks/index.ts @@ -9,7 +9,8 @@ import { import { nodeAtPath } from '../../json'; import { getSchema } from '../../to-schema'; import { isBlock, isSection } from '../../to-schema'; -import { getBlocks, reportWarning } from './valid-block-utils'; +import { getBlocks } from './valid-block-utils'; +import { reportOnJsonNode } from '../../utils'; type BlockNodeWithPath = { node: Section.Block | Preset.Block; @@ -55,7 +56,7 @@ export const ValidLocalBlocks: LiquidCheckDefinition = { if (staticBlockLocations.length > 0 && localBlockLocations.length > 0) { staticBlockLocations.forEach((blockWithPath: BlockNodeWithPath) => { const astNode = nodeAtPath(ast, blockWithPath.path)! as LiteralNode; - reportWarning( + reportOnJsonNode( `Sections cannot use static theme blocks together with locally scoped blocks.`, offset, astNode, @@ -71,7 +72,7 @@ export const ValidLocalBlocks: LiquidCheckDefinition = { ) { localBlockLocations.forEach((blockWithPath: BlockNodeWithPath) => { const astNode = nodeAtPath(ast, blockWithPath.path)! as LiteralNode; - reportWarning( + reportOnJsonNode( 'Sections cannot use theme blocks together with locally scoped blocks.', offset, astNode, @@ -85,7 +86,7 @@ export const ValidLocalBlocks: LiquidCheckDefinition = { if (localBlockLocations.length > 0) { localBlockLocations.forEach((blockWithPath: BlockNodeWithPath) => { const astNode = nodeAtPath(ast, blockWithPath.path)! as LiteralNode; - reportWarning( + reportOnJsonNode( 'Local scoped blocks are not supported in theme blocks.', offset, astNode, diff --git a/packages/theme-check-common/src/checks/valid-local-blocks/valid-block-utils.ts b/packages/theme-check-common/src/checks/valid-local-blocks/valid-block-utils.ts index afa4f592a..eff66efe0 100644 --- a/packages/theme-check-common/src/checks/valid-local-blocks/valid-block-utils.ts +++ b/packages/theme-check-common/src/checks/valid-local-blocks/valid-block-utils.ts @@ -82,16 +82,3 @@ export function getBlocks(validSchema: ThemeBlock.Schema | Section.Schema): { hasRootLevelThemeBlocks: themeBlockLocations.some((block) => block.path[0] === 'blocks'), }; } - -export function reportWarning( - message: string, - offset: number, - astNode: LiteralNode, - context: Context, -) { - context.report({ - message, - startIndex: offset + getLocStart(astNode), - endIndex: offset + getLocEnd(astNode), - }); -} diff --git a/packages/theme-check-common/src/checks/valid-settings-key/index.ts b/packages/theme-check-common/src/checks/valid-settings-key/index.ts index c8eb2cf8a..9f3ebc8f8 100644 --- a/packages/theme-check-common/src/checks/valid-settings-key/index.ts +++ b/packages/theme-check-common/src/checks/valid-settings-key/index.ts @@ -10,7 +10,7 @@ import { } from '../../types'; import { nodeAtPath } from '../../json'; import { getSchema, isSectionSchema } from '../../to-schema'; -import { BlockNodeWithPath, getBlocks, reportWarning } from '../../utils'; +import { BlockNodeWithPath, getBlocks, reportOnJsonNode } from '../../utils'; export const ValidSettingsKey: LiquidCheckDefinition = { meta: { @@ -123,7 +123,7 @@ function validateSettingsKey( ? `Setting '${setting.key.value}' does not exist in 'blocks/${blockNode.type}.liquid'.` : `Setting '${setting.key.value}' does not exist in schema.`; - reportWarning(errorMessage, offset, setting.key, context); + reportOnJsonNode(errorMessage, offset, setting.key, context); } } } diff --git a/packages/theme-check-common/src/checks/valid-visible-if/index.spec.ts b/packages/theme-check-common/src/checks/valid-visible-if/index.spec.ts index 24ef3cde9..f11392543 100644 --- a/packages/theme-check-common/src/checks/valid-visible-if/index.spec.ts +++ b/packages/theme-check-common/src/checks/valid-visible-if/index.spec.ts @@ -42,7 +42,7 @@ const baseThemeData: Theme = { name: 't:some.namespace', settings: [ { type: 'header', content: 't:some.other.namespace' }, - { id: 'some-global-setting' }, + { type: 'text', id: 'some-global-setting' }, ], }, ], @@ -142,29 +142,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 49, - "index": 195, - "line": 10, - }, - "fix": undefined, - "message": "Invalid variable: "some-nonexistent-whatever" was not found.", - "severity": 0, - "start": { - "character": 24, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///blocks/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "some-nonexistent-whatever" was not found.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); }); it('reports an error for an invalid reference to a block schema setting (simple lookup)', async () => { @@ -176,29 +160,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 69, - "index": 215, - "line": 10, - }, - "fix": undefined, - "message": "Invalid variable: "block.settings.some-nonexistent-block-setting" was not found.", - "severity": 0, - "start": { - "character": 24, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///blocks/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "block.settings.some-nonexistent-block-setting" was not found.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); }); it('reports an error for an invalid reference to a block schema setting (expression)', async () => { @@ -210,29 +178,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 69, - "index": 215, - "line": 10, - }, - "fix": undefined, - "message": "Invalid variable: "block.settings.some-nonexistent-block-setting" was not found.", - "severity": 0, - "start": { - "character": 24, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///blocks/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "block.settings.some-nonexistent-block-setting" was not found.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); }); it('reports an error for an invalid reference to a section schema (simple lookup)', async () => { @@ -244,29 +196,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 73, - "index": 223, - "line": 10, - }, - "fix": undefined, - "message": "Invalid variable: "section.settings.some-nonexistent-section-setting" was not found.", - "severity": 0, - "start": { - "character": 24, - "index": 174, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///sections/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "section.settings.some-nonexistent-section-setting" was not found.', + uri: 'file:///sections/example.liquid', + severity: 0, + }); }); it('reports an error for an invalid reference to a section schema (expression)', async () => { @@ -278,29 +214,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 73, - "index": 223, - "line": 10, - }, - "fix": undefined, - "message": "Invalid variable: "section.settings.some-nonexistent-section-setting" was not found.", - "severity": 0, - "start": { - "character": 24, - "index": 174, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///sections/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "section.settings.some-nonexistent-section-setting" was not found.', + uri: 'file:///sections/example.liquid', + severity: 0, + }); }); it('reports an error for an invalid reference to a global setting (in theme settings)', async () => { @@ -312,29 +232,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 60, - "index": 427, - "line": 19, - }, - "fix": undefined, - "message": "Invalid variable: "settings.some-non-existent-setting" was not found.", - "severity": 0, - "start": { - "character": 26, - "index": 393, - "line": 19, - }, - "suggest": undefined, - "type": "JSON", - "uri": "file:///config/settings_schema.json", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "settings.some-non-existent-setting" was not found.', + uri: 'file:///config/settings_schema.json', + severity: 0, + }); }); it('reports a detailed error when trying to use a block var in a section', async () => { @@ -346,29 +250,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 59, - "index": 209, - "line": 10, - }, - "fix": undefined, - "message": "Invalid visible_if: can't refer to "block" when not in a block file.", - "severity": 0, - "start": { - "character": 24, - "index": 174, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///sections/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid visible_if: can\'t refer to "block" when not in a block file.', + uri: 'file:///sections/example.liquid', + severity: 0, + }); }); it('reports a detailed error when trying to use a section var in a block', async () => { @@ -380,29 +268,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 59, - "index": 205, - "line": 10, - }, - "fix": undefined, - "message": "Invalid visible_if: can't refer to "section" when not in a section file.", - "severity": 0, - "start": { - "character": 24, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///blocks/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid visible_if: can\'t refer to "section" when not in a section file.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); }); it('reports a detailed error when trying to use a block var in theme settings', async () => { @@ -414,29 +286,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 59, - "index": 426, - "line": 19, - }, - "fix": undefined, - "message": "Invalid visible_if: can't refer to "block" when not in a block file.", - "severity": 0, - "start": { - "character": 26, - "index": 393, - "line": 19, - }, - "suggest": undefined, - "type": "JSON", - "uri": "file:///config/settings_schema.json", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid visible_if: can\'t refer to "block" when not in a block file.', + uri: 'file:///config/settings_schema.json', + severity: 0, + }); }); it('reports a detailed error when attempting to use a namespace as a var', async () => { @@ -448,29 +304,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 38, - "index": 184, - "line": 10, - }, - "fix": undefined, - "message": "Invalid variable: "block.settings" refers to a namespace, but is being used here as a variable.", - "severity": 0, - "start": { - "character": 24, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///blocks/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "block.settings" refers to a namespace, but is being used here as a variable.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); }); it('reports a detailed error when attempting to use a var as a namespace', async () => { @@ -482,29 +322,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 71, - "index": 217, - "line": 10, - }, - "fix": undefined, - "message": "Invalid variable: "block.settings.some-block-setting" refers to a variable, but is being used here as a namespace.", - "severity": 0, - "start": { - "character": 24, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///blocks/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "block.settings.some-block-setting" refers to a variable, but is being used here as a namespace.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); }); it('reports visible_if fields declared with incorrect or missing brackets', async () => { @@ -516,29 +340,13 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 77, - "index": 227, - "line": 10, - }, - "fix": undefined, - "message": "Invalid visible_if expression. It should take the form "{{ }}".", - "severity": 0, - "start": { - "character": 20, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///sections/example.liquid", - }, - ] - `); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid visible_if expression. It should take the form "{{ }}".', + uri: 'file:///sections/example.liquid', + severity: 0, + }); }); it('reports malformed visible_if fields', async () => { @@ -550,62 +358,31 @@ describe('Module: ValidVisibleIf', () => { }); const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 81, - "index": 231, - "line": 10, - }, - "fix": undefined, - "message": "Syntax error: cannot parse visible_if expression.", - "severity": 0, - "start": { - "character": 20, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///sections/example.liquid", - }, - ] - `); - }); - - it('reports when no variable lookup is found', async () => { - const themeData = structuredClone(baseThemeData); - - themeData['sections/example.liquid'].settings!.push({ - id: 'some-other-setting', - visible_if: '{{ "some random string" }}', + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Syntax error: cannot parse visible_if expression.', + uri: 'file:///sections/example.liquid', + severity: 0, }); - - const offenses = await checkRule(themeData); - expect(offenses).toMatchInlineSnapshot(` - [ - { - "check": "ValidVisibleIf", - "end": { - "character": 50, - "index": 200, - "line": 10, - }, - "fix": undefined, - "message": "visible_if expression contains no references to any settings. This is likely an error.", - "severity": 0, - "start": { - "character": 20, - "index": 170, - "line": 10, - }, - "suggest": undefined, - "type": "LiquidHtml", - "uri": "file:///sections/example.liquid", - }, - ] - `); }); + + // THIS WILL BE MOVED TO THE VisibleIfUsage check + // it('reports when no variable lookup is found', async () => { + // const themeData = structuredClone(baseThemeData); + + // themeData['sections/example.liquid'].settings!.push({ + // id: 'some-other-setting', + // visible_if: '{{ "some random string" }}', + // }); + + // const offenses = await checkRule(themeData); + // expect(offenses).to.have.length(1); + // expect(offenses).to.containOffense({ + // check: ValidVisibleIf.meta.code, + // message: 'visible_if expression contains no references to any settings. This may be an error.', + // uri: 'file:///sections/example.liquid', + // severity: 0, + // }); + // }); }); diff --git a/packages/theme-check-common/src/checks/valid-visible-if/index.ts b/packages/theme-check-common/src/checks/valid-visible-if/index.ts index 097ac8da8..12eeb26e0 100644 --- a/packages/theme-check-common/src/checks/valid-visible-if/index.ts +++ b/packages/theme-check-common/src/checks/valid-visible-if/index.ts @@ -1,20 +1,23 @@ import { type LiquidVariableLookup } from '@shopify/liquid-html-parser'; +import { nodeAtPath } from '../../json'; +import { getSchema, isBlockSchema, isSectionSchema } from '../../to-schema'; import { Severity, SourceCodeType, - type LiquidCheckDefinition, type JSONCheckDefinition, + type LiquidCheckDefinition, } from '../../types'; -import { getLocStart, nodeAtPath } from '../../json'; -import { getSchema, isBlockSchema, isSectionSchema } from '../../to-schema'; -import { reportWarning } from '../../utils'; +import { reportOnJsonNode } from '../../utils'; import { + buildLiquidLookupReport, getGlobalSettings, getVariableLookupsInExpression, - validateLookup, + lookupIsError, + lookupIsWarning, offsetAdjust, + validateLookupErrors, type Vars, -} from './visible-if-utils'; +} from '../../utils/visible-if-utils'; // Note that unlike most other files in the `checks` directory, this exports two // checks: one for Liquid files and one for 'config/settings_schema.json'. They @@ -58,7 +61,7 @@ export const ValidVisibleIf: LiquidCheckDefinition = { const offset = node.blockStartPosition.end; const settings = Object.fromEntries( - (await getGlobalSettings(context)).map((s) => [s, true] as const), + (await getGlobalSettings(context)).map((s) => [s.id, true] as const), ); const currentFileSettings = Object.fromEntries( validSchema.settings.map((setting) => [setting.id, true] as const), @@ -76,29 +79,21 @@ export const ValidVisibleIf: LiquidCheckDefinition = { const visibleIfNode = nodeAtPath(ast, ['settings', i, 'visible_if'])!; - const varLookupsOrWarning = getVariableLookupsInExpression(setting.visible_if); - if ('warning' in varLookupsOrWarning) { - reportWarning(varLookupsOrWarning.warning, offset, visibleIfNode, context); + const varLookups = getVariableLookupsInExpression(setting.visible_if); + if (lookupIsError(varLookups)) { + reportOnJsonNode(varLookups.error, offset, visibleIfNode, context); continue; } - const report = (message: string | null, lookup: LiquidVariableLookup) => { - if (typeof message === 'string') { - context.report({ - message, - // the JSONNode start location returned by `getLocStart` - // includes the opening quotation mark — whereas when we parse - // the inner expression, 0 is the location _inside_ the quotes. - // we add 1 to the offsets to compensate. - startIndex: - offset + getLocStart(visibleIfNode) + lookup.position.start + offsetAdjust + 1, - endIndex: - offset + getLocStart(visibleIfNode) + lookup.position.end + offsetAdjust + 1, - }); - } - }; + if (lookupIsWarning(varLookups)) { + // Skip validation warnings from the expression parsing. + // Those warnings will be reported in the VisibleIfUsage check. + continue; + } + + const report = buildLiquidLookupReport(context, offset, visibleIfNode); - for (const lookup of varLookupsOrWarning) { + for (const lookup of varLookups) { if (lookup.name === 'section' && !isSectionSchema(schema)) { report( `Invalid visible_if: can't refer to "section" when not in a section file.`, @@ -110,7 +105,7 @@ export const ValidVisibleIf: LiquidCheckDefinition = { lookup, ); } else { - report(validateLookup(lookup, vars), lookup); + report(validateLookupErrors(lookup, vars), lookup); } } } @@ -133,18 +128,24 @@ export const ValidVisibleIfSettingsSchema: JSONCheckDefinition = { if (typeof visibleIfExpression !== 'string') return; const offset = node.value.loc.start.offset; - const varLookupsOrWarning = getVariableLookupsInExpression(visibleIfExpression); - if ('warning' in varLookupsOrWarning) { + const varLookups = getVariableLookupsInExpression(visibleIfExpression); + if (lookupIsError(varLookups)) { context.report({ - message: varLookupsOrWarning.warning, + message: varLookups.error, startIndex: node.value.loc.start.offset, endIndex: node.value.loc.end.offset, }); return; } + if (lookupIsWarning(varLookups)) { + // Skip validation warnings from the expression parsing. + // Those warnings will be reported in the VisibleIfUsage check. + return; + } + const settings = Object.fromEntries( - (await getGlobalSettings(context)).map((s) => [s, true] as const), + (await getGlobalSettings(context)).map((s) => [s.id, true] as const), ); const vars: Vars = { settings }; @@ -159,7 +160,7 @@ export const ValidVisibleIfSettingsSchema: JSONCheckDefinition = { } }; - for (const lookup of varLookupsOrWarning) { + for (const lookup of varLookups) { // settings_schema.json can't reference `section` or `block`. if (lookup.name === 'section') { report( @@ -169,7 +170,7 @@ export const ValidVisibleIfSettingsSchema: JSONCheckDefinition = { } else if (lookup.name === 'block') { report(`Invalid visible_if: can't refer to "block" when not in a block file.`, lookup); } else { - report(validateLookup(lookup, vars), lookup); + report(validateLookupErrors(lookup, vars), lookup); } } }, diff --git a/packages/theme-check-common/src/checks/visible-if-usage/index.spec.ts b/packages/theme-check-common/src/checks/visible-if-usage/index.spec.ts new file mode 100644 index 000000000..a3df57f1f --- /dev/null +++ b/packages/theme-check-common/src/checks/visible-if-usage/index.spec.ts @@ -0,0 +1,387 @@ +import { expect, describe, it } from 'vitest'; +import { check } from '../../test'; +import { ValidVisibleIf, ValidVisibleIfSettingsSchema } from './index'; + +type ThemeSchema = { [key: string]: unknown; settings?: Record[] }; +type Theme = { [key: `${string}.json`]: ThemeSchema[]; [key: `${string}.liquid`]: ThemeSchema }; + +const toLiquid = (schema: unknown) => ` + {% schema %} + ${JSON.stringify(schema, undefined, 2)} + {% endschema %} +`; + +const makeTheme = (themeData: Record) => + Object.fromEntries( + Object.entries(themeData).map(([key, value]) => [ + key, + key.endsWith('.json') ? JSON.stringify(value, undefined, 2) : toLiquid(value), + ]), + ); + +const checkRule = (themeData: Theme) => + check(makeTheme(themeData), [ValidVisibleIf, ValidVisibleIfSettingsSchema]); + +const baseThemeData: Theme = { + 'sections/example.liquid': { + name: 'My Section', + settings: [{ id: 'some-section-setting' }], + }, + 'blocks/example.liquid': { + name: 'My Block', + settings: [{ id: 'some-block-setting' }], + }, + 'config/settings_schema.json': [ + { + name: 'theme_info', + theme_name: 'Test Theme', + theme_version: '0.1.0', + theme_author: 'Shopify', + }, + { + name: 't:some.namespace', + settings: [ + { type: 'header', content: 't:some.other.namespace' }, + { type: 'text', id: 'some-global-setting' }, + ], + }, + ], +}; + +describe('Module: ValidVisibleIf', () => { + it('reports no error for a valid reference to a block schema setting (simple lookup)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ block.settings.some-block-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).toEqual([]); + }); + + it('reports no error for a valid reference to a block schema setting (expression)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ block.settings.some-block-setting != "null" }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).toEqual([]); + }); + + it('reports no error for a valid reference to a global setting (simple lookup)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ settings.some-global-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).toEqual([]); + }); + + it('reports no error for a valid reference to a global setting (expression)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ "null" != settings.some-global-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).toEqual([]); + }); + + it('reports no error for a valid reference to a section schema (simple lookup)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ section.settings.some-section-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).toEqual([]); + }); + + it('reports no error for a valid reference to a section schema (expression)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ section.settings.some-section-setting != "null" }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).toEqual([]); + }); + + it('reports no error for a valid reference to a global setting (in theme settings)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['config/settings_schema.json'].at(-1)!.settings!.push({ + id: 'some-other-setting', + visible_if: '{{ settings.some-global-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).toEqual([]); + }); + + it('reports an error for an invalid reference to a global variable (in block)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ some-nonexistent-whatever }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "some-nonexistent-whatever" was not found.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); + }); + + it('reports an error for an invalid reference to a block schema setting (simple lookup)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ block.settings.some-nonexistent-block-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "block.settings.some-nonexistent-block-setting" was not found.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); + }); + + it('reports an error for an invalid reference to a block schema setting (expression)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ block.settings.some-nonexistent-block-setting != "null" }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "block.settings.some-nonexistent-block-setting" was not found.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); + }); + + it('reports an error for an invalid reference to a section schema (simple lookup)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ section.settings.some-nonexistent-section-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "section.settings.some-nonexistent-section-setting" was not found.', + uri: 'file:///sections/example.liquid', + severity: 0, + }); + }); + + it('reports an error for an invalid reference to a section schema (expression)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ section.settings.some-nonexistent-section-setting != "null" }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "section.settings.some-nonexistent-section-setting" was not found.', + uri: 'file:///sections/example.liquid', + severity: 0, + }); + }); + + it('reports an error for an invalid reference to a global setting (in theme settings)', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['config/settings_schema.json'].at(-1)!.settings!.push({ + id: 'some-other-setting', + visible_if: '{{ settings.some-non-existent-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "settings.some-non-existent-setting" was not found.', + uri: 'file:///config/settings_schema.json', + severity: 0, + }); + }); + + it('reports a detailed error when trying to use a block var in a section', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ block.settings.some-section-setting != "null" }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid visible_if: can\'t refer to "block" when not in a block file.', + uri: 'file:///sections/example.liquid', + severity: 0, + }); + }); + + it('reports a detailed error when trying to use a section var in a block', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ section.settings.some-block-setting != "null" }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid visible_if: can\'t refer to "section" when not in a section file.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); + }); + + it('reports a detailed error when trying to use a block var in theme settings', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['config/settings_schema.json'].at(-1)!.settings!.push({ + id: 'some-other-setting', + visible_if: '{{ block.settings.some-block-setting }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid visible_if: can\'t refer to "block" when not in a block file.', + uri: 'file:///config/settings_schema.json', + severity: 0, + }); + }); + + it('reports a detailed error when attempting to use a namespace as a var', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ block.settings }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "block.settings" refers to a namespace, but is being used here as a variable.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); + }); + + it('reports a detailed error when attempting to use a var as a namespace', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['blocks/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ block.settings.some-block-setting.another-thing }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid variable: "block.settings.some-block-setting" refers to a variable, but is being used here as a namespace.', + uri: 'file:///blocks/example.liquid', + severity: 0, + }); + }); + + it('reports visible_if fields declared with incorrect or missing brackets', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{% section.settings.some-section-setting != "null" %}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Invalid visible_if expression. It should take the form "{{ }}".', + uri: 'file:///sections/example.liquid', + severity: 0, + }); + }); + + it('reports malformed visible_if fields', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ section.settings.some-section-setting !r=erf "null" }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'Syntax error: cannot parse visible_if expression.', + uri: 'file:///sections/example.liquid', + severity: 0, + }); + }); + + it('reports when no variable lookup is found', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + id: 'some-other-setting', + visible_if: '{{ "some random string" }}', + }); + + const offenses = await checkRule(themeData); + expect(offenses).to.have.length(1); + expect(offenses).to.containOffense({ + check: ValidVisibleIf.meta.code, + message: 'visible_if expression contains no references to any settings. This may be an error.', + uri: 'file:///sections/example.liquid', + severity: 0, + }); + }); +}); diff --git a/packages/theme-check-common/src/checks/visible-if-usage/index.ts b/packages/theme-check-common/src/checks/visible-if-usage/index.ts new file mode 100644 index 000000000..26a9eeae5 --- /dev/null +++ b/packages/theme-check-common/src/checks/visible-if-usage/index.ts @@ -0,0 +1,172 @@ +import { type LiquidVariableLookup } from '@shopify/liquid-html-parser'; +import { + Severity, + SourceCodeType, + type LiquidCheckDefinition, + type JSONCheckDefinition, +} from '../../types'; +import { getLocStart, nodeAtPath } from '../../json'; +import { getSchema, isBlockSchema, isSectionSchema } from '../../to-schema'; +import { reportOnJsonNode } from '../../utils'; +import { + getGlobalSettings, + getVariableLookupsInExpression, + lookupIsError, + validateLookupErrors, + offsetAdjust, + type Vars, + buildLiquidLookupReport, + lookupIsWarning, + validateLookupWarnings, +} from '../../utils/visible-if-utils'; + +// Note that unlike most other files in the `checks` directory, this exports two +// checks: one for Liquid files and one for 'config/settings_schema.json'. They +// perform the same check using the same logic (modulo differences extracting +// the schema and determining warning start and end indices). + +const meta = { + code: 'VisibleIfUsage', + name: 'Validate visible_if expressions', + docs: { + description: 'Ensures visible_if expressions are defined with recommended usage patterns.', + recommended: true, + url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/valid-visible-if', + }, + severity: Severity.WARNING, + schema: {}, + targets: [], +}; + +export const VisibleIfUsage: LiquidCheckDefinition = { + meta: { ...meta, type: SourceCodeType.LiquidHtml }, + + create(context) { + return { + async LiquidRawTag(node) { + if (node.name !== 'schema' || node.body.kind !== 'json') return; + + const schema = await getSchema(context); + + const { validSchema, ast } = schema ?? {}; + if ( + !validSchema || + validSchema instanceof Error || + !validSchema.settings?.some((setting) => 'visible_if' in setting) || + !ast || + ast instanceof Error + ) { + return; + } + + const allGlobalSettings = await getGlobalSettings(context); + const settings = Object.fromEntries( + (await getGlobalSettings(context)).map((s) => [s.id, s.type] as const), + ); + const currentFileSettings = Object.fromEntries( + validSchema.settings.map((setting) => [setting.id, setting.type] as const), + ); + // TODO get the types from these settings ^ + + const vars: Vars = { settings }; + if (isSectionSchema(schema)) { + vars.section = { settings: currentFileSettings }; + } else if (isBlockSchema(schema)) { + vars.block = { settings: currentFileSettings }; + } + + const offset = node.blockStartPosition.end; + for (const [i, setting] of validSchema.settings.entries()) { + if (!('visible_if' in setting) || typeof setting.visible_if !== 'string') continue; + + const visibleIfNode = nodeAtPath(ast, ['settings', i, 'visible_if'])!; + + const varLookups = getVariableLookupsInExpression(setting.visible_if); + if (lookupIsError(varLookups)) { + // Skip validation errors from the expression parsing. + // Those errors will be reported in the ValidVisibleIf check. + continue; + } + + if (lookupIsWarning(varLookups)) { + reportOnJsonNode(varLookups.warning, offset, visibleIfNode, context); + continue; + } + + const report = buildLiquidLookupReport(context, offset, visibleIfNode); + + for (const lookup of varLookups) { + if ( + (lookup.name === 'section' && !isSectionSchema(schema)) || + (lookup.name === 'block' && !isBlockSchema(schema)) + ) { + // Skip validation on the var lookups when there is any errors present. + // Those errors will be reported in the ValidVisibleIf check. + continue; + } else { + report(validateLookupWarnings(lookup, vars), lookup); + } + } + } + }, + }; + }, +}; + +// export const VisibleIfUsageSettingsSchema: JSONCheckDefinition = { +// meta: { ...meta, type: SourceCodeType.JSON }, + +// create(context) { +// const relativePath = context.toRelativePath(context.file.uri); +// if (relativePath !== 'config/settings_schema.json') return {}; + +// return { +// async Property(node) { +// if (node.key.value !== 'visible_if' || node.value.type !== 'Literal') return; +// const visibleIfExpression = node.value.value; +// if (typeof visibleIfExpression !== 'string') return; +// const offset = node.value.loc.start.offset; + +// const varLookups = getVariableLookupsInExpression(visibleIfExpression); +// if (lookupIsError(varLookups)) { +// context.report({ +// message: varLookups.error, +// startIndex: node.value.loc.start.offset, +// endIndex: node.value.loc.end.offset, +// }); +// return; +// } + +// const settings = Object.fromEntries( +// (await getGlobalSettings(context)).map((s) => [s.id, true] as const), +// ); + +// const vars: Vars = { settings }; + +// const report = (message: string | null, lookup: LiquidVariableLookup) => { +// if (typeof message === 'string') { +// context.report({ +// message, +// startIndex: offset + lookup.position.start + offsetAdjust + 1, +// endIndex: offset + lookup.position.end + offsetAdjust + 1, +// }); +// } +// }; + +// for (const lookup of varLookups) { +// // settings_schema.json can't reference `section` or `block`. +// if (lookup.name === 'section') { +// report( +// `Invalid visible_if: can't refer to "section" when not in a section file.`, +// lookup, +// ); +// } else if (lookup.name === 'block') { +// report(`Invalid visible_if: can't refer to "block" when not in a block file.`, lookup); +// } else { +// report(validateLookupErrors(lookup, vars), lookup); +// } +// } +// }, +// }; +// }, +// }; diff --git a/packages/theme-check-common/src/utils/block.ts b/packages/theme-check-common/src/utils/block.ts index 261502640..fcbc29d75 100644 --- a/packages/theme-check-common/src/utils/block.ts +++ b/packages/theme-check-common/src/utils/block.ts @@ -144,7 +144,7 @@ function validateBlockTargeting( : `Block type "${nestedBlock.type}" is not allowed in "${ parentNode.type }" blocks. Allowed types are: ${allowedBlockTypes.join(', ')}.`; - reportWarning(errorMessage, offset, typeNode, context); + reportOnJsonNode(errorMessage, offset, typeNode, context); } if ('blocks' in nestedBlock && nestedBlock.blocks) { @@ -209,7 +209,7 @@ export async function validateNestedBlocks( } } -export function reportWarning( +export function reportOnJsonNode( message: string, offset: number, astNode: JSONNode, diff --git a/packages/theme-check-common/src/utils/visible-if-utils.spec.ts b/packages/theme-check-common/src/utils/visible-if-utils.spec.ts new file mode 100644 index 000000000..c0b5a7667 --- /dev/null +++ b/packages/theme-check-common/src/utils/visible-if-utils.spec.ts @@ -0,0 +1,192 @@ +import { describe, it, expect, vi } from 'vitest'; +import { type LiquidVariableLookup, NodeTypes, type LiquidString } from '@shopify/liquid-html-parser'; +import { type Context, SourceCodeType } from '..'; +import { + lookupIsError, + getVariableLookupsInExpression, + validateLookupErrors, + getGlobalSettings, + type Vars, +} from './visible-if-utils'; + +describe('Module: visible-if-utils', () => { + describe('Function: lookupIsError', () => { + it('returns true for error objects', () => { + const error = { error: 'some error message' }; + expect(lookupIsError(error)).toBe(true); + }); + + it('returns false for lookup arrays', () => { + const lookups: LiquidVariableLookup[] = []; + expect(lookupIsError(lookups)).toBe(false); + }); + }); + + describe('Function: getVariableLookupsInExpression', () => { + it('returns lookups for valid expressions', () => { + const result = getVariableLookupsInExpression('{{ settings.foo }}'); + expect(lookupIsError(result)).toBe(false); + if (!lookupIsError(result)) { + expect(result[0].name).toBe('settings'); + expect((result[0].lookups[0] as LiquidString).value).toBe('foo'); + } + }); + + it('returns error for invalid expression format', () => { + const result = getVariableLookupsInExpression('settings.foo'); + expect(lookupIsError(result)).toBe(true); + if (lookupIsError(result)) { + expect(result.error).toBe('Invalid visible_if expression. It should take the form "{{ }}".'); + } + }); + + it('returns error for syntax errors', () => { + const result = getVariableLookupsInExpression('{{ settings.foo !@# }}'); + expect(lookupIsError(result)).toBe(true); + if (lookupIsError(result)) { + expect(result.error).toBe('Syntax error: cannot parse visible_if expression.'); + } + }); + + it('returns error when no variable lookups found', () => { + const result = getVariableLookupsInExpression('{{ "just a string" }}'); + expect(lookupIsError(result)).toBe(true); + if (lookupIsError(result)) { + expect(result.error).toBe('visible_if expression contains no references to any settings. This may be an error.'); + } + }); + }); + + describe('Function: validateLookupErrors', () => { + const vars: Vars = { + settings: { + foo: true, + bar: { + baz: true, + }, + }, + }; + + const makeLookup = (name: string, lookups: string[] = []): LiquidVariableLookup => ({ + type: NodeTypes.VariableLookup, + name, + lookups: lookups.map(value => ({ type: NodeTypes.String, value } as LiquidString)), + position: null!, + source: '', + }); + + it('returns null for valid lookups', () => { + const lookup = makeLookup('settings', ['foo']); + expect(validateLookupErrors(lookup, vars)).toBeNull(); + }); + + it('returns error for non-existent variables', () => { + const lookup = makeLookup('settings', ['nonexistent']); + expect(validateLookupErrors(lookup, vars)).toBe('Invalid variable: "settings.nonexistent" was not found.'); + }); + + it('returns error when using variable as namespace', () => { + const lookup = makeLookup('settings', ['foo', 'invalid']); + expect(validateLookupErrors(lookup, vars)).toBe( + 'Invalid variable: "settings.foo" refers to a variable, but is being used here as a namespace.', + ); + }); + + it('returns error when using namespace as variable', () => { + const lookup = makeLookup('settings', ['bar']); + expect(validateLookupErrors(lookup, vars)).toBe( + 'Invalid variable: "settings.bar" refers to a namespace, but is being used here as a variable.', + ); + }); + }); + + describe('Function: getGlobalSettings', () => { + const createMockContext = (fsOverrides = {}): Context => ({ + file: { + uri: 'file:///some/path', + type: SourceCodeType.JSON, + source: '', + ast: new Error(), + }, + fs: { + readFile: vi.fn(), + stat: vi.fn(), + readDirectory: vi.fn(), + ...fsOverrides, + }, + fileExists: vi.fn().mockResolvedValue(true), + } as unknown as Context); + + it('returns global settings from settings_schema.json', async () => { + const mockContext = createMockContext({ + readFile: vi.fn().mockResolvedValue( + JSON.stringify([ + { + name: 'theme_info', + theme_name: 'Test Theme', + }, + { + name: 'Colors', + settings: [ + { id: 'color_1', type: 'color' }, + { id: 'color_2', type: 'color' } + ], + }, + ]), + ), + }); + + const settings = await getGlobalSettings(mockContext); + expect(settings).toEqual([ + { id: 'color_1', type: 'color' }, + { id: 'color_2', type: 'color' } + ]); + }); + + it('returns empty array when settings_schema.json is not found', async () => { + const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const mockContext = createMockContext({ + readFile: vi.fn().mockRejectedValue(new Error('File not found')), + }); + + const settings = await getGlobalSettings(mockContext); + expect(settings).toEqual([]); + expect(consoleSpy).toHaveBeenCalled(); + consoleSpy.mockRestore(); + }); + + it('handles malformed settings_schema.json', async () => { + const mockContext = createMockContext({ + readFile: vi.fn().mockResolvedValue('invalid json'), + }); + + const settings = await getGlobalSettings(mockContext); + expect(settings).toEqual([]); + }); + + it('filters out settings without id or type', async () => { + const mockContext = createMockContext({ + readFile: vi.fn().mockResolvedValue( + JSON.stringify([ + { + name: 'Settings', + settings: [ + { id: 'valid_1', type: 'text' }, + { id: 'valid_2', type: 'color' }, + { id: 'missing_type' }, + { type: 'missing_id' }, + {} + ], + }, + ]), + ), + }); + + const settings = await getGlobalSettings(mockContext); + expect(settings).toEqual([ + { id: 'valid_1', type: 'text' }, + { id: 'valid_2', type: 'color' } + ]); + }); + }); +}); \ No newline at end of file diff --git a/packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts b/packages/theme-check-common/src/utils/visible-if-utils.ts similarity index 58% rename from packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts rename to packages/theme-check-common/src/utils/visible-if-utils.ts index 9db8c2abd..02250d2d1 100644 --- a/packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts +++ b/packages/theme-check-common/src/utils/visible-if-utils.ts @@ -1,20 +1,30 @@ import { type LiquidVariableLookup, toLiquidHtmlAST, NodeTypes } from '@shopify/liquid-html-parser'; -import type { Context, SourceCodeType } from '../..'; -import { findRoot } from '../../find-root'; -import { parseJSON } from '../../json'; -import { join } from '../../path'; -import { visit } from '../../visitor'; - -export type Vars = { [key: string]: Vars | true }; +import get from 'lodash/get'; +import type { Context, JSONNode, SourceCodeType } from '..'; +import { findRoot } from '../find-root'; +import { parseJSON, getLocStart } from '../json'; +import { join } from '../path'; +import { visit } from '../visitor'; +import { Setting } from '../types/schemas/setting'; +export type Vars = { [key: string]: Vars | true | string }; +type LookupError = { error: string }; +type LookupWarning = { warning: string }; +type LookupResult = LiquidVariableLookup[] | LookupError | LookupWarning; export const variableExpressionMatcher = /{{(.+?)}}/; export const adjustedPrefix = '{% if '; export const adjustedSuffix = ' %}{% endif %}'; export const offsetAdjust = '{{'.length - adjustedPrefix.length; -export function getVariableLookupsInExpression( - expression: string, -): LiquidVariableLookup[] | { warning: string } { +export function lookupIsError(lookup: LookupResult): lookup is LookupError { + return 'error' in lookup; +} + +export function lookupIsWarning(lookup: LookupResult): lookup is LookupWarning { + return 'warning' in lookup; +} + +export function getVariableLookupsInExpression(expression: string): LookupResult { // As of February 2025, parsers other than LiquidJS don't yet support // expressions in {{ variable }} tags. So we have to do something a little // gnarly — before parsing it we extract the expression from within the tag @@ -25,7 +35,7 @@ export function getVariableLookupsInExpression( const match = variableExpressionMatcher.exec(expression); if (match == null) { return { - warning: `Invalid visible_if expression. It should take the form "{{ }}".`, + error: `Invalid visible_if expression. It should take the form "{{ }}".`, }; } const unwrappedExpression = match[1]; @@ -54,7 +64,7 @@ export function getVariableLookupsInExpression( if (vars.length === 0) { return { - warning: `visible_if expression contains no references to any settings. This is likely an error.`, + warning: `visible_if expression contains no references to any settings. This may be an error.`, }; } @@ -64,14 +74,14 @@ export function getVariableLookupsInExpression( // Because of our hackish approach, the underlying error is likely to // include an incorrect character range and/or mention {% if %} tags. // Squelch the details and just report it as a simple syntax error. - return { warning: 'Syntax error: cannot parse visible_if expression.' }; + return { error: 'Syntax error: cannot parse visible_if expression.' }; } - return { warning: String(error) }; + return { error: String(error) }; } } -export function validateLookup(lookup: LiquidVariableLookup, vars: Vars): string | null { +export function validateLookupErrors(lookup: LiquidVariableLookup, vars: Vars): string | null { const normalized = getNormalizedLookups(lookup); const poppedSegments = []; @@ -105,6 +115,24 @@ export function validateLookup(lookup: LiquidVariableLookup, vars: Vars): string )}" refers to a namespace, but is being used here as a variable.`; } +export function validateLookupWarnings(lookup: LiquidVariableLookup, vars: Vars): string | null { + const normalized = getNormalizedLookups(lookup); + const settingType = get(vars, normalized); + + if ( + typeof settingType === 'string' && + (settingType === Setting.Type.Richtext || + settingType === Setting.Type.InlineRichtext || + settingType === Setting.Type.Html || + settingType === Setting.Type.Text || + settingType === Setting.Type.Textarea) + ) { + return `It is not recommended to use a ${settingType} setting in a visible_if expression.`; + } + + return null; +} + function getNormalizedLookups(lookup: LiquidVariableLookup) { const nestedLookups = lookup.lookups.map((lookup) => { if (lookup.type !== NodeTypes.String) { @@ -113,11 +141,15 @@ function getNormalizedLookups(lookup: LiquidVariableLookup) { return lookup.value; }); + if (lookup.name === null) { + return nestedLookups; + } + return [lookup.name, ...nestedLookups]; } export async function getGlobalSettings(context: Context) { - const globalSettings: string[] = []; + const globalSettings: Array<{ id: string; type: string }> = []; try { const path = join( @@ -130,7 +162,9 @@ export async function getGlobalSettings(context: Context) { for (const group of settings) { if ('settings' in group && Array.isArray(group.settings)) { globalSettings.push( - ...group.settings.map((setting: any) => setting.id).filter((id: any) => id), + ...group.settings + .filter((setting: any) => setting.id && setting.type) + .map((setting: any) => ({ id: setting.id, type: setting.type })), ); } } @@ -142,3 +176,24 @@ export async function getGlobalSettings(context: Context) { return globalSettings; } + +export function buildLiquidLookupReport( + context: Context, + offset: number, + visibleIfNode: JSONNode, +) { + // the JSONNode start location returned by `getLocStart` + // includes the opening quotation mark — whereas when we parse + // the inner expression, 0 is the location _inside_ the quotes. + // we add 1 to the offsets to compensate. + const generalOffset = offset + getLocStart(visibleIfNode) + offsetAdjust + 1; + + return (message: string | null, lookup: LiquidVariableLookup) => { + if (typeof message === 'string') { + const startIndex = lookup.position.start + generalOffset; + const endIndex = lookup.position.end + generalOffset; + + context.report({ message, startIndex, endIndex }); + } + }; +} diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index 87d260b65..5a3eb1234 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -158,3 +158,6 @@ VariableName: enabled: true severity: 1 format: snake_case +VisibleIfUsage: + enabled: true + severity: 1 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index 7001e83af..84afc18b2 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -136,3 +136,6 @@ VariableName: enabled: true severity: 1 format: snake_case +VisibleIfUsage: + enabled: true + severity: 1