-
Notifications
You must be signed in to change notification settings - Fork 37
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Refactor: Move render snippet type validations into their own check. …
…This allows users to disable it on its own
- Loading branch information
1 parent
536a1b2
commit 75ab8e0
Showing
7 changed files
with
291 additions
and
135 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
166 changes: 166 additions & 0 deletions
166
packages/theme-check-common/src/checks/valid-render-snippet-param-types/index.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,166 @@ | ||
import { describe, it, expect } from 'vitest'; | ||
import { runLiquidCheck } from '../../test'; | ||
import { ValidRenderSnippetParamTypes } from '.'; | ||
import { MockFileSystem } from '../../test'; | ||
import { SupportedParamTypes } from '../../liquid-doc/utils'; | ||
|
||
describe('Module: ValidRenderSnippetParamTypes', () => { | ||
describe('type validation', () => { | ||
const typeTests = [ | ||
{ | ||
type: 'string', | ||
validValues: ["'hello'", "''", 'product'], | ||
invalidValues: [ | ||
{ value: '123', actualType: SupportedParamTypes.Number }, | ||
{ value: 'true', actualType: SupportedParamTypes.Boolean }, | ||
], | ||
}, | ||
{ | ||
type: 'number', | ||
validValues: ['0', '123', '-1', 'product'], | ||
invalidValues: [ | ||
{ value: "'hello'", actualType: SupportedParamTypes.String }, | ||
{ value: 'true', actualType: SupportedParamTypes.Boolean }, | ||
], | ||
}, | ||
{ | ||
type: 'boolean', | ||
validValues: ['true', 'false', 'nil', 'empty', 'product'], | ||
invalidValues: [ | ||
{ value: "'hello'", actualType: SupportedParamTypes.String }, | ||
{ value: '123', actualType: SupportedParamTypes.Number }, | ||
], | ||
}, | ||
{ | ||
type: 'object', | ||
validValues: ['product', '(1..3)'], | ||
invalidValues: [ | ||
{ value: "'hello'", actualType: SupportedParamTypes.String }, | ||
{ value: '123', actualType: SupportedParamTypes.Number }, | ||
{ value: 'true', actualType: SupportedParamTypes.Boolean }, | ||
{ value: 'empty', actualType: SupportedParamTypes.Boolean }, | ||
], | ||
}, | ||
]; | ||
|
||
for (const test of typeTests) { | ||
describe(`${test.type} validation`, () => { | ||
const makeSnippet = (type: string) => ` | ||
{% doc %} | ||
@param {${type}} param - Description | ||
{% enddoc %} | ||
<div>{{ param }}</div> | ||
`; | ||
|
||
test.validValues.forEach((value) => { | ||
it(`should accept ${value} for ${test.type}`, async () => { | ||
const fs = new MockFileSystem({ | ||
'snippets/card.liquid': makeSnippet(test.type), | ||
}); | ||
|
||
const sourceCode = `{% render 'card', param: ${value} %}`; | ||
const offenses = await runLiquidCheck( | ||
ValidRenderSnippetParamTypes, | ||
sourceCode, | ||
undefined, | ||
{ | ||
fs, | ||
}, | ||
); | ||
expect(offenses).toHaveLength(0); | ||
}); | ||
}); | ||
|
||
test.invalidValues.forEach(({ value, actualType: expectedType }) => { | ||
it(`should reject ${value} for ${test.type}`, async () => { | ||
const fs = new MockFileSystem({ | ||
'snippets/card.liquid': makeSnippet(test.type), | ||
}); | ||
|
||
const sourceCode = `{% render 'card', param: ${value} %}`; | ||
const offenses = await runLiquidCheck( | ||
ValidRenderSnippetParamTypes, | ||
sourceCode, | ||
undefined, | ||
{ | ||
fs, | ||
}, | ||
); | ||
expect(offenses).toHaveLength(1); | ||
expect(offenses[0].message).toBe( | ||
`Type mismatch for parameter 'param': expected ${test.type}, got ${expectedType}`, | ||
); | ||
}); | ||
}); | ||
}); | ||
} | ||
}); | ||
|
||
describe('edge cases', () => { | ||
it('should handle mixed case type annotations', async () => { | ||
const fs = new MockFileSystem({ | ||
'snippets/card.liquid': ` | ||
{% doc %} | ||
@param {String} text - The text | ||
@param {NUMBER} count - The count | ||
@param {BOOLEAN} flag - The flag | ||
@param {Object} data - The data | ||
{% enddoc %} | ||
<div>{{ text }}{{ count }}{{ flag }}{{ data }}</div> | ||
`, | ||
}); | ||
|
||
const sourceCode = `{% render 'card', text: "hello", count: 5, flag: true, data: product %}`; | ||
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, { | ||
fs, | ||
}); | ||
expect(offenses).toHaveLength(0); | ||
}); | ||
|
||
it('should ignore variable lookups', async () => { | ||
const fs = new MockFileSystem({ | ||
'snippets/card.liquid': ` | ||
{% doc %} | ||
@param {String} title - The title | ||
{% enddoc %} | ||
<div>{{ title }}</div> | ||
`, | ||
}); | ||
|
||
const sourceCode = `{% render 'card', title: product_title %}`; | ||
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, { | ||
fs, | ||
}); | ||
expect(offenses).toHaveLength(0); | ||
}); | ||
|
||
it('should not report when snippet has no doc comment', async () => { | ||
const fs = new MockFileSystem({ | ||
'snippets/card.liquid': `<h1>This snippet has no doc comment</h1>`, | ||
}); | ||
|
||
const sourceCode = `{% render 'card', title: 123 %}`; | ||
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, { | ||
fs, | ||
}); | ||
expect(offenses).toHaveLength(0); | ||
}); | ||
|
||
it('should not report for unrelated parameters', async () => { | ||
const fs = new MockFileSystem({ | ||
'snippets/card.liquid': ` | ||
{% doc %} | ||
@param {String} title - The title | ||
{% enddoc %} | ||
<div>{{ title }}</div> | ||
`, | ||
}); | ||
|
||
const sourceCode = `{% render 'card', title: "hello", unrelated: 123 %}`; | ||
const offenses = await runLiquidCheck(ValidRenderSnippetParamTypes, sourceCode, undefined, { | ||
fs, | ||
}); | ||
expect(offenses).toHaveLength(0); | ||
}); | ||
}); | ||
}); |
112 changes: 112 additions & 0 deletions
112
packages/theme-check-common/src/checks/valid-render-snippet-param-types/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,112 @@ | ||
import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; | ||
import { LiquidNamedArgument, NodeTypes, RenderMarkup } from '@shopify/liquid-html-parser'; | ||
import { toLiquidHtmlAST } from '@shopify/liquid-html-parser'; | ||
import { getSnippetDefinition, LiquidDocParameter } from '../../liquid-doc/liquidDoc'; | ||
import { isLiquidString } from '../utils'; | ||
import { inferArgumentType, getDefaultValueForType } from '../../liquid-doc/utils'; | ||
|
||
export const ValidRenderSnippetParamTypes: LiquidCheckDefinition = { | ||
meta: { | ||
code: 'ValidRenderSnippetParamTypes', | ||
name: 'Valid Render Snippet Parameter Types', | ||
|
||
docs: { | ||
description: | ||
'This check ensures that parameters passed to snippet match the expected types defined in the liquidDoc header if present.', | ||
recommended: true, | ||
url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/valid-render-snippet-param-types', | ||
}, | ||
type: SourceCodeType.LiquidHtml, | ||
severity: Severity.WARNING, | ||
schema: {}, | ||
targets: [], | ||
}, | ||
|
||
create(context) { | ||
function findTypeMismatchParams( | ||
liquidDocParameters: Map<string, LiquidDocParameter>, | ||
providedParams: LiquidNamedArgument[], | ||
) { | ||
const typeMismatchParams: LiquidNamedArgument[] = []; | ||
|
||
for (const arg of providedParams) { | ||
const liquidDocParamDef = liquidDocParameters.get(arg.name); | ||
if (liquidDocParamDef && liquidDocParamDef.type) { | ||
if (arg.value.type !== NodeTypes.VariableLookup) { | ||
if (inferArgumentType(arg) !== liquidDocParamDef.type?.toLowerCase()) { | ||
typeMismatchParams.push(arg); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return typeMismatchParams; | ||
} | ||
|
||
function reportTypeMismatches( | ||
typeMismatchParams: LiquidNamedArgument[], | ||
liquidDocParameters: Map<string, LiquidDocParameter>, | ||
) { | ||
for (const arg of typeMismatchParams) { | ||
const paramDef = liquidDocParameters.get(arg.name); | ||
if (!paramDef || !paramDef.type) continue; | ||
|
||
const expectedType = paramDef.type.toLowerCase(); | ||
const actualType = inferArgumentType(arg); | ||
|
||
context.report({ | ||
message: `Type mismatch for parameter '${arg.name}': expected ${expectedType}, got ${actualType}`, | ||
startIndex: arg.value.position.start, | ||
endIndex: arg.value.position.end, | ||
suggest: [ | ||
{ | ||
message: `Replace with default value '${getDefaultValueForType( | ||
expectedType, | ||
)}' for ${expectedType}`, | ||
fix: (fixer) => { | ||
return fixer.replace( | ||
arg.value.position.start, | ||
arg.value.position.end, | ||
getDefaultValueForType(expectedType), | ||
); | ||
}, | ||
}, | ||
{ | ||
message: `Remove value`, | ||
fix: (fixer) => { | ||
return fixer.remove(arg.value.position.start, arg.value.position.end); | ||
}, | ||
}, | ||
], | ||
}); | ||
} | ||
} | ||
|
||
return { | ||
async RenderMarkup(node: RenderMarkup) { | ||
if (!isLiquidString(node.snippet) || node.variable) { | ||
return; | ||
} | ||
|
||
const snippetName = node.snippet.value; | ||
const snippetPath = `snippets/${snippetName}.liquid`; | ||
const snippetUri = context.toUri(snippetPath); | ||
|
||
const snippetContent = await context.fs.readFile(snippetUri); | ||
const snippetAst = toLiquidHtmlAST(snippetContent); | ||
const snippetDef = getSnippetDefinition(snippetAst, snippetName); | ||
|
||
if (!snippetDef.liquidDoc?.parameters) { | ||
return; | ||
} | ||
|
||
const liquidDocParameters = new Map( | ||
snippetDef.liquidDoc.parameters.map((p) => [p.name, p]), | ||
); | ||
|
||
const typeMismatchParams = findTypeMismatchParams(liquidDocParameters, node.args); | ||
reportTypeMismatches(typeMismatchParams, liquidDocParameters); | ||
}, | ||
}; | ||
}, | ||
}; |
Oops, something went wrong.