diff --git a/.changeset/await-block-validation.md b/.changeset/await-block-validation.md new file mode 100644 index 000000000..168caa19d --- /dev/null +++ b/.changeset/await-block-validation.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme-check-common': patch +--- + +Wait for nested block validation in `ValidBlockTarget` and referenced block validation in `ValidSettingsKey`. Neither was awaited, so their offenses could arrive after `check()` returned and be lost. diff --git a/.changeset/single-walk-liquid-checks.md b/.changeset/single-walk-liquid-checks.md new file mode 100644 index 000000000..9dd3ef475 --- /dev/null +++ b/.changeset/single-walk-liquid-checks.md @@ -0,0 +1,7 @@ +--- +'@shopify/theme-check-common': patch +--- + +Run all checks on a Liquid file in one walk of its AST, instead of one walk per check. + +Each check still runs its methods in the order it did before, each one settled before the next, without waiting for other checks, and a check that throws still stops on that file only. On Dawn and Horizon, `check()` returns the same offenses 2–3× faster. Offenses from different checks may come back in a different order. diff --git a/packages/theme-check-common/src/checks/valid-block-content-setting-type/index.spec.ts b/packages/theme-check-common/src/checks/valid-block-content-setting-type/index.spec.ts index abc50d33c..9abd98056 100644 --- a/packages/theme-check-common/src/checks/valid-block-content-setting-type/index.spec.ts +++ b/packages/theme-check-common/src/checks/valid-block-content-setting-type/index.spec.ts @@ -98,22 +98,24 @@ describe('ValidBlockContentSettingType', () => { ValidBlockArgumentTypes, ValidBlockContentSettingType, ]); + // Checks report in no particular order. + offenses.sort((a, b) => a.check.localeCompare(b.check)); expect(offenses).toMatchObject([ { - check: 'ValidBlockContentSettingType', + check: 'ValidBlockArgumentTypes', message: - "Schema setting 'content' has Liquid type 'number', but the built-in 'content' parameter has type 'string'.", + "The built-in parameter 'content' has Liquid type 'string', but LiquidDoc declares 'number'. The built-in parameter type is authoritative.", }, { - check: 'ValidBlockArgumentTypes', + check: 'ValidBlockContentSettingType', message: - "The built-in parameter 'content' has Liquid type 'string', but LiquidDoc declares 'number'. The built-in parameter type is authoritative.", + "Schema setting 'content' has Liquid type 'number', but the built-in 'content' parameter has type 'string'.", }, ]); expect(highlightedOffenses({ 'blocks/card.liquid': source }, offenses)).toEqual([ - '"number"', '{number}', + '"number"', ]); }); }); diff --git a/packages/theme-check-common/src/checks/valid-block-target/index.spec.ts b/packages/theme-check-common/src/checks/valid-block-target/index.spec.ts index 4a848b698..aa5a1b17d 100644 --- a/packages/theme-check-common/src/checks/valid-block-target/index.spec.ts +++ b/packages/theme-check-common/src/checks/valid-block-target/index.spec.ts @@ -1,6 +1,6 @@ import { expect, describe, it } from 'vitest'; import { ValidBlockTarget } from './index'; -import { check, MockTheme } from '../../test'; +import { check, MockFileSystem, MockTheme } from '../../test'; import { Preset, Setting } from '../../types/schemas'; describe('Module: ValidBlockTarget', () => { @@ -851,6 +851,10 @@ describe('Module: ValidBlockTarget', () => { expect(offenses[0].message).to.equal( 'Block type "image" is not allowed in "group" blocks. Allowed types are: text.', ); + + // Also when the file system answers slowly + const fs = new SlowFileSystem(theme); + expect(await check(theme, [ValidBlockTarget], { fs })).to.have.length(1); }); describe(`Static Blocks used in a ${path} file`, () => { @@ -1463,3 +1467,11 @@ describe('Module: ValidBlockTarget', () => { }); }); }); + +// Answers like MockFileSystem, a moment later. +class SlowFileSystem extends MockFileSystem { + async stat(uri: string) { + await new Promise((resolve) => setTimeout(resolve, 10)); + return super.stat(uri); + } +} diff --git a/packages/theme-check-common/src/checks/valid-settings-key/index.spec.ts b/packages/theme-check-common/src/checks/valid-settings-key/index.spec.ts index 69bccea7c..d40e35fbc 100644 --- a/packages/theme-check-common/src/checks/valid-settings-key/index.spec.ts +++ b/packages/theme-check-common/src/checks/valid-settings-key/index.spec.ts @@ -1,5 +1,7 @@ import { expect, describe, it } from 'vitest'; +import { toSchema, toSourceCode } from '../../index'; import { check } from '../../test'; +import { ThemeBlockSchema } from '../../types'; import { ValidSettingsKey } from './index'; describe('Module: ValidSettingsKey', () => { @@ -225,6 +227,28 @@ describe('Module: ValidSettingsKey', () => { `Setting 'non-existent-setting' does not exist in 'blocks/referenced.liquid'.`, ); }); + + it(`reports an error when ${label} block setting does not exist in a referenced file that loads slowly`, async () => { + const theme = { + ...referencedBlock, + 'sections/example.liquid': toLiquidFile({ + ...schemaTemplate, + ...blockTemplate([{ type: 'referenced', settings: { 'non-existent-setting': 'v' } }]), + }), + }; + const uri = 'file:///blocks/referenced.liquid'; + const source = toSourceCode(uri, theme['blocks/referenced.liquid']); + + const offenses = await check(theme, [ValidSettingsKey], { + async getBlockSchema() { + await new Promise((resolve) => setTimeout(resolve, 10)); + return toSchema('theme', uri, source, async () => true) as Promise< + ThemeBlockSchema | undefined + >; + }, + }); + expect(offenses).to.have.length(1); + }); }); describe('local blocks', () => { 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 0fbaebb53..83b859fd1 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 @@ -58,20 +58,36 @@ export const ValidSettingsKey: LiquidCheckDefinition = { validateSettingsKey(context, offset, settingsNode, validSchema.settings); // Check if default block settings match the settings defined in the block file's schema - validSchema.default.blocks?.forEach((block, i) => { - const settingsNode = nodeAtPath(ast, ['default', 'blocks', i, 'settings']); - - validateReferencedBlock(context, offset, settingsNode, rootLevelLocalBlocks, block); - }); + await Promise.all( + (validSchema.default.blocks ?? []).map((block, i) => { + const settingsNode = nodeAtPath(ast, ['default', 'blocks', i, 'settings']); + + return validateReferencedBlock( + context, + offset, + settingsNode, + rootLevelLocalBlocks, + block, + ); + }), + ); } // Check if preset block settings match the settings defined in the block file's schema for (const [_depthStr, blocks] of Object.entries(presetLevelBlocks)) { - blocks.forEach(({ node: blockNode, path }) => { - const settingsNode = nodeAtPath(ast, path.slice(0, -1).concat('settings')); - - validateReferencedBlock(context, offset, settingsNode, rootLevelLocalBlocks, blockNode); - }); + await Promise.all( + blocks.map(({ node: blockNode, path }) => { + const settingsNode = nodeAtPath(ast, path.slice(0, -1).concat('settings')); + + return validateReferencedBlock( + context, + offset, + settingsNode, + rootLevelLocalBlocks, + blockNode, + ); + }), + ); } }, }; diff --git a/packages/theme-check-common/src/index.spec.ts b/packages/theme-check-common/src/index.spec.ts index c514b44ad..a81b37679 100644 --- a/packages/theme-check-common/src/index.spec.ts +++ b/packages/theme-check-common/src/index.spec.ts @@ -3,8 +3,16 @@ import { MissingBlockArguments } from './checks/missing-block-arguments'; import { UnrecognizedBlockArguments } from './checks/unrecognized-block-arguments'; import { ValidBlockArgumentTypes } from './checks/valid-block-argument-types'; import { check } from './index'; -import { check as runChecks } from './test'; -import { type Setting, type ThemeBlock, ThemeSchemaType, type ThemeBlockSchema } from './types'; +import { getTheme, MockFileSystem, check as runChecks } from './test'; +import { + type LiquidCheckDefinition, + type Setting, + Severity, + SourceCodeType, + type ThemeBlock, + ThemeSchemaType, + type ThemeBlockSchema, +} from './types'; describe('Module: Hello World', () => { it('should validate that we can test files', () => { @@ -57,6 +65,127 @@ describe('check', () => { }); }); +describe('check on Liquid files', () => { + const files = { + 'snippets/a.liquid': '

