Skip to content

Commit f69929d

Browse files
jamesmengojamesmengo
committed
Add a new theme check to validate the types of parameters passed to render snippets
- Reports type mismatches - Suggests autofixes (replace with default or remove value) - Skips type checking for variable lookups - We would need to evaluate this dynamically, so we should accept this until we can do so - Skips type checking for unknown parameters - Skips type checking for unknown types
1 parent 6dcdfc3 commit f69929d

File tree

7 files changed

+339
-3
lines changed

7 files changed

+339
-3
lines changed

‎packages/theme-check-common/src/checks/index.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import { ValidHTMLTranslation } from './valid-html-translation';
4242
import { ValidJSON } from './valid-json';
4343
import { ValidLocalBlocks } from './valid-local-blocks';
4444
import { ValidRenderSnippetParams } from './valid-render-snippet-params';
45+
import { ValidRenderSnippetParamTypes } from './valid-render-snippet-param-types';
4546
import { ValidSchema } from './valid-schema';
4647
import { ValidSchemaName } from './valid-schema-name';
4748
import { ValidSettingsKey } from './valid-settings-key';
@@ -102,6 +103,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [
102103
ValidVisibleIfSettingsSchema,
103104
VariableName,
104105
ValidRenderSnippetParams,
106+
ValidRenderSnippetParamTypes,
105107
ValidSchemaName,
106108
];
107109

‎packages/theme-check-common/src/checks/valid-render-snippet-param-types/index.spec.ts

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
import { describe, it, expect } from 'vitest';
2+
import { runLiquidCheck } from '../../test';
3+
import { ValidRenderSnippetParamTypes } from '.';
4+
import { MockFileSystem } from '../../test';
5+
import { SupportedParamTypes } from '../../liquid-doc/utils';
6+
7+
describe('Module: ValidRenderSnippetParamTypes', () => {
8+
describe('type validation', () => {
9+
const typeTests = [
10+
{
11+
type: 'string',
12+
validValues: ["'hello'", "''", 'product'],
13+
invalidValues: [
14+
{ value: '123', actualType: SupportedParamTypes.Number },
15+
{ value: 'true', actualType: SupportedParamTypes.Boolean },
16+
],
17+
},
18+
{
19+
type: 'number',
20+
validValues: ['0', '123', '-1', 'product'],
21+
invalidValues: [
22+
{ value: "'hello'", actualType: SupportedParamTypes.String },
23+
{ value: 'true', actualType: SupportedParamTypes.Boolean },
24+
],
25+
},
26+
{
27+
type: 'boolean',
28+
validValues: ['true', 'false', 'nil', 'empty', 'product'],
29+
invalidValues: [
30+
{ value: "'hello'", actualType: SupportedParamTypes.String },
31+
{ value: '123', actualType: SupportedParamTypes.Number },
32+
],
33+
},
34+
{
35+
type: 'object',
36+
validValues: ['product', '(1..3)'],
37+
invalidValues: [
38+
{ value: "'hello'", actualType: SupportedParamTypes.String },
39+
{ value: '123', actualType: SupportedParamTypes.Number },
40+
{ value: 'true', actualType: SupportedParamTypes.Boolean },
41+
{ value: 'empty', actualType: SupportedParamTypes.Boolean },
42+
],
43+
},
44+
];
45+
46+
for (const test of typeTests) {
47+
describe(`${test.type} validation`, () => {
48+
const makeSnippet = (type: string) => `
49+
{% doc %}
50+
@param {${type}} param - Description
51+
{% enddoc %}
52+
<div>{{ param }}</div>
53+
`;
54+
55+
test.validValues.forEach((value) => {
56+
it(`should accept ${value} for ${test.type}`, async () => {
57+
const fs = new MockFileSystem({
58+
'snippets/card.liquid': makeSnippet(test.type),
59+
});
60+
61+
const sourceCode = `{% render 'card', param: ${value} %}`;
62+
const offenses = await runLiquidCheck(
63+
ValidRenderSnippetParamTypes,
64+
sourceCode,
65+
undefined,
66+
{
67+
fs,
68+
},
69+
);
70+
expect(offenses).toHaveLength(0);
71+
});
72+
});
73+
74+
test.invalidValues.forEach(({ value, actualType: expectedType }) => {
75+
it(`should reject ${value} for ${test.type}`, async () => {
76+
const fs = new MockFileSystem({
77+
'snippets/card.liquid': makeSnippet(test.type),
78+
});
79+
80+
const sourceCode = `{% render 'card', param: ${value} %}`;
81+
const offenses = await runLiquidCheck(
82+
ValidRenderSnippetParamTypes,
83+
sourceCode,
84+
undefined,
85+
{
86+
fs,
87+
},
88+
);
89+
expect(offenses).toHaveLength(1);
90+
expect(offenses[0].message).toBe(
91+
`Type mismatch for parameter 'param': expected ${test.type}, got ${expectedType}`,
92+
);
93+
});
94+
});
95+
});
96+
}
97+
});
98+
99+
describe('edge cases', () => {
100+
it('should handle mixed case type annotations', async () => {
101+
const fs = new MockFileSystem({
102+
'snippets/card.liquid': `
103+
{% doc %}
104+
@param {String} text - The text
105+
@param {NUMBER} count - The count
106+
@param {BOOLEAN} flag - The flag
107+
@param {Object} data - The data
108+
{% enddoc %}
109+
<div>{{ text }}{{ count }}{{ flag }}{{ data }}</div>
110+
`,
111+
});
112+
113+
const sourceCode = `{% render 'card', text: "hello", count: 5, flag: true, data: product %}`;
114+
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, {
115+
fs,
116+
});
117+
expect(offenses).toHaveLength(0);
118+
});
119+
120+
it('should ignore variable lookups', async () => {
121+
const fs = new MockFileSystem({
122+
'snippets/card.liquid': `
123+
{% doc %}
124+
@param {String} title - The title
125+
{% enddoc %}
126+
<div>{{ title }}</div>
127+
`,
128+
});
129+
130+
const sourceCode = `{% render 'card', title: product_title %}`;
131+
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, {
132+
fs,
133+
});
134+
expect(offenses).toHaveLength(0);
135+
});
136+
137+
it('should not report when snippet has no doc comment', async () => {
138+
const fs = new MockFileSystem({
139+
'snippets/card.liquid': `<h1>This snippet has no doc comment</h1>`,
140+
});
141+
142+
const sourceCode = `{% render 'card', title: 123 %}`;
143+
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, {
144+
fs,
145+
});
146+
expect(offenses).toHaveLength(0);
147+
});
148+
149+
it('should not enforce unsupported types', async () => {
150+
const fs = new MockFileSystem({
151+
'snippets/card.liquid': `
152+
{% doc %}
153+
@param {Unsupported} title - The title
154+
{% enddoc %}
155+
<div>{{ title }}</div>
156+
`,
157+
});
158+
159+
const sourceCode = `{% render 'card', title: 123 %}`;
160+
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, {
161+
fs,
162+
});
163+
expect(offenses).toHaveLength(0);
164+
});
165+
166+
it('should not report for unrecognized parameters', async () => {
167+
const fs = new MockFileSystem({
168+
'snippets/card.liquid': `
169+
{% doc %}
170+
@param {String} title - The title
171+
{% enddoc %}
172+
<div>{{ title }}</div>
173+
`,
174+
});
175+
176+
const sourceCode = `{% render 'card', title: "hello", unrecognized: 123 %}`;
177+
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, {
178+
fs,
179+
});
180+
expect(offenses).toHaveLength(0);
181+
});
182+
});
183+
});

