From b6a589d5dc73c36b35bbf7a84d8ae8732a20451f Mon Sep 17 00:00:00 2001 From: SinhSinh An Date: Sat, 2 May 2026 17:59:31 -0500 Subject: [PATCH] feat(theme-check-common): add BlockMissingShopifyAttributes check Closes #867. When a block's `{% schema %}` declares `"tag": null`, Shopify generates no wrapping element for the block. The block's own outermost element must include `{{ block.shopify_attributes }}` for the theme editor to recognise it; without that, the block is unrecognised in the preview and merchants reordering blocks leaves orphaned markup behind (documented at https://shopify.dev/docs/storefronts/themes/architecture/blocks/theme-blocks/schema#tag). This adds a new theme-check rule, scoped to phase 1 of the plan @benjaminsehl outlined on the issue: a single-file check that runs on `blocks/*.liquid`, parses the schema, and verifies the file's Liquid output references `block.shopify_attributes` somewhere when the tag is explicitly null. Implementation notes: - Uses `isBlock` to scope the check; section files and snippets are ignored. - Tracks `block.shopify_attributes` references via the `VariableLookup` visitor. Both dot access and bracket access (`block["shopify_attributes"]`) are accepted because the parser normalises both to `String` lookups with the same `value`. - Reports the offense on the JSON `tag` field's value (`null`) so the editor squiggle lands inside the schema, not on the entire file. - Skipped when `tag` is missing or non-null: in those cases Shopify's own wrapper element receives the attributes, so the requirement does not apply. Out of scope (phase 2/3 from the issue): cross-file checks for snippets that supply the markup, and detecting `block.shopify_attributes` rendered multiple times. Either can be layered on top of this check without changing the public API. 8 new spec cases: - Reports on `tag: null` with no usage (the bug) - Accepts dot access on the wrapper - Accepts bracket access - Skips files with no `tag` field - Skips files with a non-null `tag` - Skips section files - Skips snippet files - Verifies the offense range falls on the `tag` value 954 / 954 tests pass overall. --- .../block-missing-shopify-attributes.md | 7 + .../index.spec.ts | 155 ++++++++++++++++++ .../block-missing-shopify-attributes/index.ts | 102 ++++++++++++ .../theme-check-common/src/checks/index.ts | 2 + packages/theme-check-node/configs/all.yml | 3 + .../theme-check-node/configs/recommended.yml | 3 + 6 files changed, 272 insertions(+) create mode 100644 .changeset/block-missing-shopify-attributes.md create mode 100644 packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.spec.ts create mode 100644 packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.ts diff --git a/.changeset/block-missing-shopify-attributes.md b/.changeset/block-missing-shopify-attributes.md new file mode 100644 index 000000000..e66906dfe --- /dev/null +++ b/.changeset/block-missing-shopify-attributes.md @@ -0,0 +1,7 @@ +--- +'@shopify/theme-check-common': minor +--- + +Add `BlockMissingShopifyAttributes` check. Warns when a `blocks/*.liquid` file declares `"tag": null` in its `{% schema %}` but its rendered markup does not include `{{ block.shopify_attributes }}`. Without that, the theme editor cannot recognise the block in the preview, and merchants reordering blocks leaves orphaned markup behind. See [Shopify's `tag` field documentation](https://shopify.dev/docs/storefronts/themes/architecture/blocks/theme-blocks/schema#tag). + +Phase 1: only the current file is checked. Cases where the markup is delegated to a rendered snippet, or where `block.shopify_attributes` is rendered multiple times, are out of scope. diff --git a/packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.spec.ts b/packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.spec.ts new file mode 100644 index 000000000..273b947ff --- /dev/null +++ b/packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.spec.ts @@ -0,0 +1,155 @@ +import { expect, describe, it } from 'vitest'; +import { BlockMissingShopifyAttributes } from './index'; +import { check, MockTheme } from '../../test'; + +describe('Module: BlockMissingShopifyAttributes', () => { + it('reports an offense when the block schema sets `tag: null` and the markup omits `block.shopify_attributes`', async () => { + const theme: MockTheme = { + 'blocks/text.liquid': ` +

{{ block.settings.heading }}

+ {% schema %} + { + "name": "Text", + "tag": null, + "settings": [] + } + {% endschema %} + `, + }; + + const offenses = await check(theme, [BlockMissingShopifyAttributes]); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).to.match(/must render `\{\{ block\.shopify_attributes \}\}`/); + expect(offenses[0].check).to.eql('BlockMissingShopifyAttributes'); + }); + + it('does not report when `block.shopify_attributes` is rendered on the wrapper', async () => { + const theme: MockTheme = { + 'blocks/text.liquid': ` +
+

