feat(theme-check-common): warn against missing block.shopify_attributes when tag is null#1198
Open
SinhSinhAn wants to merge 1 commit intoShopify:mainfrom
Open
Conversation
Closes Shopify#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.
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
1 participant
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.
Summary
Closes #867.
When a block's
{% schema %}declares"tag": null, Shopify generates no wrapping element. The block's own outermost element must contain{{ block.shopify_attributes }}for the theme editor to recognise it; without that, the block is invisible in the preview and reordering blocks leaves orphaned markup (docs). Today the linter is silent on this very common footgun — exactly the case @benjaminsehl shared from Shopify/horizon#2780.Scope
Phase 1 from @benjaminsehl's comment on the issue: a single-file check. Cases where the block delegates to a snippet, or where
block.shopify_attributesends up rendered multiple times, are explicitly out of scope and can be layered on later.The implementation closely mirrors @charlespwd's sketch on the issue, with the visitor name corrected (
VariableLookup, notLiquidVariable) and the schema check moved intoonCodePathEndso traversal order doesn't matter.What the check does
blocks/*.liquidonly — section files and snippets are skipped viaisBlock(context.file.uri).block.shopify_attributesvia theVariableLookupvisitor. Both dot access and bracket access (block["shopify_attributes"]) are accepted because the parser normalises both into aStringlookup with the samevalue.onCodePathEnd, parses the schema. Only flags when thetagfield is explicitlynull— a missingtagfalls back to Shopify's default<div>wrapper, which receives the attributes itself, so the requirement does not apply.tagfield'snullvalue position so the editor squiggle lands inside the schema.Behaviour matrix
block.shopify_attributesblocks/text.liquidnullblocks/text.liquidnull{{ block.shopify_attributes }})blocks/text.liquidnullblocks/text.liquidblocks/text.liquid"section"sections/foo.liquidnullsnippets/helper.liquidFiles
packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.ts(new, 90 LOC)packages/theme-check-common/src/checks/block-missing-shopify-attributes/index.spec.ts(new, 8 test cases)packages/theme-check-common/src/checks/index.ts(registered inallChecks)packages/theme-check-node/configs/all.ymlandrecommended.ymlregenerated viayarn build.changeset/block-missing-shopify-attributes.md(@shopify/theme-check-common: minor)Test plan
yarn buildcleanprettier --checkclean on all touched filesIf maintainers prefer the offense to land on the entire
{% schema %}opening line (matchingEmptyBlockContent) rather than on thetag: nullvalue specifically, happy to swap that with a one-line change.