Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/block-missing-shopify-attributes.md
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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': `
<h2>{{ block.settings.heading }}</h2>
{% 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': `
<div {{ block.shopify_attributes }}>
<h2>{{ block.settings.heading }}</h2>
</div>
{% 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': `
<h2>{{ block.settings.heading }}</h2>
{% 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': `
<h2>{{ block.settings.heading }}</h2>
{% 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': `
<div {{ block["shopify_attributes"] }}>content</div>
{% 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': `
<div></div>
{% 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': `
<div>{{ thing }}</div>
`,
};

const offenses = await check(theme, [BlockMissingShopifyAttributes]);
expect(offenses).to.be.empty;
});

it('reports the offense at the schema `tag` field position', async () => {
const source = `
<h2>{{ block.settings.heading }}</h2>
{% 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/);
});
});
Original file line number Diff line number Diff line change
@@ -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 `<div>`) 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,
);
},
};
},
};
2 changes: 2 additions & 0 deletions packages/theme-check-common/src/checks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -77,6 +78,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [
AssetSizeCSS,
AssetSizeJavaScript,
BlockIdUsage,
BlockMissingShopifyAttributes,
CdnPreconnect,
ContentForHeaderModification,
DeprecateBgsizes,
Expand Down
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ AssetSizeJavaScript:
BlockIdUsage:
enabled: true
severity: 1
BlockMissingShopifyAttributes:
enabled: true
severity: 1
CdnPreconnect:
enabled: true
severity: 0
Expand Down
3 changes: 3 additions & 0 deletions packages/theme-check-node/configs/recommended.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ AssetPreload:
BlockIdUsage:
enabled: true
severity: 1
BlockMissingShopifyAttributes:
enabled: true
severity: 1
CdnPreconnect:
enabled: true
severity: 0
Expand Down
Loading