{{ block.settings.heading }}

+
+ {% schema %} + { + "name": "Text", + "tag": null, + "settings": [] + } + {% endschema %} + `, + }; + + const offenses = await check(theme, [BlockMissingShopifyAttributes]); + expect(offenses).to.be.empty; + }); + + it('does not report when `tag` is omitted (Shopify wraps the block automatically)', async () => { + const theme: MockTheme = { + 'blocks/text.liquid': ` +

{{ block.settings.heading }}

+ {% schema %} + { + "name": "Text", + "settings": [] + } + {% endschema %} + `, + }; + + const offenses = await check(theme, [BlockMissingShopifyAttributes]); + expect(offenses).to.be.empty; + }); + + it('does not report when `tag` is a non-null string (Shopify wraps the block in that element)', async () => { + const theme: MockTheme = { + 'blocks/text.liquid': ` +

{{ block.settings.heading }}

+ {% schema %} + { + "name": "Text", + "tag": "section", + "settings": [] + } + {% endschema %} + `, + }; + + const offenses = await check(theme, [BlockMissingShopifyAttributes]); + expect(offenses).to.be.empty; + }); + + it('accepts bracket access (`block["shopify_attributes"]`) as valid', async () => { + const theme: MockTheme = { + 'blocks/text.liquid': ` +
content
+ {% schema %} + { + "name": "Text", + "tag": null + } + {% endschema %} + `, + }; + + const offenses = await check(theme, [BlockMissingShopifyAttributes]); + expect(offenses).to.be.empty; + }); + + it('does not run on section files', async () => { + // Even if a section has `tag: null` declared (which is permitted on + // sections too), the requirement to render block.shopify_attributes is + // a block-specific concern, so this check should ignore sections. + const theme: MockTheme = { + 'sections/missing.liquid': ` +
+ {% schema %} + { + "name": "Section", + "tag": null, + "settings": [] + } + {% endschema %} + `, + }; + + const offenses = await check(theme, [BlockMissingShopifyAttributes]); + expect(offenses).to.be.empty; + }); + + it('does not run on snippet files', async () => { + // Snippets do not have schemas, so the check has nothing to act on. + // This is a guard against future changes that might trip on the path. + const theme: MockTheme = { + 'snippets/helper.liquid': ` +
{{ thing }}
+ `, + }; + + const offenses = await check(theme, [BlockMissingShopifyAttributes]); + expect(offenses).to.be.empty; + }); + + it('reports the offense at the schema `tag` field position', async () => { + const source = ` +

{{ block.settings.heading }}