{% if x %}{{ x }}{% endif %}

', + 'snippets/b.liquid': "{% render 'a' %}{% render 'a' %}", + }; + + function run(checks: LiquidCheckDefinition[], onError?: (error: Error) => void) { + const config = { context: 'theme' as const, settings: {}, checks, rootUri: 'file:/', onError }; + return check(getTheme(files), config, { fs: new MockFileSystem(files) }); + } + + // Logs each method it runs, which takes a moment when `wait` is set. + function logger(code: string, log: string[], wait = false) { + return liquidCheck(code, ({ file }) => { + const step = async (method: string) => { + log.push(`${file.uri} ${code} ${method} start`); + if (wait) await new Promise((resolve) => setTimeout(resolve, 1)); + log.push(`${file.uri} ${code} ${method} end`); + }; + return { + onCodePathStart: () => step('onCodePathStart'), + LiquidTag: (node) => step(node.name), + 'LiquidTag:exit': (node) => step(`${node.name}:exit`), + HtmlElement: () => step('HtmlElement'), + onCodePathEnd: () => step('onCodePathEnd'), + }; + }); + } + + const eventsOf = (log: string[], uri: string, code: string) => + log.filter((event) => event.startsWith(`${uri} ${code} `)); + + it("runs each check's methods in the order it runs them alone, each settled before the next", async () => { + const [fast, slow, together]: string[][] = [[], [], []]; + await run([logger('Fast', fast)]); + await run([logger('Slow', slow, true)]); + await run([logger('Fast', together), logger('Slow', together, true)]); + + for (const uri of ['file:///snippets/a.liquid', 'file:///snippets/b.liquid']) { + expect(eventsOf(together, uri, 'Fast')).toEqual(eventsOf(fast, uri, 'Fast')); + expect(eventsOf(together, uri, 'Slow')).toEqual(eventsOf(slow, uri, 'Slow')); + } + expect(eventsOf(slow, 'file:///snippets/b.liquid', 'Slow')).toEqual( + [ + 'onCodePathStart start', + 'onCodePathStart end', + 'render start', + 'render end', + 'render:exit start', + 'render:exit end', + 'render start', + 'render end', + 'render:exit start', + 'render:exit end', + 'onCodePathEnd start', + 'onCodePathEnd end', + ].map((event) => `file:///snippets/b.liquid Slow ${event}`), + ); + }); + + it("doesn't make a check wait for a slower one", async () => { + const log: string[] = []; + await run([logger('Fast', log), logger('Slow', log, true)]); + + const fastDone = log.indexOf('file:///snippets/a.liquid Fast onCodePathEnd end'); + const slowFirstDone = log.indexOf('file:///snippets/a.liquid Slow onCodePathStart end'); + expect(fastDone).toBeGreaterThan(-1); + expect(fastDone).toBeLessThan(slowFirstDone); + }); + + it('stops a check that throws on that file only, and reports its error once', async () => { + const calls: string[] = []; + const errors: Error[] = []; + const throws = liquidCheck('Throws', ({ file }) => ({ + async LiquidTag() { + calls.push(file.uri); + throw new Error(`Throws failed on ${file.uri}`); + }, + async onCodePathEnd() { + calls.push(`${file.uri} end`); + }, + })); + const empty = liquidCheck('Empty', () => undefined as any); + const reports = liquidCheck('Reports', (context) => ({ + async LiquidTag(node) { + context.report({ + message: node.name, + startIndex: node.position.start, + endIndex: node.position.end, + }); + }, + })); + + const offenses = await run([throws, empty, reports], (error) => errors.push(error)); + + expect(calls.sort()).toEqual(['file:///snippets/a.liquid', 'file:///snippets/b.liquid']); + expect(errors.map((error) => error.message).sort()).toEqual([ + expect.stringContaining("(reading 'onCodePathStart')"), + expect.stringContaining("(reading 'onCodePathStart')"), + 'Throws failed on file:///snippets/a.liquid', + 'Throws failed on file:///snippets/b.liquid', + ]); + expect(offenses.map((offense) => offense.message).sort()).toEqual(['if', 'render', 'render']); + }); +}); + +function liquidCheck(code: string, create: LiquidCheckDefinition['create']): LiquidCheckDefinition { + return { + meta: { + code, + name: code, + docs: { description: code }, + type: SourceCodeType.LiquidHtml, + severity: Severity.ERROR, + schema: {}, + targets: [], + }, + create, + }; +} + function setting(value: Record): Setting.InputSetting { return value as unknown as Setting.InputSetting; } diff --git a/packages/theme-check-common/src/index.ts b/packages/theme-check-common/src/index.ts index 9bd1bb0cf..72853b9b1 100644 --- a/packages/theme-check-common/src/index.ts +++ b/packages/theme-check-common/src/index.ts @@ -127,11 +127,12 @@ export async function check( const files = filesOfType(type, theme); const checkDefs = [DisabledChecksVisitor, ...checksOfType(type, config.checks)]; for (const file of files) { + const checks: LiquidCheck[] = []; for (const checkDef of checkDefs) { if (isIgnored(file.uri, config, checkDef)) continue; - const check = createCheck(checkDef, file, config, offenses, dependencies, validateJSON); - pipelines.push(checkLiquidFile(check, file)); + checks.push(createCheck(checkDef, file, config, offenses, dependencies, validateJSON)); } + pipelines.push(...checkLiquidFile(checks, file)); } break; } @@ -219,9 +220,48 @@ async function checkJSONFile(check: JSONCheck, file: JSONSourceCode): Promise { - if (check.onCodePathStart) await check.onCodePathStart(file); - if (file.ast instanceof Error) return; - if (Object.keys(check).length > 0) await visitLiquid(file.ast, check); - if (check.onCodePathEnd) await check.onCodePathEnd(file as typeof file & { ast: LiquidHtmlNode }); +/** + * Runs every check on a Liquid file in one walk of its AST, instead of one walk per check. The walk + * collects each check's calls; then each check makes its own, in walk order and each settled + * before the next, as when it walked the AST alone. Checks don't wait for each other. + * + * Returns one promise per check. A check that throws skips the rest of this file, and its promise + * rejects with the error. + */ +function checkLiquidFile(checks: LiquidCheck[], file: LiquidSourceCode): Promise[] { + if (checks.length === 0) return []; + + const calls: [method: keyof LiquidCheck, args: unknown[]][][] = checks.map(() => []); + const errors = new Map(); + const checksWithMethod = new Map(); + + const collect = (method: keyof LiquidCheck, ...args: unknown[]) => { + let found = checksWithMethod.get(method); + if (!found) { + found = []; + for (const [i, check] of checks.entries()) { + try { + if (check[method]) found.push(i); + } catch (error) { + // e.g. a check whose create() returned nothing + if (!errors.has(i)) errors.set(i, error); + } + } + checksWithMethod.set(method, found); + } + for (const i of found) calls[i].push([method, args]); + }; + + collect('onCodePathStart', file); + if (!(file.ast instanceof Error)) { + visitLiquid(file.ast, collect); + collect('onCodePathEnd', file); + } + + return checks.map(async (check, i) => { + if (errors.has(i)) throw errors.get(i); + for (const [method, args] of calls[i]) { + await (check as Record Promise>)[method](...args); + } + }); } diff --git a/packages/theme-check-common/src/utils/block.ts b/packages/theme-check-common/src/utils/block.ts index d87e60970..20582ac80 100644 --- a/packages/theme-check-common/src/utils/block.ts +++ b/packages/theme-check-common/src/utils/block.ts @@ -223,7 +223,7 @@ async function validateBlockTargeting( } if ('blocks' in nestedBlock && nestedBlock.blocks) { - validateNestedBlocks( + await validateNestedBlocks( context, nestedBlock, nestedBlock.blocks, @@ -254,7 +254,7 @@ export async function validateNestedBlocks( const allowedBlockTypes = rootLevelThemeBlocks.map((block) => block.node.type); if (Array.isArray(nestedBlocks)) { - Promise.all( + await Promise.all( nestedBlocks.map((nestedBlock, index) => { const nestedPath = currentPath.concat(['blocks', String(index), 'type']); return validateBlockTargeting( @@ -271,7 +271,7 @@ export async function validateNestedBlocks( }), ); } else if (typeof nestedBlocks === 'object') { - Promise.all( + await Promise.all( Object.entries(nestedBlocks).map(([key, nestedBlock]) => { const nestedPath = currentPath.concat(['blocks', key, 'type']); return validateBlockTargeting( diff --git a/packages/theme-check-common/src/visitors/liquid.ts b/packages/theme-check-common/src/visitors/liquid.ts index 41c78ddef..1b77d598c 100644 --- a/packages/theme-check-common/src/visitors/liquid.ts +++ b/packages/theme-check-common/src/visitors/liquid.ts @@ -1,20 +1,25 @@ import { nonTraversableProperties } from '@shopify/liquid-html-parser'; -import { LiquidHtmlNode, CheckNodeMethod, LiquidCheck, SourceCodeType } from '../types'; +import { LiquidHtmlNode, LiquidCheck } from '../types'; function isLiquidHtmlNode(thing: unknown): thing is LiquidHtmlNode { return !!thing && typeof thing === 'object' && 'type' in thing; } -export async function visitLiquid(node: LiquidHtmlNode, check: LiquidCheck): Promise { +/** + * Walks the AST, calling `visit` with the name of the check method for each node: its type on the + * way down, then `${type}:exit` once its children are queued. + */ +export function visitLiquid( + node: LiquidHtmlNode, + visit: (method: keyof LiquidCheck, node: LiquidHtmlNode, ancestors: LiquidHtmlNode[]) => void, +): void { const stack: { node: LiquidHtmlNode; ancestors: LiquidHtmlNode[] }[] = [{ node, ancestors: [] }]; - let method: CheckNodeMethod | undefined; while (stack.length > 0) { const { node, ancestors } = stack.pop()!; const lineage = ancestors.concat(node); - method = check[node.type]; - if (method) await method(node, ancestors); + visit(node.type, node, ancestors); for (const key in node) { if (!node.hasOwnProperty(key) || nonTraversableProperties.has(key)) { @@ -34,7 +39,6 @@ export async function visitLiquid(node: LiquidHtmlNode, check: LiquidCheck): Pro } } - method = check[`${node.type}:exit`]; - if (method) await method(node, ancestors); + visit(`${node.type}:exit`, node, ancestors); } }