From e0f63a0056474746ebb6ab0f7beb34e772cdc211 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claud=C3=A9ric=20Demers?= Date: Thu, 1 Oct 2026 08:24:18 -0400 Subject: [PATCH 1/3] Run all Liquid checks in one walk of the AST check() walked each Liquid file's AST once per check, about 70 walks per file with the recommended config. Run every check on a file in a single walk instead. At each node, the methods of every check that has one run together, so each check still sees its nodes in walk order with each method settled before its next, and a check that throws still stops on that file only and is reported once. Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/single-walk-liquid-checks.md | 7 ++ .../index.spec.ts | 12 +- packages/theme-check-common/src/index.spec.ts | 104 +++++++++++++++++- packages/theme-check-common/src/index.ts | 63 +++++++++-- .../theme-check-common/src/visitors/liquid.ts | 26 +++-- 5 files changed, 191 insertions(+), 21 deletions(-) create mode 100644 .changeset/single-walk-liquid-checks.md diff --git a/.changeset/single-walk-liquid-checks.md b/.changeset/single-walk-liquid-checks.md new file mode 100644 index 000000000..70f9ee2fb --- /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, 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/index.spec.ts b/packages/theme-check-common/src/index.spec.ts index c514b44ad..873af8874 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,98 @@ 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')).toHaveLength(12); + }); + + 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: 0 }); + }, + })); + + const offenses = await run([throws, empty, reports], (error) => errors.push(error)); + + expect(calls).toEqual(['file:///snippets/a.liquid', 'file:///snippets/b.liquid']); + expect(errors.map((error) => error.message).sort()).toEqual([ + expect.stringContaining('Cannot read'), + expect.stringContaining('Cannot read'), + '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..bbd25bea7 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,57 @@ 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. At each + * node, the checks' methods for it run together. Each check still sees its nodes in walk order, and + * each of its methods settles before its next one starts, as when it walked the AST on its own. + * + * Returns one promise per check. A check that throws stops on this file only, and its promise + * rejects with the error. + */ +function checkLiquidFile(checks: LiquidCheck[], file: LiquidSourceCode): Promise[] { + const errors = new Map(); + const checksWithMethod = new Map(); + + const run = (method: keyof LiquidCheck, ...args: unknown[]): Promise | undefined => { + let found = checksWithMethod.get(method); + if (!found) { + found = checks.filter((check) => { + try { + return !!check[method]; + } catch (error) { + // e.g. a check whose create() returned nothing + errors.set(check, error); + return false; + } + }); + checksWithMethod.set(method, found); + } + + const running = found.filter((check) => !errors.has(check)); + if (running.length === 0) return; + + return Promise.all( + running.map(async (check) => { + try { + await (check as Record Promise>)[method](...args); + } catch (error) { + errors.set(check, error); + } + }), + ); + }; + + const walked = (async () => { + await run('onCodePathStart', file); + if (file.ast instanceof Error) return; + await visitLiquid(file.ast, run); + await run('onCodePathEnd', file); + })(); + + return checks.map((check) => + walked.then(() => { + if (errors.has(check)) throw errors.get(check); + }), + ); } diff --git a/packages/theme-check-common/src/visitors/liquid.ts b/packages/theme-check-common/src/visitors/liquid.ts index 41c78ddef..a4b95c29e 100644 --- a/packages/theme-check-common/src/visitors/liquid.ts +++ b/packages/theme-check-common/src/visitors/liquid.ts @@ -1,20 +1,32 @@ 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. Waits for `visit` when it returns a + * promise. + */ +export async function visitLiquid( + node: LiquidHtmlNode, + visit: ( + method: keyof LiquidCheck, + node: LiquidHtmlNode, + ancestors: LiquidHtmlNode[], + ) => Promise | undefined, +): Promise { const stack: { node: LiquidHtmlNode; ancestors: LiquidHtmlNode[] }[] = [{ node, ancestors: [] }]; - let method: CheckNodeMethod | undefined; + let visiting: Promise | 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); + visiting = visit(node.type, node, ancestors); + if (visiting) await visiting; for (const key in node) { if (!node.hasOwnProperty(key) || nonTraversableProperties.has(key)) { @@ -34,7 +46,7 @@ export async function visitLiquid(node: LiquidHtmlNode, check: LiquidCheck): Pro } } - method = check[`${node.type}:exit`]; - if (method) await method(node, ancestors); + visiting = visit(`${node.type}:exit`, node, ancestors); + if (visiting) await visiting; } } From 835125525a0c5f9bac817a2a7c745cf2ab533427 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claud=C3=A9ric=20Demers?= Date: Thu, 1 Oct 2026 09:29:02 -0400 Subject: [PATCH 2/3] Await nested and referenced block validation validateNestedBlocks started its Promise.all without awaiting it, and ValidSettingsKey called the async validateReferencedBlock from forEach. Their offenses could arrive after check() returned and be lost; a slow file system or block schema makes it happen on main. Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/await-block-validation.md | 5 +++ .../checks/valid-block-target/index.spec.ts | 14 +++++++- .../checks/valid-settings-key/index.spec.ts | 24 +++++++++++++ .../src/checks/valid-settings-key/index.ts | 36 +++++++++++++------ .../theme-check-common/src/utils/block.ts | 6 ++-- 5 files changed, 71 insertions(+), 14 deletions(-) create mode 100644 .changeset/await-block-validation.md 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/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/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( From 3e581428f658423dbae8c75004eb08712bbbd8b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Claud=C3=A9ric=20Demers?= Date: Thu, 1 Oct 2026 09:29:03 -0400 Subject: [PATCH 3/3] Don't make Liquid checks wait for each other Running every check's methods for a node together made each check wait at every node for the slowest one there, so I/O from different checks on the same file no longer overlapped: up to 3x slower than main on a cold file system, 2x with remote assets. The walk now collects each check's calls, and each check makes its own in order, as before. Co-Authored-By: Claude Opus 5.5 (1M context) --- .changeset/single-walk-liquid-checks.md | 2 +- packages/theme-check-common/src/index.spec.ts | 39 ++++++++++-- packages/theme-check-common/src/index.ts | 63 ++++++++----------- .../theme-check-common/src/visitors/liquid.ts | 20 ++---- 4 files changed, 68 insertions(+), 56 deletions(-) diff --git a/.changeset/single-walk-liquid-checks.md b/.changeset/single-walk-liquid-checks.md index 70f9ee2fb..9dd3ef475 100644 --- a/.changeset/single-walk-liquid-checks.md +++ b/.changeset/single-walk-liquid-checks.md @@ -4,4 +4,4 @@ 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, 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. +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/index.spec.ts b/packages/theme-check-common/src/index.spec.ts index 873af8874..a81b37679 100644 --- a/packages/theme-check-common/src/index.spec.ts +++ b/packages/theme-check-common/src/index.spec.ts @@ -107,7 +107,32 @@ describe('check on Liquid files', () => { 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')).toHaveLength(12); + 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 () => { @@ -125,16 +150,20 @@ describe('check on Liquid files', () => { 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: 0 }); + 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).toEqual(['file:///snippets/a.liquid', 'file:///snippets/b.liquid']); + expect(calls.sort()).toEqual(['file:///snippets/a.liquid', 'file:///snippets/b.liquid']); expect(errors.map((error) => error.message).sort()).toEqual([ - expect.stringContaining('Cannot read'), - expect.stringContaining('Cannot read'), + expect.stringContaining("(reading 'onCodePathStart')"), + expect.stringContaining("(reading 'onCodePathStart')"), 'Throws failed on file:///snippets/a.liquid', 'Throws failed on file:///snippets/b.liquid', ]); diff --git a/packages/theme-check-common/src/index.ts b/packages/theme-check-common/src/index.ts index bbd25bea7..72853b9b1 100644 --- a/packages/theme-check-common/src/index.ts +++ b/packages/theme-check-common/src/index.ts @@ -221,56 +221,47 @@ async function checkJSONFile(check: JSONCheck, file: JSONSourceCode): Promise[] { - const errors = new Map(); - const checksWithMethod = new Map(); + if (checks.length === 0) return []; - const run = (method: keyof LiquidCheck, ...args: unknown[]): Promise | undefined => { + 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 = checks.filter((check) => { + found = []; + for (const [i, check] of checks.entries()) { try { - return !!check[method]; + if (check[method]) found.push(i); } catch (error) { // e.g. a check whose create() returned nothing - errors.set(check, error); - return false; + if (!errors.has(i)) errors.set(i, error); } - }); + } checksWithMethod.set(method, found); } - - const running = found.filter((check) => !errors.has(check)); - if (running.length === 0) return; - - return Promise.all( - running.map(async (check) => { - try { - await (check as Record Promise>)[method](...args); - } catch (error) { - errors.set(check, error); - } - }), - ); + for (const i of found) calls[i].push([method, args]); }; - const walked = (async () => { - await run('onCodePathStart', file); - if (file.ast instanceof Error) return; - await visitLiquid(file.ast, run); - await run('onCodePathEnd', file); - })(); + collect('onCodePathStart', file); + if (!(file.ast instanceof Error)) { + visitLiquid(file.ast, collect); + collect('onCodePathEnd', file); + } - return checks.map((check) => - walked.then(() => { - if (errors.has(check)) throw errors.get(check); - }), - ); + 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/visitors/liquid.ts b/packages/theme-check-common/src/visitors/liquid.ts index a4b95c29e..1b77d598c 100644 --- a/packages/theme-check-common/src/visitors/liquid.ts +++ b/packages/theme-check-common/src/visitors/liquid.ts @@ -7,26 +7,19 @@ function isLiquidHtmlNode(thing: unknown): thing is LiquidHtmlNode { /** * 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. Waits for `visit` when it returns a - * promise. + * way down, then `${type}:exit` once its children are queued. */ -export async function visitLiquid( +export function visitLiquid( node: LiquidHtmlNode, - visit: ( - method: keyof LiquidCheck, - node: LiquidHtmlNode, - ancestors: LiquidHtmlNode[], - ) => Promise | undefined, -): Promise { + visit: (method: keyof LiquidCheck, node: LiquidHtmlNode, ancestors: LiquidHtmlNode[]) => void, +): void { const stack: { node: LiquidHtmlNode; ancestors: LiquidHtmlNode[] }[] = [{ node, ancestors: [] }]; - let visiting: Promise | undefined; while (stack.length > 0) { const { node, ancestors } = stack.pop()!; const lineage = ancestors.concat(node); - visiting = visit(node.type, node, ancestors); - if (visiting) await visiting; + visit(node.type, node, ancestors); for (const key in node) { if (!node.hasOwnProperty(key) || nonTraversableProperties.has(key)) { @@ -46,7 +39,6 @@ export async function visitLiquid( } } - visiting = visit(`${node.type}:exit`, node, ancestors); - if (visiting) await visiting; + visit(`${node.type}:exit`, node, ancestors); } }