feat(theme-check-common): add LiquidSchema check visitor method#1194
Open
SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
Open
feat(theme-check-common): add LiquidSchema check visitor method#1194SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
Conversation
Introduces a `LiquidSchema(node)` method on `Check<LiquidHtml>` that
replaces the repeated preamble found in every schema-validating check:
async LiquidRawTag(node) {
if (node.name !== 'schema' || node.body.kind !== 'json') return;
const offset = node.blockStartPosition.end;
const schema = await getSchema(context);
const { validSchema, ast } = schema ?? {};
if (!validSchema || validSchema instanceof Error) return;
if (!ast || ast instanceof Error) return;
// ... check logic ...
}
becomes:
async LiquidSchema({ validSchema, ast, offset, schema }) {
// ... check logic ...
}
Implementation:
- New `LiquidSchemaNode` type exposing `{ node, schema, validSchema,
ast, offset }`, with `validSchema` and `ast` narrowed to their
non-Error variants.
- New `wrapLiquidSchema` helper that transforms a check's
`LiquidSchema` method into a composed `LiquidRawTag` visitor. This
keeps `visitLiquid` oblivious to schemas and avoids threading
`context` through the visitor.
- `createCheck` runs `wrapLiquidSchema` once per check instance, so
every LiquidHtml check gets the new method for free.
- When a check declares both `LiquidRawTag` and `LiquidSchema`, the
existing `LiquidRawTag` runs first for every raw tag, then the
schema preamble runs and `LiquidSchema` fires only for valid
schemas.
- Checks not in section or theme-block files never fire
`LiquidSchema` (matches the existing `getSchema` behaviour).
Migrated `ValidSettingsKey` as a demonstration. Its 17 tests pass
unchanged, proving the API is a drop-in replacement.
Closes Shopify#817
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR?
Implements the
LiquidSchemacheck node type proposed in #817. It abstracts the repeated preamble found in every schema-validating Liquid check.Before
Every schema check currently opens with the same ten-line preamble:
At least nine checks in the codebase repeat this boilerplate verbatim.
After
The
LiquidSchemacallback fires once per{% schema %}tag in a section or theme-block file, after the schema has been parsed and validated. The payload exposes:node= the originalLiquidRawTagnodeschema= fullSectionSchema | ThemeBlockSchema, forisSectionSchema/isBlockSchemanarrowingvalidSchema= narrowed toSection.Schema | ThemeBlock.Schema(neverError)ast= JSON AST (neverError)offset= character offset of the JSON inside the Liquid sourceImplementation
LiquidSchemaNodetype and extendedCheck<LiquidHtml>with an optionalLiquidSchemamethod via aCheckExtraMethodsparameter on theCheck<T>type.wrapLiquidSchemahelper inpackages/theme-check-common/src/wrap-liquid-schema.ts. It transforms a check'sLiquidSchemamethod into a standardLiquidRawTagvisitor by baking in the preamble. When a check declares bothLiquidRawTagandLiquidSchema, the existingLiquidRawTagruns first for every raw tag, then the schema preamble runs andLiquidSchemafires only for valid schemas.createCheckinindex.tsrunswrapLiquidSchemaonce per check instance, so every LiquidHtml check gets the new method for free. No changes tovisitLiquid, no new node type in the parser, no context threading through the visitor.Why a wrapper rather than a visitor change
visitLiquidonly has access to(node, check). Resolving a schema requirescontext.getBlockSchema/context.getSectionSchema, which are per-check dependencies captured in thecreate(context)closure. Threadingcontextthrough the visitor would be a larger API change and would couple the visitor to a check-level concept. Wrapping atcreateChecktime keeps the visitor clean and keeps the new method a first-class part of the check contract.LiquidSchemadoes not fire for{% schema %}tags outsidesections/andblocks/(matches existinggetSchemabehavior)body.kind !== 'json')Error(invalid JSON or validation failure)LiquidRawTag(stylesheet, javascript, comment, raw, etc.)Tests
Added 5 dedicated unit tests in
wrap-liquid-schema.spec.ts:{% schema %}in a sectionLiquidRawTagmethodMigrated
ValidSettingsKeyas a demonstration. Its full 17-test suite passes unchanged, proving the API is a drop-in replacement.Full regression run: 861 theme-check tests pass (856 existing + 5 new), 162 parser tests pass, 141 prettier-plugin tests pass. Zero regressions across 1,164 tests.
Follow-up opportunities
Eight additional checks currently follow the verbatim preamble and could be migrated in follow-up PRs once this API is accepted:
valid-visible-ifvalid-block-targetvalid-local-blocksvalid-schema-namevalid-schema-translationsschema-presets-block-orderschema-presets-static-blocks(partial migration — it uses bothLiquidRawTagfor schema andLiquidTagfor content_for)deprecated-fonts-on-sections-and-blocksI kept migrations out of this PR to minimize scope. Happy to migrate any/all in this PR or separate follow-ups based on reviewer preference.
Closes #817