Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/unsupported-visible-if.md
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 3 additions & 0 deletions packages/theme-check-common/src/checks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -145,6 +146,8 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [
UnrecognizedContentForArguments,
UnrecognizedRenderSnippetArguments,
UnsupportedDocTag,
UnsupportedVisibleIf,
UnsupportedVisibleIfSettingsSchema,
UnusedAssign,
UnusedDocParam,
ValidBlockTarget,
Expand Down
Original file line number Diff line number Diff line change
@@ -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<string, string>) =>
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')]);
});
});
Original file line number Diff line number Diff line change
@@ -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,
});
},
};
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import { reportWarning } from '../../utils';
import {
getGlobalSettings,
getVariableLookupsInExpression,
isVisibleIfUnsupportedSettingType,
settingTypeOf,
validateLookup,
offsetAdjust,
type Vars,
Expand Down Expand Up @@ -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'])!;

Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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<string> = 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 %}';
Expand Down
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,9 @@ UnsupportedDocTag:
UnsupportedFilterArguments:
enabled: true
severity: 0
UnsupportedVisibleIf:
enabled: true
severity: 1
UnusedAssign:
enabled: true
severity: 1
Expand Down
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/recommended.yml
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,9 @@ UnsupportedDocTag:
UnsupportedFilterArguments:
enabled: true
severity: 0
UnsupportedVisibleIf:
enabled: true
severity: 1
UnusedAssign:
enabled: true
severity: 1
Expand Down
Loading