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