‎packages/theme-check-common/src/checks/valid-render-snippet-param-types/index.ts

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';
2+
import { LiquidNamedArgument, NodeTypes, RenderMarkup } from '@shopify/liquid-html-parser';
3+
import { toLiquidHtmlAST } from '@shopify/liquid-html-parser';
4+
import { getSnippetDefinition, LiquidDocParameter } from '../../liquid-doc/liquidDoc';
5+
import { isLiquidString } from '../utils';
6+
import {
7+
inferArgumentType,
8+
getDefaultValueForType,
9+
SupportedParamTypes,
10+
} from '../../liquid-doc/utils';
11+
12+
export const ValidRenderSnippetParamTypes: LiquidCheckDefinition = {
13+
meta: {
14+
code: 'ValidRenderSnippetParamTypes',
15+
name: 'Valid Render Snippet Parameter Types',
16+
17+
docs: {
18+
description:
19+
'This check ensures that parameters passed to snippet match the expected types defined in the liquidDoc header if present.',
20+
recommended: true,
21+
url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/valid-render-snippet-param-types',
22+
},
23+
type: SourceCodeType.LiquidHtml,
24+
severity: Severity.WARNING,
25+
schema: {},
26+
targets: [],
27+
},
28+
29+
create(context) {
30+
function findTypeMismatchParams(
31+
liquidDocParameters: Map<string, LiquidDocParameter>,
32+
providedParams: LiquidNamedArgument[],
33+
) {
34+
const typeMismatchParams: LiquidNamedArgument[] = [];
35+
36+
for (const arg of providedParams) {
37+
if (arg.value.type === NodeTypes.VariableLookup) {
38+
continue;
39+
}
40+
41+
const liquidDocParamDef = liquidDocParameters.get(arg.name);
42+
if (liquidDocParamDef && liquidDocParamDef.type) {
43+
const paramType = liquidDocParamDef.type.toLowerCase();
44+
const supportedTypes = Object.keys(SupportedParamTypes).map((type) => type.toLowerCase());
45+
if (!supportedTypes.includes(paramType)) {
46+
continue;
47+
}
48+
49+
if (inferArgumentType(arg) !== paramType) {
50+
typeMismatchParams.push(arg);
51+
}
52+
}
53+
}
54+
55+
return typeMismatchParams;
56+
}
57+
58+
function reportTypeMismatches(
59+
typeMismatchParams: LiquidNamedArgument[],
60+
liquidDocParameters: Map<string, LiquidDocParameter>,
61+
) {
62+
for (const arg of typeMismatchParams) {
63+
const paramDef = liquidDocParameters.get(arg.name);
64+
if (!paramDef || !paramDef.type) continue;
65+
66+
const expectedType = paramDef.type.toLowerCase();
67+
const actualType = inferArgumentType(arg);
68+
69+
context.report({
70+
message: `Type mismatch for parameter '${arg.name}': expected ${expectedType}, got ${actualType}`,
71+
startIndex: arg.value.position.start,
72+
endIndex: arg.value.position.end,
73+
suggest: [
74+
{
75+
message: `Replace with default value '${getDefaultValueForType(
76+
expectedType,
77+
)}' for ${expectedType}`,
78+
fix: (fixer) => {
79+
return fixer.replace(
80+
arg.value.position.start,
81+
arg.value.position.end,
82+
getDefaultValueForType(expectedType),
83+
);
84+
},
85+
},
86+
{
87+
message: `Remove value`,
88+
fix: (fixer) => {
89+
return fixer.remove(arg.value.position.start, arg.value.position.end);
90+
},
91+
},
92+
],
93+
});
94+
}
95+
}
96+
97+
return {
98+
async RenderMarkup(node: RenderMarkup) {
99+
if (!isLiquidString(node.snippet) || node.variable) {
100+
return;
101+
}
102+
103+
const snippetName = node.snippet.value;
104+
const snippetPath = `snippets/${snippetName}.liquid`;
105+
const snippetUri = context.toUri(snippetPath);
106+
107+
const snippetContent = await context.fs.readFile(snippetUri);
108+
const snippetAst = toLiquidHtmlAST(snippetContent);
109+
const snippetDef = getSnippetDefinition(snippetAst, snippetName);
110+
111+
if (!snippetDef.liquidDoc?.parameters) {
112+
return;
113+
}
114+
115+
const liquidDocParameters = new Map(
116+
snippetDef.liquidDoc.parameters.map((p) => [p.name, p]),
117+
);
118+
119+
const typeMismatchParams = findTypeMismatchParams(liquidDocParameters, node.args);
120+
reportTypeMismatches(typeMismatchParams, liquidDocParameters);
121+
},
122+
};
123+
},
124+
};

