From 709e359e4e2d6459f9bb39eef36522cb60b05300 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 30 Sep 2026 11:12:54 -0400 Subject: [PATCH 1/2] Add implicit block content tooling --- .../block-content-implicit-argument-checks.md | 12 ++ ...ntent-implicit-argument-language-server.md | 7 ++ .../checks/block-content-usage/index.spec.ts | 63 ++++++++++ .../src/checks/block-content-usage/index.ts | 53 ++++++++ .../theme-check-common/src/checks/index.ts | 4 + .../src/checks/undefined-object/index.spec.ts | 17 +++ .../src/checks/undefined-object/index.ts | 8 +- .../valid-block-argument-types/index.spec.ts | 35 ++---- .../valid-block-argument-types/index.ts | 51 ++------ .../index.spec.ts | 119 ++++++++++++++++++ .../valid-block-content-setting-type/index.ts | 67 ++++++++++ packages/theme-check-node/configs/all.yml | 6 + .../theme-check-node/configs/recommended.yml | 6 + .../src/TypeSystem.spec.ts | 107 ++++++++++++++++ .../src/TypeSystem.ts | 58 +++++---- .../ObjectCompletionProvider.spec.ts | 83 ++++++++++-- .../LiquidObjectHoverProvider.spec.ts | 78 ++++++++++-- 17 files changed, 668 insertions(+), 106 deletions(-) create mode 100644 .changeset/block-content-implicit-argument-checks.md create mode 100644 .changeset/block-content-implicit-argument-language-server.md create mode 100644 packages/theme-check-common/src/checks/block-content-usage/index.spec.ts create mode 100644 packages/theme-check-common/src/checks/block-content-usage/index.ts create mode 100644 packages/theme-check-common/src/checks/valid-block-content-setting-type/index.spec.ts create mode 100644 packages/theme-check-common/src/checks/valid-block-content-setting-type/index.ts diff --git a/.changeset/block-content-implicit-argument-checks.md b/.changeset/block-content-implicit-argument-checks.md new file mode 100644 index 000000000..605c6db1e --- /dev/null +++ b/.changeset/block-content-implicit-argument-checks.md @@ -0,0 +1,12 @@ +--- +'@shopify/theme-check-common': minor +'@shopify/theme-check-node': minor +--- + +Guide theme blocks to the implicit `content` parameter. + +Add the recommended `BlockContentUsage` warning for `block.content` in `blocks/*.liquid`. Its message is "Use the implicit 'content' parameter directly instead of 'block.content'." + +Add the recommended `ValidBlockContentSettingType` error for a theme block schema setting named `content` whose Liquid type is not `string`. This error replaces the `ValidBlockArgumentTypes` warning for the same schema declaration. + +`UndefinedObject` accepts bare `content` in `blocks/*.liquid`. diff --git a/.changeset/block-content-implicit-argument-language-server.md b/.changeset/block-content-implicit-argument-language-server.md new file mode 100644 index 000000000..efe3131a5 --- /dev/null +++ b/.changeset/block-content-implicit-argument-language-server.md @@ -0,0 +1,7 @@ +--- +'@shopify/theme-language-server-common': patch +--- + +Complete and hover the implicit `content` parameter inside `blocks/*.liquid`. + +Bare `content` is a `string` variable in every theme block file, and `block.content` is a `string` property there. A later assign or capture changes the variable's type from that point. A schema setting or LiquidDoc parameter named `content` keeps the `string` type. diff --git a/packages/theme-check-common/src/checks/block-content-usage/index.spec.ts b/packages/theme-check-common/src/checks/block-content-usage/index.spec.ts new file mode 100644 index 000000000..305570728 --- /dev/null +++ b/packages/theme-check-common/src/checks/block-content-usage/index.spec.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from 'vitest'; +import { recommended } from '../index'; +import { highlightedOffenses, runLiquidCheck } from '../../test'; +import { Severity } from '../../types'; +import { BlockContentUsage } from './index'; + +const MESSAGE = "Use the implicit 'content' parameter directly instead of 'block.content'."; + +describe('BlockContentUsage', () => { + it('is a recommended warning', () => { + expect(recommended).toContain(BlockContentUsage); + expect(BlockContentUsage.meta.severity).toBe(Severity.WARNING); + }); + + it.each([ + ['{{ block.content }}', 'block.content'], + ["{{ block['content'] | upcase }}", "block['content']"], + ['{{ block.content.size }}', 'block.content.size'], + ['{% if block.content != blank %}{{ content }}{% endif %}', 'block.content'], + ["{% render 'card', body: block.content %}", 'block.content'], + ['{% liquid\n echo block.content\n%}', 'block.content'], + ])('reports the block.content lookup in %j', async (source, highlight) => { + const offenses = await runLiquidCheck(BlockContentUsage, source, 'blocks/card.liquid'); + + expect(offenses).toMatchObject([{ message: MESSAGE, severity: Severity.WARNING }]); + expect(highlightedOffenses({ 'blocks/card.liquid': source }, offenses)).toEqual([highlight]); + }); + + it('accepts bare content and other block properties', async () => { + const source = [ + '{{ content }}', + '{{ block.id }}', + '{{ block.settings.content }}', + '{{ block.shopify_attributes }}', + '{{ product.content }}', + ].join('\n'); + + const offenses = await runLiquidCheck(BlockContentUsage, source, 'blocks/card.liquid'); + + expect(offenses).toEqual([]); + }); + + it.each([ + 'sections/main.liquid', + 'snippets/card.liquid', + 'templates/index.liquid', + 'layout/theme.liquid', + ])('does not report the ordinary block object in %s', async (fileName) => { + const source = '{% for block in section.blocks %}{{ block.content }}{% endfor %}'; + + const offenses = await runLiquidCheck(BlockContentUsage, source, fileName); + + expect(offenses).toEqual([]); + }); + + it('does not report block.content inside a tag that does not render Liquid', async () => { + const source = '{% javascript %}console.log("{{ block.content }}");{% endjavascript %}'; + + const offenses = await runLiquidCheck(BlockContentUsage, source, 'blocks/card.liquid'); + + expect(offenses).toEqual([]); + }); +}); diff --git a/packages/theme-check-common/src/checks/block-content-usage/index.ts b/packages/theme-check-common/src/checks/block-content-usage/index.ts new file mode 100644 index 000000000..3e2133679 --- /dev/null +++ b/packages/theme-check-common/src/checks/block-content-usage/index.ts @@ -0,0 +1,53 @@ +import { LiquidVariableLookup, NodeTypes } from '@shopify/liquid-html-parser'; +import { BLOCK_CONTENT_PARAMETER } from '../../block-parameters'; +import { isBlock } from '../../to-schema'; +import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; +import { isWithinRawTagThatDoesNotParseItsContents } from '../utils'; + +const MESSAGE = "Use the implicit 'content' parameter directly instead of 'block.content'."; + +export const BlockContentUsage: LiquidCheckDefinition = { + meta: { + code: 'BlockContentUsage', + name: 'Use `content` instead of `block.content`', + docs: { + description: + "Reports 'block.content' in theme block files, where the built-in 'content' parameter is available as the 'content' variable.", + recommended: true, + url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/block-content-usage', + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.WARNING, + schema: {}, + targets: [], + }, + + create(context) { + if (!isBlock(context.file.uri)) return {}; + + return { + // BAD: {{ block.content }} + // BAD: {{ block['content'] | upcase }} + // GOOD: {{ content }} + async VariableLookup(node, ancestors) { + if (isWithinRawTagThatDoesNotParseItsContents(ancestors)) return; + if (!isBlockContentLookup(node)) return; + + context.report({ + message: MESSAGE, + startIndex: node.position.start, + endIndex: node.position.end, + }); + }, + }; + }, +}; + +function isBlockContentLookup(node: LiquidVariableLookup): boolean { + const [firstLookup] = node.lookups; + return ( + node.name === 'block' && + firstLookup?.type === NodeTypes.String && + firstLookup.value === BLOCK_CONTENT_PARAMETER + ); +} diff --git a/packages/theme-check-common/src/checks/index.ts b/packages/theme-check-common/src/checks/index.ts index e54ae76ed..a29d25543 100644 --- a/packages/theme-check-common/src/checks/index.ts +++ b/packages/theme-check-common/src/checks/index.ts @@ -79,6 +79,7 @@ import { StylesheetOncePerFile, StylesheetTagInWrongFile, } from './raw-tags'; +import { BlockContentUsage } from './block-content-usage'; import { DuplicateBlockArguments } from './duplicate-block-arguments'; import { ExcessiveSettingsCount } from './excessive-settings-count'; import { LiquidComplexity } from './liquid-complexity'; @@ -88,6 +89,7 @@ import { MaxFileSize, MaxFileSizeJSON } from './max-file-size'; import { MissingBlockArguments } from './missing-block-arguments'; import { UnrecognizedBlockArguments } from './unrecognized-block-arguments'; import { ValidBlockArgumentTypes } from './valid-block-argument-types'; +import { ValidBlockContentSettingType } from './valid-block-content-setting-type'; import { ValidBlockTagPlacement } from './valid-block-tag-placement'; export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ @@ -169,6 +171,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ SchemaSectionOrBlockOnly, StylesheetOncePerFile, StylesheetTagInWrongFile, + BlockContentUsage, DuplicateBlockArguments, ExcessiveSettingsCount, LiquidComplexity, @@ -179,6 +182,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ MissingBlockArguments, UnrecognizedBlockArguments, ValidBlockArgumentTypes, + ValidBlockContentSettingType, ValidBlockTagPlacement, ]; diff --git a/packages/theme-check-common/src/checks/undefined-object/index.spec.ts b/packages/theme-check-common/src/checks/undefined-object/index.spec.ts index 455909cfe..dbec713f3 100644 --- a/packages/theme-check-common/src/checks/undefined-object/index.spec.ts +++ b/packages/theme-check-common/src/checks/undefined-object/index.spec.ts @@ -449,6 +449,23 @@ describe('Module: UndefinedObject', () => { expect(offenses[0].message).toBe("Unknown object 'undefined_variable' used."); }); + it('does not report the built-in content variable in a block file', async () => { + const sourceCode = ` + {{ content }} + {% if content != blank %}{{ content | upcase }}{% endif %} + `; + + const offenses = await runLiquidCheck(UndefinedObject, sourceCode, 'blocks/card.liquid'); + + expect(offenses).toEqual([]); + }); + + it('reports content outside block files', async () => { + const offenses = await runLiquidCheck(UndefinedObject, '{{ content }}', 'sections/main.liquid'); + + expect(offenses).toMatchObject([{ message: "Unknown object 'content' used." }]); + }); + it('should not report an offense when a self defined variable is defined with a @param tag', async () => { const sourceCode = ` {% doc %} diff --git a/packages/theme-check-common/src/checks/undefined-object/index.ts b/packages/theme-check-common/src/checks/undefined-object/index.ts index 55d2b6a4b..e894a8180 100644 --- a/packages/theme-check-common/src/checks/undefined-object/index.ts +++ b/packages/theme-check-common/src/checks/undefined-object/index.ts @@ -13,6 +13,7 @@ import { NodeTypes, Position, } from '@shopify/liquid-html-parser'; +import { BLOCK_CONTENT_PARAMETER } from '../../block-parameters'; import { LiquidCheckDefinition, Mode, Severity, SourceCodeType, ThemeDocset } from '../../types'; import { isError, last } from '../../utils'; import { hasLiquidDoc } from '../../liquid-doc/liquidDoc'; @@ -55,7 +56,7 @@ export const UndefinedObject: LiquidCheckDefinition = { const themeDocset = context.themeDocset; const scopedVariables: Map = new Map(); - const fileScopedVariables: Set = new Set(); + const fileScopedVariables: Set = new Set(builtInVariables(relativePath)); const variables: LiquidVariableLookup[] = []; function indexVariableScope(variableName: string | null, scope: Scope) { @@ -188,6 +189,11 @@ async function globalObjects(themeDocset: ThemeDocset, relativePath: string, mod return globalObjects; } +/** Theme blocks always receive the built-in `content` parameter as a variable. */ +function builtInVariables(relativePath: string): string[] { + return relativePath.startsWith('blocks/') ? [BLOCK_CONTENT_PARAMETER] : []; +} + const BLOCK_CONTEXTUAL_OBJECTS = ['app', 'section', 'recommendations', 'block']; function getContextualObjects(relativePath: string, mode: Mode = 'theme'): string[] { diff --git a/packages/theme-check-common/src/checks/valid-block-argument-types/index.spec.ts b/packages/theme-check-common/src/checks/valid-block-argument-types/index.spec.ts index d3eba86a6..b28cdb258 100644 --- a/packages/theme-check-common/src/checks/valid-block-argument-types/index.spec.ts +++ b/packages/theme-check-common/src/checks/valid-block-argument-types/index.spec.ts @@ -264,28 +264,19 @@ describe('ValidBlockArgumentTypes', () => { ]); }); - it('reports a schema content setting that is not string-compatible', async () => { - const source = blockSource([{ id: 'content', type: 'number' }]); - const offenses = await definitions(source); - - expect(offenses).toMatchObject([ - { - message: - "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"']); - }); - - it('uses the built-in string type for a LiquidDoc echo of schema content', async () => { - const offenses = await definitions( - blockSource([{ id: 'content', type: 'number' }], ['@param {string} [content] - Body']), - ); - - expect(offenses.map((offense) => offense.message)).toEqual([ - "Schema setting 'content' has Liquid type 'number', but the built-in 'content' parameter has type 'string'.", - ]); - }); + it.each([ + ['without LiquidDoc', []], + ['with a LiquidDoc string echo', ['@param {string} [content] - Body']], + ])( + 'leaves a non-string schema content setting %s to ValidBlockContentSettingType', + async (_name, params) => { + const offenses = await definitions( + blockSource([{ id: 'content', type: 'number' }], params), + ); + + expect(offenses).toEqual([]); + }, + ); it('accepts a string-compatible schema content setting', async () => { const offenses = await definitions(blockSource([{ id: 'content', type: 'text' }])); diff --git a/packages/theme-check-common/src/checks/valid-block-argument-types/index.ts b/packages/theme-check-common/src/checks/valid-block-argument-types/index.ts index b9e6d95f6..83e11af8c 100644 --- a/packages/theme-check-common/src/checks/valid-block-argument-types/index.ts +++ b/packages/theme-check-common/src/checks/valid-block-argument-types/index.ts @@ -4,19 +4,10 @@ import { type BlockParameters, liquidDocType, } from '../../block-parameters'; -import { nodeAtPath } from '../../json'; import { BasicParamTypes, inferArgumentType, isTypeCompatible } from '../../liquid-doc/utils'; import * as path from '../../path'; -import { schemaSettingLiquidType } from '../../schema-settings'; import { isBlock } from '../../to-schema'; -import { - Severity, - SourceCodeType, - type Context, - type LiquidCheckDefinition, - type ThemeBlockSchema, -} from '../../types'; -import { reportWarning } from '../../utils'; +import { Severity, SourceCodeType, type Context, type LiquidCheckDefinition } from '../../types'; import { blockTagSyntaxError } from '../liquid-syntax-error/block'; import { NodeTypes, @@ -81,13 +72,6 @@ export const ValidBlockArgumentTypes: LiquidCheckDefinition = { reportLiquidDocTypeMismatch(context, node, parameter); }, - - async LiquidRawTag(node) { - if (node.name !== 'schema' || node.body.kind !== 'json') return; - if (!currentBlockName || !context.getBlockSchema) return; - - reportContentSettingTypeMismatch(context, await context.getBlockSchema(currentBlockName)); - }, }; }, }; @@ -129,7 +113,7 @@ function reportLiquidDocTypeMismatch( if (!node.paramType || !declaredType || !authoritativeType) return; if (declaredType === BasicParamTypes.Object || declaredType === authoritativeType) return; - const source = parameter.schemaSetting ? 'schema setting' : 'built-in parameter'; + const source = declarationSource(parameter); context.report({ message: `The ${source} '${parameter.name}' has Liquid type '${authoritativeType}', ` + @@ -139,30 +123,17 @@ function reportLiquidDocTypeMismatch( }); } +/** + * The built-in `content` parameter decides the type of `content`, even when a + * schema setting also declares it. + */ +function declarationSource(parameter: BlockParameter): string { + if (parameter.name === BLOCK_CONTENT_PARAMETER) return 'built-in parameter'; + return 'schema setting'; +} + /** Returns the merged type that a schema or built-in declaration must fit. */ function declarationType(parameter: BlockParameter): string | undefined { if (!parameter.schemaSetting && parameter.name !== BLOCK_CONTENT_PARAMETER) return undefined; return parameter.type; } - -function reportContentSettingTypeMismatch( - context: Context, - schema: ThemeBlockSchema | undefined, -): void { - if (!schema || schema.validSchema instanceof Error || schema.ast instanceof Error) return; - - const settings = schema.validSchema.settings ?? []; - const index = settings.findIndex((setting) => setting.id === BLOCK_CONTENT_PARAMETER); - if (index < 0) return; - - const settingType = schemaSettingLiquidType(settings[index].type); - const typeNode = nodeAtPath(schema.ast, ['settings', index, 'type']); - if (!settingType || settingType === 'string' || !typeNode) return; - - reportWarning( - `Schema setting 'content' has Liquid type '${settingType}', but the built-in 'content' parameter has type 'string'.`, - schema.offset, - typeNode, - context, - ); -} 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 new file mode 100644 index 000000000..abc50d33c --- /dev/null +++ b/packages/theme-check-common/src/checks/valid-block-content-setting-type/index.spec.ts @@ -0,0 +1,119 @@ +import { describe, expect, it } from 'vitest'; +import { recommended } from '../index'; +import { ValidBlockArgumentTypes } from '../valid-block-argument-types'; +import { check, highlightedOffenses } from '../../test'; +import { blockSource } from '../../test/block-fixtures'; +import { Severity } from '../../types'; +import { ValidBlockContentSettingType } from './index'; + +describe('ValidBlockContentSettingType', () => { + it('is a recommended error', () => { + expect(recommended).toContain(ValidBlockContentSettingType); + expect(ValidBlockContentSettingType.meta.severity).toBe(Severity.ERROR); + }); + + it.each([ + ['number', 'number'], + ['checkbox', 'boolean'], + ['product', 'product'], + ['product_list', 'product[]'], + ])('reports a %s schema setting named content on its type', async (settingType, liquidType) => { + const source = blockSource([{ id: 'content', type: settingType }]); + + const offenses = await check({ 'blocks/card.liquid': source }, [ValidBlockContentSettingType]); + + expect(offenses).toMatchObject([ + { + message: `Schema setting 'content' has Liquid type '${liquidType}', but the built-in 'content' parameter has type 'string'.`, + severity: Severity.ERROR, + }, + ]); + expect(highlightedOffenses({ 'blocks/card.liquid': source }, offenses)).toEqual([ + `"${settingType}"`, + ]); + }); + + it.each(['text', 'textarea', 'richtext', 'inline_richtext', 'html'])( + 'accepts a string-compatible %s schema setting named content', + async (settingType) => { + const offenses = await check( + { 'blocks/card.liquid': blockSource([{ id: 'content', type: settingType }]) }, + [ValidBlockContentSettingType], + ); + + expect(offenses).toEqual([]); + }, + ); + + it('does not speculate about an unmapped schema setting type named content', async () => { + const offenses = await check( + { 'blocks/card.liquid': blockSource([{ id: 'content', type: 'metaobject' }]) }, + [ValidBlockContentSettingType], + ); + + expect(offenses).toEqual([]); + }); + + it('accepts non-string schema settings with other IDs', async () => { + const offenses = await check( + { 'blocks/card.liquid': blockSource([{ id: 'count', type: 'number' }]) }, + [ValidBlockContentSettingType], + ); + + expect(offenses).toEqual([]); + }); + + it('does not report a non-string content setting outside theme block files', async () => { + const offenses = await check( + { 'sections/main.liquid': blockSource([{ id: 'content', type: 'number' }]) }, + [ValidBlockContentSettingType], + ); + + expect(offenses).toEqual([]); + }); + + it('reports the schema mismatch once alongside ValidBlockArgumentTypes', async () => { + const source = blockSource( + [{ id: 'content', type: 'number' }], + ['@param {string} [content] - Body'], + ); + + const offenses = await check({ 'blocks/card.liquid': source }, [ + ValidBlockArgumentTypes, + ValidBlockContentSettingType, + ]); + + expect(offenses).toMatchObject([ + { check: 'ValidBlockContentSettingType', severity: Severity.ERROR }, + ]); + }); + + it('names the built-in parameter as the string authority when schema and LiquidDoc agree on another type', async () => { + const source = blockSource( + [{ id: 'content', type: 'number' }], + ['@param {number} [content] - Body'], + ); + + const offenses = await check({ 'blocks/card.liquid': source }, [ + ValidBlockArgumentTypes, + ValidBlockContentSettingType, + ]); + + expect(offenses).toMatchObject([ + { + check: 'ValidBlockContentSettingType', + message: + "Schema setting 'content' has Liquid type 'number', but the built-in 'content' parameter has type 'string'.", + }, + { + check: 'ValidBlockArgumentTypes', + message: + "The built-in parameter 'content' has Liquid type 'string', but LiquidDoc declares 'number'. The built-in parameter type is authoritative.", + }, + ]); + expect(highlightedOffenses({ 'blocks/card.liquid': source }, offenses)).toEqual([ + '"number"', + '{number}', + ]); + }); +}); diff --git a/packages/theme-check-common/src/checks/valid-block-content-setting-type/index.ts b/packages/theme-check-common/src/checks/valid-block-content-setting-type/index.ts new file mode 100644 index 000000000..0596b6f43 --- /dev/null +++ b/packages/theme-check-common/src/checks/valid-block-content-setting-type/index.ts @@ -0,0 +1,67 @@ +import { BLOCK_CONTENT_PARAMETER } from '../../block-parameters'; +import { nodeAtPath } from '../../json'; +import * as path from '../../path'; +import { schemaSettingLiquidType } from '../../schema-settings'; +import { isBlock } from '../../to-schema'; +import { + Severity, + SourceCodeType, + type Context, + type LiquidCheckDefinition, + type ThemeBlockSchema, +} from '../../types'; +import { reportWarning } from '../../utils'; + +export const ValidBlockContentSettingType: LiquidCheckDefinition = { + meta: { + code: 'ValidBlockContentSettingType', + name: 'Valid Block Content Setting Type', + docs: { + description: + "Reports a theme block schema setting named 'content' whose Liquid type is not the built-in 'content' parameter's 'string' type.", + recommended: true, + url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/valid-block-content-setting-type', + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.ERROR, + schema: {}, + targets: [], + }, + + create(context) { + if (!isBlock(context.file.uri)) return {}; + + const blockName = path.basename(context.file.uri, '.liquid'); + + return { + async LiquidRawTag(node) { + if (node.name !== 'schema' || node.body.kind !== 'json') return; + if (!context.getBlockSchema) return; + + reportContentSettingTypeMismatch(context, await context.getBlockSchema(blockName)); + }, + }; + }, +}; + +function reportContentSettingTypeMismatch( + context: Context, + schema: ThemeBlockSchema | undefined, +): void { + if (!schema || schema.validSchema instanceof Error || schema.ast instanceof Error) return; + + const settings = schema.validSchema.settings ?? []; + const index = settings.findIndex((setting) => setting.id === BLOCK_CONTENT_PARAMETER); + if (index < 0) return; + + const settingType = schemaSettingLiquidType(settings[index].type); + const typeNode = nodeAtPath(schema.ast, ['settings', index, 'type']); + if (!settingType || settingType === 'string' || !typeNode) return; + + reportWarning( + `Schema setting 'content' has Liquid type '${settingType}', but the built-in 'content' parameter has type 'string'.`, + schema.offset, + typeNode, + context, + ); +} diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index 42352843a..a303c8add 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -28,6 +28,9 @@ AssetSizeJavaScript: enabled: true severity: 0 thresholdInBytes: 10000 +BlockContentUsage: + enabled: true + severity: 1 BlockIdUsage: enabled: true severity: 1 @@ -220,6 +223,9 @@ UnusedDocParam: ValidBlockArgumentTypes: enabled: true severity: 1 +ValidBlockContentSettingType: + enabled: true + severity: 0 ValidBlockTagPlacement: enabled: true severity: 0 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index a5bc12ecc..aa32580c5 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -6,6 +6,9 @@ ignore: AssetPreload: enabled: true severity: 1 +BlockContentUsage: + enabled: true + severity: 1 BlockIdUsage: enabled: true severity: 1 @@ -198,6 +201,9 @@ UnusedDocParam: ValidBlockArgumentTypes: enabled: true severity: 1 +ValidBlockContentSettingType: + enabled: true + severity: 0 ValidBlockTagPlacement: enabled: true severity: 0 diff --git a/packages/theme-language-server-common/src/TypeSystem.spec.ts b/packages/theme-language-server-common/src/TypeSystem.spec.ts index 2e6528753..bc5f7678a 100644 --- a/packages/theme-language-server-common/src/TypeSystem.spec.ts +++ b/packages/theme-language-server-common/src/TypeSystem.spec.ts @@ -117,6 +117,10 @@ describe('Module: TypeSystem', () => { name: 'settings', return_type: [{ type: 'untyped', name: '' }], }, + { + name: 'blocks', + return_type: [{ type: 'array', array_value: 'block' }], + }, ], }, { @@ -587,6 +591,109 @@ describe('Module: TypeSystem', () => { ); }); + describe('when a theme block uses the built-in content parameter', () => { + const blockUri = 'file:///blocks/card.liquid'; + + it.each(['content', 'block.content'])( + 'infers %s as a string without a schema or LiquidDoc', + async (expression) => { + const ast = toLiquidHtmlAST('{{ content }}{{ block.content }}'); + + const inferredType = await typeSystem.inferType( + liquidVariable(ast, expression), + ast, + blockUri, + ); + + expect(inferredType).toEqual('string'); + }, + ); + + it('makes content available as a string variable', async () => { + const ast = toLiquidHtmlAST('{{ content }}'); + + const variables = await typeSystem.availableVariables( + ast, + 'cont', + variableLookup(ast, 'content'), + blockUri, + ); + + expect(variables.map(({ entry, type }) => [entry.name, type])).toEqual([ + ['content', 'string'], + ]); + }); + + it('infers the assigned type after content is reassigned', async () => { + const ast = toLiquidHtmlAST('{{ content }}{% assign content = 1 %}{{ content }}'); + const [beforeAssign, afterAssign] = liquidVariables(ast, 'content'); + + const typeBeforeAssign = await typeSystem.inferType(beforeAssign, ast, blockUri); + const typeAfterAssign = await typeSystem.inferType(afterAssign, ast, blockUri); + + expect(typeBeforeAssign).toEqual('string'); + expect(typeAfterAssign).toEqual('number'); + }); + + it.each([ + [ + 'a non-string schema setting', + '{{ content }}{{ block.content }}{% schema %}{ "settings": [{ "type": "number", "id": "content", "label": "Content" }] }{% endschema %}', + ], + [ + 'a non-string LiquidDoc parameter', + '{% doc %}@param {number} content - Body{% enddoc %}{{ content }}{{ block.content }}', + ], + ])('keeps the string type over %s named content', async (_name, source) => { + const ast = toLiquidHtmlAST(source); + + const contentType = await typeSystem.inferType(liquidVariable(ast, 'content'), ast, blockUri); + const blockContentType = await typeSystem.inferType( + liquidVariable(ast, 'block.content'), + ast, + blockUri, + ); + + expect(contentType).toEqual('string'); + expect(blockContentType).toEqual('string'); + }); + + it.each([ + 'sections/card.liquid', + 'snippets/card.liquid', + 'shop/blocks/theme/sections/card.liquid', + 'shop/myblocks/card.liquid', + ])('does not expose content as a variable in %s', async (relativePath) => { + const ast = toLiquidHtmlAST('{{ content }}'); + + const inferredType = await typeSystem.inferType( + liquidVariable(ast, 'content'), + ast, + `file:///${relativePath}`, + ); + + expect(inferredType).toEqual('unknown'); + }); + + it.each([ + 'sections/main.liquid', + 'shop/blocks/theme/sections/main.liquid', + 'shop/myblocks/main.liquid', + ])('does not add content to the section block object in %s', async (relativePath) => { + const ast = toLiquidHtmlAST( + '{% for block in section.blocks %}{{ block.content }}{% endfor %}', + ); + + const inferredType = await typeSystem.inferType( + liquidVariable(ast, 'block.content'), + ast, + `file:///${relativePath}`, + ); + + expect(inferredType).toEqual('untyped'); + }); + }); + // TODO it.skip('should support narrowing the type of blocks', async () => { const sourceCode = ` diff --git a/packages/theme-language-server-common/src/TypeSystem.ts b/packages/theme-language-server-common/src/TypeSystem.ts index cc3ce23f4..e5bfc2c8a 100644 --- a/packages/theme-language-server-common/src/TypeSystem.ts +++ b/packages/theme-language-server-common/src/TypeSystem.ts @@ -14,6 +14,7 @@ import { } from '@shopify/liquid-html-parser'; import { ArrayReturnType, + BLOCK_CONTENT_PARAMETER, DocsetEntry, FilterEntry, MetafieldDefinitionMap, @@ -176,9 +177,13 @@ export class TypeSystem { }; } - // Deal with blocks/files.liquid block.settings in a similar fashion - if (/[\/\\]blocks[\/\\]/.test(uri) && result.block) { + // Deal with blocks/files.liquid block.settings in a similar fashion. + // `block.content` is the built-in string `content` parameter. + if (isThemeBlockFile(uri) && result.block) { result.block = JSON.parse(JSON.stringify(result.block)); // easy deep clone + result.block.properties = (result.block.properties ?? []) + .filter((property) => property.name !== BLOCK_CONTENT_PARAMETER) + .concat({ name: BLOCK_CONTENT_PARAMETER, return_type: [{ type: String, name: '' }] }); const settings = result.block.properties?.find((x) => x.name === 'settings'); if (!settings || !settings.return_type) return result; settings.return_type = [{ type: 'block_settings', name: '' }]; @@ -289,16 +294,16 @@ export class TypeSystem { }); private async symbolsTable(partialAst: LiquidHtmlNode, uri: string): Promise { - const schemaSettingTypes = blockSchemaSettingTypes(partialAst, uri); - const seedSymbolsTable = seedSchemaSettingVariables( + const blockVariableTypes = themeBlockVariableTypes(partialAst, uri); + const seedSymbolsTable = seedBlockVariables( await this.seedSymbolsTable(uri), - schemaSettingTypes, + blockVariableTypes, ); return buildSymbolsTable( partialAst, seedSymbolsTable, await this.themeDocset.liquidDrops(), - schemaSettingTypes, + blockVariableTypes, ); } @@ -346,8 +351,15 @@ const BLOCK_FILE_REGEX = /blocks[\/\\][^.\\\/]*\.liquid$/; const SNIPPET_FILE_REGEX = /snippets[\/\\][^.\\\/]*\.liquid$/; const LAYOUT_FILE_REGEX = /layout[\/\\]checkout\.liquid$/; +const THEME_BLOCK_FILE_REGEX = /(?:^|[\/\\])blocks[\/\\][^.\\\/]*\.liquid$/; + const BLOCK_CONTEXTUAL_ENTRIES = ['app', 'section', 'recommendations', 'block']; +/** Only files directly inside a directory named exactly `blocks` are theme block files. */ +function isThemeBlockFile(uri: string): boolean { + return THEME_BLOCK_FILE_REGEX.test(path.normalize(uri)); +} + function getContextualEntries(uri: string, mode: Mode = 'theme'): string[] { const normalizedUri = path.normalize(uri); if (LAYOUT_FILE_REGEX.test(normalizedUri)) { @@ -498,7 +510,7 @@ function buildSymbolsTable( partialAst: LiquidHtmlNode, seedSymbolsTable: SymbolsTable, liquidDrops: ObjectEntry[], - schemaSettingTypes: SchemaSettingTypes, + blockVariableTypes: BlockVariableTypes, ): SymbolsTable { const typeRanges = visit(partialAst, { // {% assign x = foo.x | filter %} @@ -514,12 +526,13 @@ function buildSymbolsTable( // @param {string} name - your name // {% enddoc %} // - // In a theme block, a schema setting with the same ID decides the type. + // In a theme block, a schema setting with the same ID or the built-in + // `content` parameter decides the type. LiquidDocParamNode(node) { const identifier = node.paramName.value; return { identifier, - type: schemaSettingTypes.get(identifier) ?? inferLiquidDocParamType(node, liquidDrops), + type: blockVariableTypes.get(identifier) ?? inferLiquidDocParamType(node, liquidDrops), range: [node.position.end], }; }, @@ -582,33 +595,36 @@ function buildSymbolsTable( }, seedSymbolsTable); } -/** The type of each schema setting ID, by setting ID. */ -type SchemaSettingTypes = Map; +/** The type of each variable that a theme block file gets implicitly, by name. */ +type BlockVariableTypes = Map; /** - * A theme block's schema settings are also plain variables in the block file. - * Other files get no schema setting variables. + * A theme block's schema settings are also plain variables in the block file, + * and the built-in `content` parameter is always a string variable there, + * even when a schema setting named `content` declares another type. + * Other files get no block variables. */ -function blockSchemaSettingTypes(partialAst: LiquidHtmlNode, uri: string): SchemaSettingTypes { - if (!BLOCK_FILE_REGEX.test(path.normalize(uri))) return new Map(); +function themeBlockVariableTypes(partialAst: LiquidHtmlNode, uri: string): BlockVariableTypes { + if (!isThemeBlockFile(uri)) return new Map(); - return new Map( + const schemaSettingTypes: BlockVariableTypes = new Map( schemaSettingsAsProperties(partialAst).map((setting) => [ setting.name, objectEntryType(setting), ]), ); + return schemaSettingTypes.set(BLOCK_CONTENT_PARAMETER, String); } /** - * Schema setting variables start at the top of the file, like other seeded - * variables, so later assigns and captures change their type by position. + * Block variables start at the top of the file, like other seeded variables, + * so later assigns and captures change their type by position. */ -function seedSchemaSettingVariables( +function seedBlockVariables( seedSymbolsTable: SymbolsTable, - schemaSettingTypes: SchemaSettingTypes, + blockVariableTypes: BlockVariableTypes, ): SymbolsTable { - for (const [identifier, type] of schemaSettingTypes) { + for (const [identifier, type] of blockVariableTypes) { seedSymbolsTable[identifier] ??= []; seedSymbolsTable[identifier].push({ identifier, type, range: [0] }); } diff --git a/packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts b/packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts index a3095f079..a967fe1bd 100644 --- a/packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts +++ b/packages/theme-language-server-common/src/completions/providers/ObjectCompletionProvider.spec.ts @@ -371,17 +371,7 @@ describe('Module: ObjectCompletionProvider', async () => { describe('when a theme block schema defines settings', () => { beforeEach(() => { - provider = new CompletionsProvider({ - documentManager: new DocumentManager(), - themeDocset: { - filters: async () => [], - objects: async () => blockSettingsObjects, - liquidDrops: async () => blockSettingsObjects, - tags: async () => [], - systemTranslations: async () => ({}), - }, - getMetafieldDefinitions: async () => ({}) as MetafieldDefinitionMap, - }); + provider = blockFileProvider(); }); it('completes a setting ID as a plain variable with its schema type', async () => { @@ -428,6 +418,54 @@ describe('Module: ObjectCompletionProvider', async () => { ); }); + describe('when a theme block uses the built-in content parameter', () => { + const contentItem = expect.objectContaining({ + label: 'content', + documentation: expect.objectContaining({ value: '### content: `string`' }), + }); + + beforeEach(() => { + provider = blockFileProvider(); + }); + + it('completes content as a string variable without a schema or LiquidDoc', async () => { + await expect(provider).to.complete( + { relativePath: 'blocks/card.liquid', source: '{{ cont█ }}' }, + [contentItem], + ); + }); + + it('completes block.content as a string property', async () => { + await expect(provider).to.complete( + { relativePath: 'blocks/card.liquid', source: '{{ block.cont█ }}' }, + [contentItem], + ); + }); + + it.each([ + 'sections/card.liquid', + 'snippets/card.liquid', + 'shop/blocks/theme/sections/card.liquid', + 'shop/myblocks/card.liquid', + ])('does not offer content as a variable in %s', async (relativePath) => { + await expect(provider).to.complete({ relativePath, source: '{{ cont█ }}' }, []); + }); + + it.each([ + 'sections/main.liquid', + 'shop/blocks/theme/sections/main.liquid', + 'shop/myblocks/main.liquid', + ])('does not add content to the section block object in %s', async (relativePath) => { + await expect(provider).to.complete( + { + relativePath, + source: '{% for block in section.blocks %}{{ block.█ }}{% endfor %}', + }, + ['settings'], + ); + }); + }); + it('should complete metafields defined by getMetafieldDefinitions', async () => { await expect(provider).to.complete('{% echo product.metafields.█ %}', ['custom']); await expect(provider).to.complete('{% echo product.metafields.custom.█ %}', ['color']); @@ -445,6 +483,15 @@ const blockSettingsObjects: ObjectEntry[] = [ return_type: [], properties: [{ name: 'settings', return_type: [{ type: 'untyped', name: '' }] }], }, + { + name: 'section', + access: { global: false, parents: [], template: [] }, + return_type: [], + properties: [ + { name: 'settings', return_type: [{ type: 'untyped', name: '' }] }, + { name: 'blocks', return_type: [{ type: 'array', array_value: 'block' }] }, + ], + }, { name: 'image', access: { global: false, parents: [], template: [] }, @@ -452,6 +499,20 @@ const blockSettingsObjects: ObjectEntry[] = [ }, ]; +function blockFileProvider() { + return new CompletionsProvider({ + documentManager: new DocumentManager(), + themeDocset: { + filters: async () => [], + objects: async () => blockSettingsObjects, + liquidDrops: async () => blockSettingsObjects, + tags: async () => [], + systemTranslations: async () => ({}), + }, + getMetafieldDefinitions: async () => ({}) as MetafieldDefinitionMap, + }); +} + function articleCard(body: string, docParam?: string) { return { relativePath: 'blocks/article-card.liquid', diff --git a/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts b/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts index 759fddc46..f98985427 100644 --- a/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts +++ b/packages/theme-language-server-common/src/hover/providers/LiquidObjectHoverProvider.spec.ts @@ -246,17 +246,7 @@ describe('Module: LiquidObjectHoverProvider', async () => { describe('when a theme block schema defines settings', () => { beforeEach(() => { - provider = new HoverProvider( - new DocumentManager(), - { - filters: async () => [], - objects: async () => blockSettingsObjects, - liquidDrops: async () => blockSettingsObjects, - tags: async () => [], - systemTranslations: async () => ({}), - }, - async () => ({}) as MetafieldDefinitionMap, - ); + provider = blockFileProvider(); }); it('hovers a setting ID as a plain variable with its schema type', async () => { @@ -295,6 +285,49 @@ describe('Module: LiquidObjectHoverProvider', async () => { ); }); + describe('when a theme block uses the built-in content parameter', () => { + beforeEach(() => { + provider = blockFileProvider(); + }); + + it('hovers content as a string variable without a schema or LiquidDoc', async () => { + await expect(provider).to.hover( + { relativePath: 'blocks/card.liquid', source: '{{ cont█ent }}' }, + '### content: `string`', + ); + }); + + it('hovers block.content as a string property', async () => { + await expect(provider).to.hover( + { relativePath: 'blocks/card.liquid', source: '{{ block.cont█ent }}' }, + '### content: `string`', + ); + }); + + it.each([ + 'sections/card.liquid', + 'snippets/card.liquid', + 'shop/blocks/theme/sections/card.liquid', + 'shop/myblocks/card.liquid', + ])('does not hover content as a variable in %s', async (relativePath) => { + await expect(provider).to.hover({ relativePath, source: '{{ cont█ent }}' }, null); + }); + + it.each([ + 'sections/main.liquid', + 'shop/blocks/theme/sections/main.liquid', + 'shop/myblocks/main.liquid', + ])('does not hover content on the section block object in %s', async (relativePath) => { + await expect(provider).to.hover( + { + relativePath, + source: '{% for block in section.blocks %}{{ block.cont█ent }}{% endfor %}', + }, + null, + ); + }); + }); + it('should return null when hovering over an undefined variable', async () => { await expect(provider).to.hover(`{{ unknown█ }}`, null); }); @@ -340,6 +373,15 @@ const blockSettingsObjects: ObjectEntry[] = [ return_type: [], properties: [{ name: 'settings', return_type: [{ type: 'untyped', name: '' }] }], }, + { + name: 'section', + access: { global: false, parents: [], template: [] }, + return_type: [], + properties: [ + { name: 'settings', return_type: [{ type: 'untyped', name: '' }] }, + { name: 'blocks', return_type: [{ type: 'array', array_value: 'block' }] }, + ], + }, { name: 'image', description: 'image description', @@ -348,6 +390,20 @@ const blockSettingsObjects: ObjectEntry[] = [ }, ]; +function blockFileProvider() { + return new HoverProvider( + new DocumentManager(), + { + filters: async () => [], + objects: async () => blockSettingsObjects, + liquidDrops: async () => blockSettingsObjects, + tags: async () => [], + systemTranslations: async () => ({}), + }, + async () => ({}) as MetafieldDefinitionMap, + ); +} + function articleCard(body: string, docParam?: string) { return { relativePath: 'blocks/article-card.liquid', From 470303814b4efe7f6ed07d071de54c84de042a70 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 30 Sep 2026 11:30:42 -0400 Subject: [PATCH 2/2] Make block content usage an error --- .changeset/block-content-implicit-argument-checks.md | 2 +- .../src/checks/block-content-usage/index.spec.ts | 6 +++--- .../src/checks/block-content-usage/index.ts | 2 +- packages/theme-check-node/configs/all.yml | 2 +- packages/theme-check-node/configs/recommended.yml | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.changeset/block-content-implicit-argument-checks.md b/.changeset/block-content-implicit-argument-checks.md index 605c6db1e..d833d96cf 100644 --- a/.changeset/block-content-implicit-argument-checks.md +++ b/.changeset/block-content-implicit-argument-checks.md @@ -5,7 +5,7 @@ Guide theme blocks to the implicit `content` parameter. -Add the recommended `BlockContentUsage` warning for `block.content` in `blocks/*.liquid`. Its message is "Use the implicit 'content' parameter directly instead of 'block.content'." +Add the recommended `BlockContentUsage` error for `block.content` in `blocks/*.liquid`. Its message is "Use the implicit 'content' parameter directly instead of 'block.content'." Add the recommended `ValidBlockContentSettingType` error for a theme block schema setting named `content` whose Liquid type is not `string`. This error replaces the `ValidBlockArgumentTypes` warning for the same schema declaration. diff --git a/packages/theme-check-common/src/checks/block-content-usage/index.spec.ts b/packages/theme-check-common/src/checks/block-content-usage/index.spec.ts index 305570728..31ef96fd2 100644 --- a/packages/theme-check-common/src/checks/block-content-usage/index.spec.ts +++ b/packages/theme-check-common/src/checks/block-content-usage/index.spec.ts @@ -7,9 +7,9 @@ import { BlockContentUsage } from './index'; const MESSAGE = "Use the implicit 'content' parameter directly instead of 'block.content'."; describe('BlockContentUsage', () => { - it('is a recommended warning', () => { + it('is a recommended error', () => { expect(recommended).toContain(BlockContentUsage); - expect(BlockContentUsage.meta.severity).toBe(Severity.WARNING); + expect(BlockContentUsage.meta.severity).toBe(Severity.ERROR); }); it.each([ @@ -22,7 +22,7 @@ describe('BlockContentUsage', () => { ])('reports the block.content lookup in %j', async (source, highlight) => { const offenses = await runLiquidCheck(BlockContentUsage, source, 'blocks/card.liquid'); - expect(offenses).toMatchObject([{ message: MESSAGE, severity: Severity.WARNING }]); + expect(offenses).toMatchObject([{ message: MESSAGE, severity: Severity.ERROR }]); expect(highlightedOffenses({ 'blocks/card.liquid': source }, offenses)).toEqual([highlight]); }); diff --git a/packages/theme-check-common/src/checks/block-content-usage/index.ts b/packages/theme-check-common/src/checks/block-content-usage/index.ts index 3e2133679..6cfc817ea 100644 --- a/packages/theme-check-common/src/checks/block-content-usage/index.ts +++ b/packages/theme-check-common/src/checks/block-content-usage/index.ts @@ -17,7 +17,7 @@ export const BlockContentUsage: LiquidCheckDefinition = { url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/block-content-usage', }, type: SourceCodeType.LiquidHtml, - severity: Severity.WARNING, + severity: Severity.ERROR, schema: {}, targets: [], }, diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index a303c8add..7b5b8dc91 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -30,7 +30,7 @@ AssetSizeJavaScript: thresholdInBytes: 10000 BlockContentUsage: enabled: true - severity: 1 + severity: 0 BlockIdUsage: enabled: true severity: 1 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index aa32580c5..255e4136e 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -8,7 +8,7 @@ AssetPreload: severity: 1 BlockContentUsage: enabled: true - severity: 1 + severity: 0 BlockIdUsage: enabled: true severity: 1