+ {% schema %} + { + "name": "Text", + "tag": null + } + {% endschema %} + `; + const theme: MockTheme = { + 'blocks/text.liquid': source, + }; + + const offenses = await check(theme, [BlockMissingShopifyAttributes]); + expect(offenses).toHaveLength(1); + + // The reported range should be inside the schema body, anchored on the + // `tag` field's value (a sanity check that we're using the JSON AST + // node positions correctly). + const offendingRange = source.slice(offenses[0].start.index, offenses[0].end.index); + expect(offendingRange).to.match(/null/); + }); +}); diff --git a/packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.ts b/packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.ts new file mode 100644 index 000000000..ae027aed8 --- /dev/null +++ b/packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.ts @@ -0,0 +1,102 @@ +import { LiquidVariableLookup } from '@shopify/liquid-html-parser'; +import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; +import { isBlock } from '../../to-schema'; +import { getSchema } from '../../to-schema'; +import { nodeAtPath } from '../../json'; +import { reportWarning } from '../../utils'; + +const SHOPIFY_ATTRIBUTES_KEY = 'shopify_attributes'; +const BLOCK_OBJECT = 'block'; + +/** + * When a block schema declares `"tag": null`, the block has no wrapping element + * generated by Shopify. To stay registered with the theme editor, the markup + * the block renders MUST contain `{{ block.shopify_attributes }}` somewhere on + * its outermost element. Forgetting this leaves the block unrecognised in the + * preview and produces orphaned markup when merchants reorder blocks. + * + * @see https://shopify.dev/docs/storefronts/themes/architecture/blocks/theme-blocks/schema#tag + * + * Phase 1 (this implementation, per #867): only the current file is checked. + * Cases where the block's markup is delegated to a rendered snippet, or where + * `block.shopify_attributes` ends up rendered multiple times, are out of scope. + */ +export const BlockMissingShopifyAttributes: LiquidCheckDefinition = { + meta: { + code: 'BlockMissingShopifyAttributes', + name: 'Block missing block.shopify_attributes', + docs: { + description: + 'Warns when a block file with `"tag": null` in its schema does not render `{{ block.shopify_attributes }}` on its outermost element.', + recommended: true, + url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/block-missing-shopify-attributes', + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.WARNING, + schema: {}, + targets: [], + }, + + create(context) { + // Skip non-block files entirely. Section files have their own wrapping + // semantics and do not require `block.shopify_attributes`. + if (!isBlock(context.file.uri)) return {}; + + let hasShopifyAttributes = false; + let schemaOffset: number | null = null; + + return { + async LiquidRawTag(node) { + if (node.name !== 'schema' || node.body.kind !== 'json') return; + // Captured here so we can translate JSON-AST positions to file + // positions inside `onCodePathEnd`. + schemaOffset = node.blockStartPosition.end; + }, + + async VariableLookup(node: LiquidVariableLookup) { + if (hasShopifyAttributes) return; + if (node.name !== BLOCK_OBJECT) return; + + // `block.shopify_attributes` shows up as a VariableLookup with + // `name === 'block'` and a single string lookup of + // `'shopify_attributes'`. Bracket access (`block['shopify_attributes']`) + // is also accepted because liquid-html-parser normalises both forms + // to a `String` lookup with the same `value`. + const matches = node.lookups.some( + (lookup) => lookup.type === 'String' && lookup.value === SHOPIFY_ATTRIBUTES_KEY, + ); + if (matches) { + hasShopifyAttributes = true; + } + }, + + async onCodePathEnd() { + if (hasShopifyAttributes) return; + if (schemaOffset === null) return; + + const schema = await getSchema(context); + if (!schema) return; + + const { validSchema, ast } = schema; + if (!validSchema || validSchema instanceof Error) return; + if (!ast || ast instanceof Error) return; + + // The schema field is `tag` and we only flag the explicit `null` case. + // Missing `tag` (i.e. the schema-default behaviour where Shopify wraps + // the block in a `
`) does NOT require `block.shopify_attributes`, + // because Shopify's wrapper element receives the attributes itself. + if (!('tag' in validSchema) || validSchema.tag !== null) return; + + const tagNode = nodeAtPath(ast, ['tag']); + if (!tagNode) return; + + reportWarning( + `Blocks with \`"tag": null\` must render \`{{ block.shopify_attributes }}\` on their outermost element so the theme editor can recognise the block.`, + schemaOffset, + tagNode, + context, + ); + }, + }; + }, +}; diff --git a/packages/theme-check-common/src/checks/index.ts b/packages/theme-check-common/src/checks/index.ts index be0370c41..7d9bec01d 100644 --- a/packages/theme-check-common/src/checks/index.ts +++ b/packages/theme-check-common/src/checks/index.ts @@ -7,6 +7,7 @@ import { AssetSizeAppBlockJavaScript } from './asset-size-app-block-javascript'; import { AssetSizeCSS } from './asset-size-css'; import { AssetSizeJavaScript } from './asset-size-javascript'; import { BlockIdUsage } from './block-id-usage'; +import { BlockMissingShopifyAttributes } from './block-missing-shopify-attributes'; import { CdnPreconnect } from './cdn-preconnect'; import { ContentForHeaderModification } from './content-for-header-modification'; import { DeprecateBgsizes } from './deprecate-bgsizes'; @@ -77,6 +78,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ AssetSizeCSS, AssetSizeJavaScript, BlockIdUsage, + BlockMissingShopifyAttributes, CdnPreconnect, ContentForHeaderModification, DeprecateBgsizes, diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index 426115894..82a3f1f7d 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -31,6 +31,9 @@ AssetSizeJavaScript: BlockIdUsage: enabled: true severity: 1 +BlockMissingShopifyAttributes: + enabled: true + severity: 1 CdnPreconnect: enabled: true severity: 0 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index adaa03fdf..dd9c1ddff 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -9,6 +9,9 @@ AssetPreload: BlockIdUsage: enabled: true severity: 1 +BlockMissingShopifyAttributes: + enabled: true + severity: 1 CdnPreconnect: enabled: true severity: 0