‎packages/theme-check-common/src/checks/valid-render-snippet-params/index.spec.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,8 +284,6 @@ describe('Module: ValidRenderSnippetParams', () => {
284284
});
285285

286286
expect(offenses).toHaveLength(0);
287-
288-
sourceCode = ``;
289287
});
290288
});
291289
});

‎packages/theme-check-common/src/liquid-doc/utils.ts

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
import { LiquidNamedArgument, NodeTypes } from '@shopify/liquid-html-parser';
2+
import { assertNever } from '../utils';
3+
14
export enum SupportedParamTypes {
25
String = 'string',
36
Number = 'number',
@@ -25,6 +28,26 @@ export function getDefaultValueForType(type: string | null) {
2528
case SupportedParamTypes.Object:
2629
return 'empty';
2730
default:
28-
return "''";
31+
return '';
32+
}
33+
}
34+
35+
/**
36+
* Casts the value of a LiquidNamedArgument to a string representing the type of the value.
37+
*/
38+
export function inferArgumentType(arg: LiquidNamedArgument): SupportedParamTypes {
39+
switch (arg.value.type) {
40+
case NodeTypes.String:
41+
return SupportedParamTypes.String;
42+
case NodeTypes.Number:
43+
return SupportedParamTypes.Number;
44+
case NodeTypes.LiquidLiteral:
45+
return SupportedParamTypes.Boolean;
46+
case NodeTypes.Range:
47+
case NodeTypes.VariableLookup:
48+
return SupportedParamTypes.Object;
49+
default:
50+
// This ensures that we have a case for every possible type for arg.value
51+
return assertNever(arg.value);
2952
}
3053
}

‎packages/theme-check-node/configs/all.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,9 @@ ValidJSON:
139139
ValidLocalBlocks:
140140
enabled: true
141141
severity: 0
142+
ValidRenderSnippetParamTypes:
143+
enabled: true
144+
severity: 1
142145
ValidRenderSnippetParams:
143146
enabled: true
144147
severity: 1

‎packages/theme-check-node/configs/recommended.yml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,9 @@ ValidJSON:
117117
ValidLocalBlocks:
118118
enabled: true
119119
severity: 0
120+
ValidRenderSnippetParamTypes:
121+
enabled: true
122+
severity: 1
120123
ValidRenderSnippetParams:
121124
enabled: true
122125
severity: 1

0 commit comments

Comments
 (0)