diff --git a/.changeset/unsupported-visible-if.md b/.changeset/unsupported-visible-if.md new file mode 100644 index 000000000..c4388bd99 --- /dev/null +++ b/.changeset/unsupported-visible-if.md @@ -0,0 +1,6 @@ +--- +'@shopify/theme-check-common': minor +'@shopify/theme-check-node': minor +--- + +Add `UnsupportedVisibleIf` check, which warns when `visible_if` is set on a setting type that ignores it (resource pickers like `product` or `collection`, `color_scheme_group` and `color_palette`). Shopify accepts these files but always shows the setting in the theme editor. `ValidVisibleIf` no longer validates `visible_if` expressions on those setting types. diff --git a/packages/theme-check-common/src/checks/index.ts b/packages/theme-check-common/src/checks/index.ts index a29d25543..53d1bc6a0 100644 --- a/packages/theme-check-common/src/checks/index.ts +++ b/packages/theme-check-common/src/checks/index.ts @@ -68,6 +68,7 @@ import { ValidSettingsKey } from './valid-settings-key'; import { ValidStandardEventData } from './valid-standard-event-data'; import { ValidStaticBlockType } from './valid-static-block-type'; import { ValidVisibleIf, ValidVisibleIfSettingsSchema } from './valid-visible-if'; +import { UnsupportedVisibleIf, UnsupportedVisibleIfSettingsSchema } from './unsupported-visible-if'; import { VariableName } from './variable-name'; import { AppBlockMissingSchema } from './app-block-missing-schema'; import { UniqueSettingIds } from './unique-settings-id'; @@ -145,6 +146,8 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ UnrecognizedContentForArguments, UnrecognizedRenderSnippetArguments, UnsupportedDocTag, + UnsupportedVisibleIf, + UnsupportedVisibleIfSettingsSchema, UnusedAssign, UnusedDocParam, ValidBlockTarget, diff --git a/packages/theme-check-common/src/checks/unsupported-visible-if/index.spec.ts b/packages/theme-check-common/src/checks/unsupported-visible-if/index.spec.ts new file mode 100644 index 000000000..3130395da --- /dev/null +++ b/packages/theme-check-common/src/checks/unsupported-visible-if/index.spec.ts @@ -0,0 +1,142 @@ +import { expect, describe, it } from 'vitest'; +import { check } from '../../test'; +import { Severity } from '../../types'; +import { UnsupportedVisibleIf, UnsupportedVisibleIfSettingsSchema } from './index'; +import { VISIBLE_IF_UNSUPPORTED_SETTING_TYPES } from '../valid-visible-if/visible-if-utils'; + +const toLiquid = (schema: unknown) => ` + {% schema %} + ${JSON.stringify(schema, undefined, 2)} + {% endschema %} +`; + +const checkRule = (files: Record) => + check(files, [UnsupportedVisibleIf, UnsupportedVisibleIfSettingsSchema]); + +const message = (type: string) => + `visible_if is not supported on settings of type '${type}' and will be ignored. The setting is always visible in the theme editor.`; + +describe('Module: UnsupportedVisibleIf', () => { + it.each([...VISIBLE_IF_UNSUPPORTED_SETTING_TYPES])( + 'warns about visible_if on a %s setting in a section', + async (type) => { + const offenses = await checkRule({ + 'sections/example.liquid': toLiquid({ + name: 'Example', + settings: [ + { type: 'checkbox', id: 'show', label: 'Show' }, + { type, id: 'picker', label: 'Picker', visible_if: '{{ section.settings.show }}' }, + ], + }), + }); + + expect(offenses).toHaveLength(1); + expect(offenses[0].message).toBe(message(type)); + expect(offenses[0].severity).toBe(Severity.WARNING); + }, + ); + + it('warns about visible_if on an unsupported setting in a theme block', async () => { + const offenses = await checkRule({ + 'blocks/example.liquid': toLiquid({ + name: 'Example', + settings: [ + { type: 'checkbox', id: 'show', label: 'Show' }, + { + type: 'collection', + id: 'c', + label: 'Collection', + visible_if: '{{ block.settings.show }}', + }, + ], + }), + }); + + expect(offenses.map((o) => o.message)).toEqual([message('collection')]); + }); + + it("warns about visible_if on an unsupported setting in a section's local block", async () => { + const offenses = await checkRule({ + 'sections/example.liquid': toLiquid({ + name: 'Example', + blocks: [ + { + type: 'featured', + name: 'Featured', + settings: [ + { type: 'checkbox', id: 'show', label: 'Show' }, + { type: 'page', id: 'p', label: 'Page', visible_if: '{{ block.settings.show }}' }, + ], + }, + ], + }), + }); + + expect(offenses.map((o) => o.message)).toEqual([message('page')]); + }); + + it('highlights the visible_if value', async () => { + const visibleIf = '{{ section.settings.show }}'; + const files = { + 'sections/example.liquid': toLiquid({ + name: 'Example', + settings: [{ type: 'product', id: 'p', label: 'Product', visible_if: visibleIf }], + }), + }; + + const offenses = await checkRule(files); + const source = files['sections/example.liquid']; + + expect(offenses).toHaveLength(1); + expect(source.slice(offenses[0].start.index, offenses[0].end.index)).toBe(`"${visibleIf}"`); + }); + + it('does not report visible_if on supported setting types', async () => { + const offenses = await checkRule({ + 'sections/example.liquid': toLiquid({ + name: 'Example', + settings: [ + { type: 'checkbox', id: 'show', label: 'Show' }, + { type: 'text', id: 't', label: 'Text', visible_if: '{{ section.settings.show }}' }, + { + type: 'image_picker', + id: 'i', + label: 'Image', + visible_if: '{{ section.settings.show }}', + }, + ], + }), + }); + + expect(offenses).toEqual([]); + }); + + it('does not report unsupported setting types without visible_if', async () => { + const offenses = await checkRule({ + 'sections/example.liquid': toLiquid({ + name: 'Example', + settings: [{ type: 'product', id: 'p', label: 'Product' }], + }), + }); + + expect(offenses).toEqual([]); + }); + + it('warns about visible_if on unsupported settings in config/settings_schema.json', async () => { + const offenses = await checkRule({ + 'config/settings_schema.json': JSON.stringify([ + { + name: 'General', + settings: [ + { type: 'checkbox', id: 'show', label: 'Show' }, + { type: 'text', id: 't', label: 'Text', visible_if: '{{ settings.show }}' }, + { type: 'color_palette', id: 'palette', visible_if: '{{ settings.show }}' }, + { type: 'blog', id: 'b', label: 'Blog', visible_if: '{{ settings.show }}' }, + ], + }, + ]), + }); + + expect(offenses.map((o) => o.message)).toEqual([message('color_palette'), message('blog')]); + }); +}); diff --git a/packages/theme-check-common/src/checks/unsupported-visible-if/index.ts b/packages/theme-check-common/src/checks/unsupported-visible-if/index.ts new file mode 100644 index 000000000..3f26c3e45 --- /dev/null +++ b/packages/theme-check-common/src/checks/unsupported-visible-if/index.ts @@ -0,0 +1,107 @@ +import { + Severity, + SourceCodeType, + type JSONCheckDefinition, + type LiquidCheckDefinition, +} from '../../types'; +import { nodeAtPath } from '../../json'; +import { getSchema } from '../../to-schema'; +import { reportWarning } from '../../utils'; +import { + isVisibleIfUnsupportedSettingType, + settingTypeOf, + unsupportedVisibleIfMessage, +} from '../valid-visible-if/visible-if-utils'; + +// Like ValidVisibleIf, this exports two checks: one for section and block +// Liquid files, and one for 'config/settings_schema.json'. + +const meta = { + code: 'UnsupportedVisibleIf', + name: 'Report visible_if on setting types that ignore it', + docs: { + description: + 'Warns when visible_if is set on a setting type that does not support conditional visibility. Shopify accepts the file, but ignores visible_if and always shows the setting in the theme editor.', + recommended: true, + url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/unsupported-visible-if', + }, + severity: Severity.WARNING, + schema: {}, + targets: [], +}; + +type SettingLike = { type?: unknown; visible_if?: unknown }; + +export const UnsupportedVisibleIf: 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) return; + if (!ast || ast instanceof Error) return; + + const offset = node.blockStartPosition.end; + + const reportSettings = (settings: unknown, path: (string | number)[]) => { + if (!Array.isArray(settings)) return; + + settings.forEach((setting: SettingLike, i) => { + if (!setting || typeof setting !== 'object' || !('visible_if' in setting)) return; + if (!isVisibleIfUnsupportedSettingType(setting.type)) return; + + const visibleIfNode = nodeAtPath(ast, [...path, i, 'visible_if']); + if (!visibleIfNode) return; + + reportWarning( + unsupportedVisibleIfMessage(setting.type), + offset, + visibleIfNode, + context, + ); + }); + }; + + reportSettings(validSchema.settings, ['settings']); + + // Section files can define local blocks with their own settings. + const blocks = 'blocks' in validSchema ? validSchema.blocks : undefined; + if (Array.isArray(blocks)) { + blocks.forEach((block, j) => { + if (block && typeof block === 'object' && 'settings' in block) { + reportSettings(block.settings, ['blocks', j, 'settings']); + } + }); + } + }, + }; + }, +}; + +export const UnsupportedVisibleIfSettingsSchema: 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, ancestors) { + if (node.key.value !== 'visible_if') return; + + const type = settingTypeOf(ancestors.at(-1)); + if (!isVisibleIfUnsupportedSettingType(type)) return; + + context.report({ + message: unsupportedVisibleIfMessage(type), + startIndex: node.value.loc.start.offset, + endIndex: node.value.loc.end.offset, + }); + }, + }; + }, +}; 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 b529ee487..a7cc43290 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 @@ -620,6 +620,24 @@ describe('Module: ValidVisibleIf', () => { expect(offenses).to.containOffense('Syntax error: cannot parse visible_if expression.'); }); + it('does not validate visible_if on setting types that ignore it', async () => { + const themeData = structuredClone(baseThemeData); + + themeData['sections/example.liquid'].settings!.push({ + type: 'product', + id: 'some-product', + visible_if: '{{ section.settings.does-not-exist }}', + }); + themeData['config/settings_schema.json'][1].settings!.push({ + type: 'collection', + id: 'some-collection', + visible_if: 'not wrapped in brackets', + }); + + const offenses = await checkRule(themeData); + expect(offenses).toEqual([]); + }); + it('reports when no variable lookup is found', async () => { const themeData = structuredClone(baseThemeData); 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 e84eee5b3..9b03bb525 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 @@ -17,6 +17,8 @@ import { reportWarning } from '../../utils'; import { getGlobalSettings, getVariableLookupsInExpression, + isVisibleIfUnsupportedSettingType, + settingTypeOf, validateLookup, offsetAdjust, type Vars, @@ -93,6 +95,8 @@ export const ValidVisibleIf: LiquidCheckDefinition = { for (const [i, setting] of validSchema.settings.entries()) { if (!('visible_if' in setting) || typeof setting.visible_if !== 'string') continue; + // Ignored on these types; reported by UnsupportedVisibleIf instead. + if (isVisibleIfUnsupportedSettingType(setting.type)) continue; const visibleIfNode = nodeAtPath(ast, ['settings', i, 'visible_if'])!; @@ -157,8 +161,10 @@ export const ValidVisibleIfSettingsSchema: JSONCheckDefinition = { if (relativePath !== 'config/settings_schema.json') return {}; return { - async Property(node) { + async Property(node, ancestors) { if (node.key.value !== 'visible_if' || node.value.type !== 'Literal') return; + // Ignored on these types; reported by UnsupportedVisibleIfSettingsSchema instead. + if (isVisibleIfUnsupportedSettingType(settingTypeOf(ancestors.at(-1)))) return; const visibleIfExpression = node.value.value; if (typeof visibleIfExpression !== 'string') return; const offset = node.value.loc.start.offset; diff --git a/packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts b/packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts index 7e3c55e86..c0ba6d1cb 100644 --- a/packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts +++ b/packages/theme-check-common/src/checks/valid-visible-if/visible-if-utils.ts @@ -1,10 +1,46 @@ import { type LiquidVariableLookup, NodeTypes, toLiquidHtmlAST } from '@shopify/liquid-html-parser'; -import { Context, SourceCodeType } from '../..'; +import { Context, SourceCodeType, type JSONNode } from '../..'; import { parseJSON } from '../../json'; import { visit } from '../../visitor'; export type Vars = { [key: string]: Vars | true }; +/** + * Setting types that accept a `visible_if` property without failing to save, + * but where the theme editor ignores it and always shows the setting. + * Mirrors Theme::SettingTypeSpecs::CONDITIONAL_SETTING_UNSUPPORTED_TYPES in Shopify Core. + */ +export const VISIBLE_IF_UNSUPPORTED_SETTING_TYPES: ReadonlySet = new Set([ + 'article', + 'article_list', + 'blog', + 'collection', + 'collection_list', + 'metaobject', + 'metaobject_list', + 'page', + 'product', + 'product_list', + 'color_scheme_group', + 'color_palette', +]); + +export function isVisibleIfUnsupportedSettingType(type: unknown): type is string { + return typeof type === 'string' && VISIBLE_IF_UNSUPPORTED_SETTING_TYPES.has(type); +} + +export function settingTypeOf(node: JSONNode | undefined): string | undefined { + if (!node || node.type !== 'Object') return; + const typeProperty = node.children.find((child) => child.key.value === 'type'); + if (typeProperty?.value.type !== 'Literal') return; + const value = typeProperty.value.value; + return typeof value === 'string' ? value : undefined; +} + +export function unsupportedVisibleIfMessage(type: string): string { + return `visible_if is not supported on settings of type '${type}' and will be ignored. The setting is always visible in the theme editor.`; +} + export const variableExpressionMatcher = /{{(.+?)}}/; export const adjustedPrefix = '{% if '; export const adjustedSuffix = ' %}{% endif %}'; diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index 7b5b8dc91..661812f19 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -214,6 +214,9 @@ UnsupportedDocTag: UnsupportedFilterArguments: enabled: true severity: 0 +UnsupportedVisibleIf: + enabled: true + severity: 1 UnusedAssign: enabled: true severity: 1 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index 255e4136e..0797132bd 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -192,6 +192,9 @@ UnsupportedDocTag: UnsupportedFilterArguments: enabled: true severity: 0 +UnsupportedVisibleIf: + enabled: true + severity: 1 UnusedAssign: enabled: true severity: 1