From 568d53b8a05a5a27d3ae78b98c2830f489da29f8 Mon Sep 17 00:00:00 2001 From: CP Clermont Date: Fri, 8 Nov 2024 14:24:35 -0500 Subject: [PATCH] Add `content_for` tag parser support + `ValidContentForArguments` check (#568) Fixes #466 --- .changeset/purple-pears-report.md | 7 ++ .changeset/tasty-mugs-behave.md | 7 ++ .../grammar/liquid-html.ohm | 6 ++ .../src/stage-1-cst.spec.ts | 39 ++++++++ .../liquid-html-parser/src/stage-1-cst.ts | 22 +++++ .../src/stage-2-ast.spec.ts | 41 ++++++++ .../liquid-html-parser/src/stage-2-ast.ts | 33 +++++++ packages/liquid-html-parser/src/types.ts | 2 + .../preprocess/augment-with-css-properties.ts | 2 + .../src/printer/print/liquid.ts | 6 ++ .../src/printer/printer-liquid-html.ts | 17 ++++ .../test/liquid-tag-content-for/fixed.liquid | 21 ++++ .../test/liquid-tag-content-for/index.liquid | 18 ++++ .../test/liquid-tag-content-for/index.spec.ts | 6 ++ .../capture-on-content-for-block/index.ts | 12 ++- .../theme-check-common/src/checks/index.ts | 10 +- .../liquid-html-syntax-error/index.spec.ts | 2 +- .../checks/unique-static-block-id/index.ts | 37 ++++---- .../valid-content-for-arguments/index.spec.ts | 75 +++++++++++++++ .../valid-content-for-arguments/index.ts | 95 +++++++++++++++++++ .../valid-static-block-type/index.spec.ts | 12 +-- .../checks/valid-static-block-type/index.ts | 32 +++---- .../src/test/MockFileSystem.ts | 2 +- .../theme-check-common/src/utils/markup.ts | 11 ++- packages/theme-check-node/configs/all.yml | 3 + .../theme-check-node/configs/recommended.yml | 3 + .../params/LiquidCompletionParams.spec.ts | 16 ++++ .../params/LiquidCompletionParams.ts | 12 +++ .../RenderSnippetCompletionProvider.ts | 12 ++- 29 files changed, 499 insertions(+), 62 deletions(-) create mode 100644 .changeset/purple-pears-report.md create mode 100644 .changeset/tasty-mugs-behave.md create mode 100644 packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/fixed.liquid create mode 100644 packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/index.liquid create mode 100644 packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/index.spec.ts create mode 100644 packages/theme-check-common/src/checks/valid-content-for-arguments/index.spec.ts create mode 100644 packages/theme-check-common/src/checks/valid-content-for-arguments/index.ts diff --git a/.changeset/purple-pears-report.md b/.changeset/purple-pears-report.md new file mode 100644 index 000000000..9240a726d --- /dev/null +++ b/.changeset/purple-pears-report.md @@ -0,0 +1,7 @@ +--- +'@shopify/theme-check-browser': minor +'@shopify/theme-check-common': minor +'@shopify/theme-check-node': minor +--- + +Add the `ValidContentForArguments` check diff --git a/.changeset/tasty-mugs-behave.md b/.changeset/tasty-mugs-behave.md new file mode 100644 index 000000000..386f814a9 --- /dev/null +++ b/.changeset/tasty-mugs-behave.md @@ -0,0 +1,7 @@ +--- +'@shopify/liquid-html-parser': minor +'@shopify/prettier-plugin-liquid': minor +'@shopify/theme-language-server-common': minor +--- + +Add support for the `content_for` Liquid tag diff --git a/packages/liquid-html-parser/grammar/liquid-html.ohm b/packages/liquid-html-parser/grammar/liquid-html.ohm index face7fb36..a431a97be 100644 --- a/packages/liquid-html-parser/grammar/liquid-html.ohm +++ b/packages/liquid-html-parser/grammar/liquid-html.ohm @@ -50,6 +50,7 @@ Liquid <: Helpers { | liquidTagBreak | liquidTagContinue | liquidTagCycle + | liquidTagContentFor | liquidTagDecrement | liquidTagEcho | liquidTagElse @@ -125,6 +126,11 @@ Liquid <: Helpers { liquidTagLiquid = liquidTagRule<"liquid", liquidTagLiquidMarkup> liquidTagLiquidMarkup = tagMarkup + liquidTagContentFor = liquidTagRule<"content_for", liquidTagContentForMarkup> + liquidTagContentForMarkup = + contentForType (argumentSeparatorOptionalComma tagArguments) (space* ",")? space* + contentForType = liquidString + liquidTagInclude = liquidTagRule<"include", liquidTagRenderMarkup> liquidTagRender = liquidTagRule<"render", liquidTagRenderMarkup> liquidTagRenderMarkup = diff --git a/packages/liquid-html-parser/src/stage-1-cst.spec.ts b/packages/liquid-html-parser/src/stage-1-cst.spec.ts index 0e36c04df..147916b23 100644 --- a/packages/liquid-html-parser/src/stage-1-cst.spec.ts +++ b/packages/liquid-html-parser/src/stage-1-cst.spec.ts @@ -586,6 +586,45 @@ describe('Unit: Stage 1 (CST)', () => { expectPath(cst, '0.markup.expression.lookups').to.eql([]); } }); + + it('should parse content_for "blocks"', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% content_for "blocks" -%}`); + expectPath(cst, '0.type').to.equal('LiquidTag'); + expectPath(cst, '0.name').to.equal('content_for'); + expectPath(cst, '0.markup.type').to.equal('ContentForMarkup'); + expectPath(cst, '0.markup.contentForType.type').to.equal('String'); + expectPath(cst, '0.markup.contentForType.value').to.equal('blocks'); + expectPath(cst, '0.markup.contentForType.single').to.equal(false); + expectPath(cst, '0.markup.args').to.have.lengthOf(0); + expectPath(cst, '0.whitespaceStart').to.equal(null); + expectPath(cst, '0.whitespaceEnd').to.equal('-'); + } + }); + + it('should parse content_for "block", id: "my-id", type: "my-block"', () => { + for (const { toCST, expectPath } of testCases) { + cst = toCST(`{% content_for "block", id: "block-id", type: "block-type" -%}`); + expectPath(cst, '0.type').to.equal('LiquidTag'); + expectPath(cst, '0.name').to.equal('content_for'); + expectPath(cst, '0.markup.type').to.equal('ContentForMarkup'); + expectPath(cst, '0.markup.contentForType.type').to.equal('String'); + expectPath(cst, '0.markup.contentForType.value').to.equal('block'); + expectPath(cst, '0.markup.contentForType.single').to.equal(false); + expectPath(cst, '0.markup.args').to.have.lengthOf(2); + const namedArguments = [ + { name: 'id', valueType: 'String' }, + { name: 'type', valueType: 'String' }, + ]; + namedArguments.forEach(({ name, valueType }, i) => { + expectPath(cst, `0.markup.args.${i}.type`).to.equal('NamedArgument'); + expectPath(cst, `0.markup.args.${i}.name`).to.equal(name); + expectPath(cst, `0.markup.args.${i}.value.type`).to.equal(valueType); + }); + expectPath(cst, '0.whitespaceStart').to.equal(null); + expectPath(cst, '0.whitespaceEnd').to.equal('-'); + } + }); }); describe('Case: LiquidTagOpen', () => { diff --git a/packages/liquid-html-parser/src/stage-1-cst.ts b/packages/liquid-html-parser/src/stage-1-cst.ts index 5ab42aeff..36c2ab853 100644 --- a/packages/liquid-html-parser/src/stage-1-cst.ts +++ b/packages/liquid-html-parser/src/stage-1-cst.ts @@ -75,6 +75,7 @@ export enum ConcreteNodeTypes { Condition = 'Condition', AssignMarkup = 'AssignMarkup', + ContentForMarkup = 'ContentForMarkup', CycleMarkup = 'CycleMarkup', ForMarkup = 'ForMarkup', RenderMarkup = 'RenderMarkup', @@ -265,6 +266,7 @@ export type ConcreteLiquidTag = ConcreteLiquidTagNamed | ConcreteLiquidTagBaseCa export type ConcreteLiquidTagNamed = | ConcreteLiquidTagAssign | ConcreteLiquidTagCycle + | ConcreteLiquidTagContentFor | ConcreteLiquidTagEcho | ConcreteLiquidTagIncrement | ConcreteLiquidTagDecrement @@ -321,11 +323,20 @@ export interface ConcreteLiquidTagCycleMarkup args: ConcreteLiquidExpression[]; } +export interface ConcreteLiquidTagContentFor + extends ConcreteLiquidTagNode {} + export interface ConcreteLiquidTagRender extends ConcreteLiquidTagNode {} export interface ConcreteLiquidTagInclude extends ConcreteLiquidTagNode {} +export interface ConcreteLiquidTagContentForMarkup + extends ConcreteBasicNode { + contentForType: ConcreteStringLiteral; + args: ConcreteLiquidNamedArgument[]; +} + export interface ConcreteLiquidTagRenderMarkup extends ConcreteBasicNode { snippet: ConcreteStringLiteral | ConcreteLiquidVariableLookup; @@ -715,6 +726,7 @@ function toCST( liquidTagBaseCase: 0, liquidTagAssign: 0, liquidTagEcho: 0, + liquidTagContentFor: 0, liquidTagCycle: 0, liquidTagIncrement: 0, liquidTagDecrement: 0, @@ -775,6 +787,16 @@ function toCST( source, }, + liquidTagContentForMarkup: { + type: ConcreteNodeTypes.ContentForMarkup, + contentForType: 0, + args: 2, + locStart, + locEnd, + source, + }, + contentForType: 0, + liquidTagRenderMarkup: { type: ConcreteNodeTypes.RenderMarkup, snippet: 0, diff --git a/packages/liquid-html-parser/src/stage-2-ast.spec.ts b/packages/liquid-html-parser/src/stage-2-ast.spec.ts index d4e5c7b09..de6de2631 100644 --- a/packages/liquid-html-parser/src/stage-2-ast.spec.ts +++ b/packages/liquid-html-parser/src/stage-2-ast.spec.ts @@ -579,6 +579,47 @@ describe('Unit: Stage 2 (AST)', () => { }); }); }); + + describe('Case: content_for', () => { + it('should parse content_for tags with no arguments', () => { + for (const { toAST, expectPath, expectPosition } of testCases) { + ast = toAST(`{% content_for "blocks" %}`); + expectPath(ast, 'children.0.type').to.equal('LiquidTag'); + expectPath(ast, 'children.0.name').to.equal('content_for'); + expectPath(ast, 'children.0.markup.type').to.equal('ContentForMarkup'); + expectPath(ast, 'children.0.markup.contentForType.type').to.equal('String'); + expectPath(ast, 'children.0.markup.contentForType.value').to.equal('blocks'); + expectPosition(ast, 'children.0'); + expectPosition(ast, 'children.0.markup'); + } + }); + + it('should parse content_for named expression arguments', () => { + for (const { toAST, expectPath, expectPosition } of testCases) { + ast = toAST(`{% content_for "snippet", s: 'string', n: 10, r: (1..2), v: variable %}`); + expectPath(ast, 'children.0.type').to.equal('LiquidTag'); + expectPath(ast, 'children.0.name').to.equal('content_for'); + expectPath(ast, 'children.0.markup.type').to.equal('ContentForMarkup'); + expectPath(ast, 'children.0.markup.contentForType.type').to.equal('String'); + expectPath(ast, 'children.0.markup.contentForType.value').to.equal('snippet'); + expectPath(ast, 'children.0.markup.args').to.have.lengthOf(4); + expectPath(ast, 'children.0.markup.args.0.type').to.equal('NamedArgument'); + expectPath(ast, 'children.0.markup.args.0.name').to.equal('s'); + expectPath(ast, 'children.0.markup.args.0.value.type').to.equal('String'); + expectPath(ast, 'children.0.markup.args.1.type').to.equal('NamedArgument'); + expectPath(ast, 'children.0.markup.args.1.name').to.equal('n'); + expectPath(ast, 'children.0.markup.args.1.value.type').to.equal('Number'); + expectPath(ast, 'children.0.markup.args.2.type').to.equal('NamedArgument'); + expectPath(ast, 'children.0.markup.args.2.name').to.equal('r'); + expectPath(ast, 'children.0.markup.args.2.value.type').to.equal('Range'); + expectPath(ast, 'children.0.markup.args.3.type').to.equal('NamedArgument'); + expectPath(ast, 'children.0.markup.args.3.name').to.equal('v'); + expectPath(ast, 'children.0.markup.args.3.value.type').to.equal('VariableLookup'); + expectPosition(ast, 'children.0'); + expectPosition(ast, 'children.0.markup'); + } + }); + }); }); it(`should parse liquid inline comments`, () => { diff --git a/packages/liquid-html-parser/src/stage-2-ast.ts b/packages/liquid-html-parser/src/stage-2-ast.ts index 8cd0559c7..fc0ed1343 100644 --- a/packages/liquid-html-parser/src/stage-2-ast.ts +++ b/packages/liquid-html-parser/src/stage-2-ast.ts @@ -72,6 +72,7 @@ import { ConcreteLiquidRawTag, LiquidHtmlConcreteNode, ConcreteLiquidTagBaseCase, + ConcreteLiquidTagContentForMarkup, } from './stage-1-cst'; import { Comparators, NamedTags, NodeTypes, nonTraversableProperties, Position } from './types'; import { assertNever, deepGet, dropLast } from './utils'; @@ -97,6 +98,7 @@ export type LiquidHtmlNode = | LiquidFilter | LiquidNamedArgument | AssignMarkup + | ContentForMarkup | CycleMarkup | ForMarkup | RenderMarkup @@ -188,6 +190,7 @@ export type LiquidTagNamed = | LiquidTagAssign | LiquidTagCase | LiquidTagCapture + | LiquidTagContentFor | LiquidTagCycle | LiquidTagDecrement | LiquidTagEcho @@ -359,6 +362,10 @@ export interface PaginateMarkup extends ASTNode { args: LiquidNamedArgument[]; } +/** https://shopify.dev/docs/api/liquid/tags#content_for */ +export interface LiquidTagContentFor + extends LiquidTagNode {} + /** https://shopify.dev/docs/api/liquid/tags#render */ export interface LiquidTagRender extends LiquidTagNode {} @@ -377,6 +384,14 @@ export interface LiquidTagLayout extends LiquidTagNode {} +/** {% content_for 'contentForType' [...namedArguments] %} */ +export interface ContentForMarkup extends ASTNode { + /** {% content_for 'contentForType' %} */ + contentForType: LiquidString; + /** {% content_for 'contentForType', arg1: value1, arg2: value2 %} */ + args: LiquidNamedArgument[]; +} + /** {% render 'snippet' [(with|for) variable [as alias]], [...namedArguments] %} */ export interface RenderMarkup extends ASTNode { /** {% render snippet %} */ @@ -1408,6 +1423,14 @@ function toNamedLiquidTag( }; } + case NamedTags.content_for: { + return { + ...liquidTagBaseAttributes(node), + name: node.name, + markup: toContentForMarkup(node.markup), + }; + } + case NamedTags.include: case NamedTags.render: { return { @@ -1684,6 +1707,16 @@ function toRawMarkupKindFromLiquidNode(node: ConcreteLiquidRawTag): RawMarkupKin } } +function toContentForMarkup(node: ConcreteLiquidTagContentForMarkup): ContentForMarkup { + return { + type: NodeTypes.ContentForMarkup, + contentForType: toExpression(node.contentForType) as LiquidString, + args: node.args.map(toNamedArgument), + position: position(node), + source: node.source, + }; +} + function toRenderMarkup(node: ConcreteLiquidTagRenderMarkup): RenderMarkup { return { type: NodeTypes.RenderMarkup, diff --git a/packages/liquid-html-parser/src/types.ts b/packages/liquid-html-parser/src/types.ts index aaf6d1b40..29648aebb 100644 --- a/packages/liquid-html-parser/src/types.ts +++ b/packages/liquid-html-parser/src/types.ts @@ -37,6 +37,7 @@ export enum NodeTypes { LogicalExpression = 'LogicalExpression', AssignMarkup = 'AssignMarkup', + ContentForMarkup = 'ContentForMarkup', CycleMarkup = 'CycleMarkup', ForMarkup = 'ForMarkup', PaginateMarkup = 'PaginateMarkup', @@ -50,6 +51,7 @@ export enum NamedTags { assign = 'assign', capture = 'capture', case = 'case', + content_for = 'content_for', cycle = 'cycle', decrement = 'decrement', echo = 'echo', diff --git a/packages/prettier-plugin-liquid/src/printer/preprocess/augment-with-css-properties.ts b/packages/prettier-plugin-liquid/src/printer/preprocess/augment-with-css-properties.ts index 9ba5d3063..cc16285e5 100644 --- a/packages/prettier-plugin-liquid/src/printer/preprocess/augment-with-css-properties.ts +++ b/packages/prettier-plugin-liquid/src/printer/preprocess/augment-with-css-properties.ts @@ -121,6 +121,7 @@ function getCssDisplay(node: AugmentedNode, options: LiquidParserO case NodeTypes.VariableLookup: case NodeTypes.AssignMarkup: case NodeTypes.CycleMarkup: + case NodeTypes.ContentForMarkup: case NodeTypes.ForMarkup: case NodeTypes.PaginateMarkup: case NodeTypes.RenderMarkup: @@ -225,6 +226,7 @@ function getNodeCssStyleWhiteSpace( case NodeTypes.VariableLookup: case NodeTypes.AssignMarkup: case NodeTypes.CycleMarkup: + case NodeTypes.ContentForMarkup: case NodeTypes.ForMarkup: case NodeTypes.PaginateMarkup: case NodeTypes.RenderMarkup: diff --git a/packages/prettier-plugin-liquid/src/printer/print/liquid.ts b/packages/prettier-plugin-liquid/src/printer/print/liquid.ts index c84c145dc..778c7262e 100644 --- a/packages/prettier-plugin-liquid/src/printer/print/liquid.ts +++ b/packages/prettier-plugin-liquid/src/printer/print/liquid.ts @@ -165,6 +165,12 @@ function printNamedLiquidBlockStart( ]); } + case NamedTags.content_for: { + const markup = node.markup; + const trailingWhitespace = markup.args.length > 0 ? line : ' '; + return tag(trailingWhitespace); + } + case NamedTags.include: case NamedTags.render: { const markup = node.markup; diff --git a/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts b/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts index 27007a9a0..3905c8008 100644 --- a/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts +++ b/packages/prettier-plugin-liquid/src/printer/printer-liquid-html.ts @@ -378,6 +378,23 @@ function printNode( return doc; } + case NodeTypes.ContentForMarkup: { + const contentForType = path.call((p: any) => print(p), 'contentForType'); + const doc: Doc = [contentForType]; + if (node.args.length > 0) { + doc.push( + ',', + line, + join( + [',', line], + path.map((p) => print(p), 'args'), + ), + ); + } + + return doc; + } + case NodeTypes.RenderMarkup: { const snippet = path.call((p: any) => print(p), 'snippet'); const doc: Doc = [snippet]; diff --git a/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/fixed.liquid b/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/fixed.liquid new file mode 100644 index 000000000..4d6a4abff --- /dev/null +++ b/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/fixed.liquid @@ -0,0 +1,21 @@ +It should never break a name +printWidth: 1 +{% content_for 'block' %} +{%- content_for 'blocks' -%} + +It should break on named args +printWidth: 1 +{% content_for 'block', + key1: val1, + key2: (0..1) +%} + +It should not require commas (as per syntax) but add them when pretty printed +{% content_for 'block', key1: val1, key2: (0..1) %} + +It should not require spaces (as per syntax) but add them when pretty printed +{% content_for 'block', key1: val1, key2: (0..1) %} + +It should strip trailing commas by default +{% content_for 'foo' %} +{% content_for 'foo', product: product %} diff --git a/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/index.liquid b/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/index.liquid new file mode 100644 index 000000000..ee8a1d1b1 --- /dev/null +++ b/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/index.liquid @@ -0,0 +1,18 @@ +It should never break a name +printWidth: 1 +{% content_for "block" %} +{%- content_for "blocks" -%} + +It should break on named args +printWidth: 1 +{% content_for "block", key1: val1, key2: (0..1) %} + +It should not require commas (as per syntax) but add them when pretty printed +{% content_for "block" key1: val1 key2: (0..1) %} + +It should not require spaces (as per syntax) but add them when pretty printed +{%content_for "block",key1:val1,key2:(0..1)%} + +It should strip trailing commas by default +{% content_for "foo", %} +{% content_for "foo",product:product, %} diff --git a/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/index.spec.ts b/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/index.spec.ts new file mode 100644 index 000000000..101a92058 --- /dev/null +++ b/packages/prettier-plugin-liquid/src/test/liquid-tag-content-for/index.spec.ts @@ -0,0 +1,6 @@ +import { test } from 'vitest'; +import { assertFormattedEqualsFixed } from '../test-helpers'; + +test('Unit: liquid-tag-content-for', async () => { + await assertFormattedEqualsFixed(__dirname); +}); diff --git a/packages/theme-check-common/src/checks/capture-on-content-for-block/index.ts b/packages/theme-check-common/src/checks/capture-on-content-for-block/index.ts index ae48e9cca..ccd1ff9de 100644 --- a/packages/theme-check-common/src/checks/capture-on-content-for-block/index.ts +++ b/packages/theme-check-common/src/checks/capture-on-content-for-block/index.ts @@ -1,4 +1,10 @@ -import { LiquidTag, LiquidTagCapture, NodeTypes, Position } from '@shopify/liquid-html-parser'; +import { + LiquidTag, + LiquidTagCapture, + NamedTags, + NodeTypes, + Position, +} from '@shopify/liquid-html-parser'; import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; import { isContentForBlock } from '../../utils/markup'; import { isNodeOfType } from '../utils'; @@ -27,7 +33,7 @@ export const CaptureOnContentForBlock: LiquidCheckDefinition = { function checkContentForBlock(node: any, position: Position) { if ( isNodeOfType(NodeTypes.LiquidTag, node) && - node.name === 'content_for' && + node.name === NamedTags.content_for && isContentForBlock(node.markup) ) { context.report({ @@ -42,7 +48,7 @@ export const CaptureOnContentForBlock: LiquidCheckDefinition = { async LiquidTag(node) { if (isLiquidTagCapture(node) && node.children) { for (const child of node.children) { - if (child.type === NodeTypes.LiquidTag && typeof child.markup == 'string') { + if (child.type === NodeTypes.LiquidTag) { checkContentForBlock(child, child.position); } } diff --git a/packages/theme-check-common/src/checks/index.ts b/packages/theme-check-common/src/checks/index.ts index 2d828d5c6..764a88969 100644 --- a/packages/theme-check-common/src/checks/index.ts +++ b/packages/theme-check-common/src/checks/index.ts @@ -6,8 +6,8 @@ import { AssetSizeAppBlockCSS } from './asset-size-app-block-css'; import { AssetSizeAppBlockJavaScript } from './asset-size-app-block-javascript'; import { AssetSizeCSS } from './asset-size-css'; import { AssetSizeJavaScript } from './asset-size-javascript'; -import { CdnPreconnect } from './cdn-preconnect'; import { CaptureOnContentForBlock } from './capture-on-content-for-block'; +import { CdnPreconnect } from './cdn-preconnect'; import { ContentForHeaderModification } from './content-for-header-modification'; import { DeprecateBgsizes } from './deprecate-bgsizes'; import { DeprecateLazysizes } from './deprecate-lazysizes'; @@ -26,14 +26,15 @@ import { RequiredLayoutThemeObject } from './required-layout-theme-object'; import { TranslationKeyExists } from './translation-key-exists'; import { UnclosedHTMLElement } from './unclosed-html-element'; import { UndefinedObject } from './undefined-object'; +import { UniqueStaticBlockId } from './unique-static-block-id'; import { UnknownFilter } from './unknown-filter'; import { UnusedAssign } from './unused-assign'; +import { ValidContentForArguments } from './valid-content-for-arguments'; import { ValidHTMLTranslation } from './valid-html-translation'; -import { ValidSchema } from './valid-schema'; import { ValidJSON } from './valid-json'; -import { VariableName } from './variable-name'; +import { ValidSchema } from './valid-schema'; import { ValidStaticBlockType } from './valid-static-block-type'; -import { UniqueStaticBlockId } from './unique-static-block-id'; +import { VariableName } from './variable-name'; export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ AppBlockValidTags, @@ -66,6 +67,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [ UnknownFilter, UnusedAssign, ValidHTMLTranslation, + ValidContentForArguments, ValidJSON, ValidSchema, ValidStaticBlockType, diff --git a/packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts b/packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts index b4b9d70fc..d73d8ab01 100644 --- a/packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts +++ b/packages/theme-check-common/src/checks/liquid-html-syntax-error/index.spec.ts @@ -86,7 +86,7 @@ describe('Module: LiquidHTMLSyntaxError', () => { const offenses = await runLiquidCheck(LiquidHTMLSyntaxError, sourceCode); expect(offenses).to.have.length(1); expect(offenses[0].message).to.equal( - `SyntaxError: expected "#", a letter, "when", "sections", "section", "render", "liquid", "layout", "increment", "include", "elsif", "else", "echo", "decrement", "cycle", "continue", "break", "assign", "tablerow", "unless", "if", "ifchanged", "for", "case", "capture", "paginate", "form", "end", "style", "stylesheet", "schema", "javascript", "raw", or "comment"`, + `SyntaxError: expected "#", a letter, "when", "sections", "section", "render", "liquid", "layout", "increment", "include", "elsif", "else", "echo", "decrement", "content_for", "cycle", "continue", "break", "assign", "tablerow", "unless", "if", "ifchanged", "for", "case", "capture", "paginate", "form", "end", "style", "stylesheet", "schema", "javascript", "raw", or "comment"`, ); }); diff --git a/packages/theme-check-common/src/checks/unique-static-block-id/index.ts b/packages/theme-check-common/src/checks/unique-static-block-id/index.ts index 05324dd44..daf33c733 100644 --- a/packages/theme-check-common/src/checks/unique-static-block-id/index.ts +++ b/packages/theme-check-common/src/checks/unique-static-block-id/index.ts @@ -1,4 +1,6 @@ +import { NamedTags, NodeTypes } from '@shopify/liquid-html-parser'; import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; +import { isContentForBlock } from '../../utils/markup'; export const UniqueStaticBlockId: LiquidCheckDefinition = { meta: { @@ -21,41 +23,36 @@ export const UniqueStaticBlockId: LiquidCheckDefinition = { const idRegex = /id:\s*["'](\S+)["']/; return { async LiquidTag(node) { - if (node.name !== 'content_for') { + if (node.name !== NamedTags.content_for) { return; } - const [blockType] = node.markup.split(','); - - if (blockType.replace(/["']/g, '') !== 'block') { + if (!isContentForBlock(node.markup)) { return; } - const idValueMatch = idRegex.exec(node.markup); + const idNode = node.markup.args.find((arg) => arg.name === 'id'); - if (idValueMatch == null) { - return; + if (!idNode) { + return; // covered by VariableContentForArguments } - const [entireIdTerm, filteredIdValue] = idValueMatch; - - if (usedIds.has(filteredIdValue)) { - const nodeInSource = node.source.substring(node.position.start); - const contentForBlockStartIndex = nodeInSource.indexOf(blockType); - - const idParamIndex = idValueMatch.index + contentForBlockStartIndex; - const idParamValueLength = entireIdTerm.length; + const idValueNode = idNode.value; + if (idValueNode.type !== NodeTypes.String) { + return; // covered by VariableContentForArguments + } - const idParamValueEndIndex = idParamIndex + idParamValueLength; + const id = idValueNode.value; + if (usedIds.has(id)) { context.report({ - message: `The id '${filteredIdValue}' is already being used by another static block`, - startIndex: node.position.start + idParamIndex, - endIndex: node.position.start + idParamValueEndIndex, + message: `The id '${id}' is already being used by another static block`, + startIndex: idValueNode.position.start, + endIndex: idValueNode.position.end, suggest: [], }); } else { - usedIds.add(filteredIdValue); + usedIds.add(id); } }, }; diff --git a/packages/theme-check-common/src/checks/valid-content-for-arguments/index.spec.ts b/packages/theme-check-common/src/checks/valid-content-for-arguments/index.spec.ts new file mode 100644 index 000000000..950578892 --- /dev/null +++ b/packages/theme-check-common/src/checks/valid-content-for-arguments/index.spec.ts @@ -0,0 +1,75 @@ +import { describe, expect, it } from 'vitest'; +import { ValidContentForArguments } from '.'; +import { runLiquidCheck } from '../../test'; + +describe('Module: ValidContentForArguments', () => { + describe('{% content_for "unknown" %}', () => { + it('should not report offenses for strategies we do not know or support yet', async () => { + const offenses = await runLiquidCheck( + ValidContentForArguments, + '{% content_for "snippet", context.product: product %}', + ); + expect(offenses).toHaveLength(0); + }); + }); + + describe('{% content_for "blocks" %}', () => { + it('should accept `context.*` kwargs', async () => { + const offenses = await runLiquidCheck( + ValidContentForArguments, + '{% content_for "blocks", context.product: product %}', + ); + expect(offenses).toHaveLength(0); + }); + + it('should report offenses for non-`context.*` kwargs', async () => { + const offenses = await runLiquidCheck( + ValidContentForArguments, + '{% content_for "blocks", product: product %}', + ); + expect(offenses).toHaveLength(1); + expect(offenses[0]!.message).to.equal( + `{% content_for "blocks" %} only accepts 'context.*' arguments`, + ); + }); + }); + + describe('{% content_for "block", type: "", id: "" %}', () => { + it('should accept valid arguments', async () => { + const offenses = await runLiquidCheck( + ValidContentForArguments, + '{% content_for "block", type: "text", id: "static-block" %}', + ); + expect(offenses).toHaveLength(0); + }); + + it(`should report an offense if 'type' is not a string`, async () => { + const offenses = await runLiquidCheck( + ValidContentForArguments, + '{% content_for "block", type: 10, id: "static-block" %}', + ); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).to.equal(`The 'type' argument should be a string`); + }); + + it(`should report an offense if 'id' is not a string`, async () => { + const offenses = await runLiquidCheck( + ValidContentForArguments, + '{% content_for "block", type: "foo", id: (0..10) %}', + ); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).to.equal(`The 'id' argument should be a string`); + }); + + it(`should report an offense if other kwargs are passed that are not named 'context.*'`, async () => { + const offenses = await runLiquidCheck( + ValidContentForArguments, + '{% content_for "block", type: "foo", id: "id", product: product %}', + ); + expect(offenses).toHaveLength(1); + expect(offenses[0].message).to.equal( + `{% content_for "block" %} only accepts 'id', 'type' and 'context.*' arguments`, + ); + }); + }); +}); diff --git a/packages/theme-check-common/src/checks/valid-content-for-arguments/index.ts b/packages/theme-check-common/src/checks/valid-content-for-arguments/index.ts new file mode 100644 index 000000000..0491ecd97 --- /dev/null +++ b/packages/theme-check-common/src/checks/valid-content-for-arguments/index.ts @@ -0,0 +1,95 @@ +import { ContentForMarkup, NodeTypes } from '@shopify/liquid-html-parser'; +import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; + +// content_for "block" and content_for "blocks" only allow `context.*` kwargs. +const isContextArgument = (argName: string) => argName.startsWith('context.'); + +export const ValidContentForArguments: LiquidCheckDefinition = { + meta: { + code: 'ValidContentForArguments', + name: 'Prevent the use of invalid arguments to the content_for tag', + docs: { + description: + 'This check is aimed at preventing the use of invalid arguments for the content_for tag.', + url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/valid-content-for-arguments', + recommended: true, + }, + type: SourceCodeType.LiquidHtml, + severity: Severity.ERROR, + schema: {}, + targets: [], + }, + + create(context) { + const validationStrategies = { + blocks: (node: ContentForMarkup) => { + const problematicArguments = node.args.filter((arg) => !isContextArgument(arg.name)); + for (const arg of problematicArguments) { + context.report({ + message: `{% content_for "blocks" %} only accepts 'context.*' arguments`, + startIndex: arg.position.start, + endIndex: arg.position.end, + }); + } + }, + + block: (node: ContentForMarkup) => { + const requiredArguments = ['id', 'type']; + + // Make sure the id and string arguments are present and are strings + for (const requiredArgumentName of requiredArguments) { + const arg = node.args.find((arg) => arg.name === requiredArgumentName); + + if (!arg) { + context.report({ + message: `{% content_for "block" %} requires a '${requiredArgumentName}' argument`, + startIndex: node.position.start, + endIndex: node.position.end, + suggest: [], + }); + continue; + } + + const argValueNode = arg.value; + if (argValueNode.type !== NodeTypes.String) { + context.report({ + message: `The '${requiredArgumentName}' argument should be a string`, + startIndex: argValueNode.position.start, + endIndex: argValueNode.position.end, + suggest: [], + }); + } + } + + const problematicArguments = node.args.filter( + (arg) => !(requiredArguments.includes(arg.name) || isContextArgument(arg.name)), + ); + + for (const arg of problematicArguments) { + context.report({ + message: `{% content_for "block" %} only accepts 'id', 'type' and 'context.*' arguments`, + startIndex: arg.position.start, + endIndex: arg.position.end, + }); + } + }, + }; + + return { + async LiquidTag(node) { + if (node.name !== 'content_for' || typeof node.markup === 'string') { + return; + } + + /** "block", "blocks", etc. */ + const contentForType = node.markup.contentForType.value; + const validate = validationStrategies[contentForType as keyof typeof validationStrategies]; + if (!validate) { + return; + } + + validate(node.markup); + }, + }; + }, +}; diff --git a/packages/theme-check-common/src/checks/valid-static-block-type/index.spec.ts b/packages/theme-check-common/src/checks/valid-static-block-type/index.spec.ts index 1e2cc0df6..b7cfd03a6 100644 --- a/packages/theme-check-common/src/checks/valid-static-block-type/index.spec.ts +++ b/packages/theme-check-common/src/checks/valid-static-block-type/index.spec.ts @@ -1,6 +1,6 @@ -import { expect, describe, it } from 'vitest'; -import { check, MockTheme, runLiquidCheck } from '../../test'; -import { SchemaProp } from '../../types'; +import { describe, expect, it } from 'vitest'; +import { check, MockTheme } from '../../test'; +import { ValidStaticBlockType } from '.'; describe('Module: ValidStaticBlockType', () => { const genericThemeBlock = { @@ -30,11 +30,9 @@ describe('Module: ValidStaticBlockType', () => { expect(offenses).toHaveLength(0); }); - it('should report a offense if type is invalid', async () => { + it('should report an offense if type is invalid', async () => { const offenses = await check(invalidExtensionFiles); expect(offenses).toHaveLength(1); - expect(offenses[0].message).to.equal( - "The type 'invalid' is not valid, use a type that exists in the blocks directory", - ); + expect(offenses[0].message).to.equal("'blocks/invalid.liquid' does not exist"); }); }); diff --git a/packages/theme-check-common/src/checks/valid-static-block-type/index.ts b/packages/theme-check-common/src/checks/valid-static-block-type/index.ts index 80033e9a5..a595d2d1a 100644 --- a/packages/theme-check-common/src/checks/valid-static-block-type/index.ts +++ b/packages/theme-check-common/src/checks/valid-static-block-type/index.ts @@ -1,3 +1,4 @@ +import { NodeTypes } from '@shopify/liquid-html-parser'; import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types'; import { doesFileExist } from '../../utils/file-utils'; import { isContentForBlock } from '../../utils/markup'; @@ -19,8 +20,6 @@ export const ValidStaticBlockType: LiquidCheckDefinition = { }, create(context) { - const typeRegex = /type:\s*["'](\S+)["']/; - return { async LiquidTag(node) { if (node.name !== 'content_for') { @@ -31,30 +30,25 @@ export const ValidStaticBlockType: LiquidCheckDefinition = { return; } - const typeValueMatch = typeRegex.exec(node.markup); + const typeArg = node.markup.args.find((arg) => arg.name === 'type'); + if (!typeArg) { + return; // covered by VariableContentForArguments + } - if (typeValueMatch == null) { - return; + const typeArgValueNode = typeArg.value; + if (typeArgValueNode.type !== NodeTypes.String) { + return; // covered by VariableContentForArguments } - const [entireTypeTerm, filteredTypeValue] = typeValueMatch; - const relativePath = `blocks/${filteredTypeValue}.liquid`; + const blockName = typeArgValueNode.value; + const relativePath = `blocks/${blockName}.liquid`; const fileExists = await doesFileExist(context, relativePath); if (!fileExists) { - const [blockType] = node.markup.split(','); - const nodeInSource = node.source.substring(node.position.start); - const contentForBlockStartIndex = nodeInSource.indexOf(blockType); - - const typeParamIndex = typeValueMatch.index + contentForBlockStartIndex; - const typeParamValueLength = entireTypeTerm.length; - - const typeParamValueEndIndex = typeParamIndex + typeParamValueLength; - context.report({ - message: `The type '${filteredTypeValue}' is not valid, use a type that exists in the blocks directory`, - startIndex: node.position.start + typeParamIndex, - endIndex: node.position.start + typeParamValueEndIndex, + message: `'blocks/${blockName}.liquid' does not exist`, + startIndex: typeArgValueNode.position.start, + endIndex: typeArgValueNode.position.end, suggest: [], }); } diff --git a/packages/theme-check-common/src/test/MockFileSystem.ts b/packages/theme-check-common/src/test/MockFileSystem.ts index d9acd43af..72fc464e2 100644 --- a/packages/theme-check-common/src/test/MockFileSystem.ts +++ b/packages/theme-check-common/src/test/MockFileSystem.ts @@ -30,7 +30,7 @@ export class MockFileSystem implements AbstractFileSystem { ? this.fileTree : deepGet(this.fileTree, relativePath.split('/')); if (tree === undefined) { - throw new Error('Directory not found'); + throw new Error(`Directory not found: ${uri}`); } if (typeof tree === 'string') { diff --git a/packages/theme-check-common/src/utils/markup.ts b/packages/theme-check-common/src/utils/markup.ts index c20a97803..bf0e93570 100644 --- a/packages/theme-check-common/src/utils/markup.ts +++ b/packages/theme-check-common/src/utils/markup.ts @@ -1,7 +1,10 @@ -export function isContentForBlock(nodeMarkup: string) { - const [blockType] = nodeMarkup.split(','); - if (blockType.replace(/["']/g, '') !== 'block') { +import { ContentForMarkup } from '@shopify/liquid-html-parser'; + +export function isContentForBlock( + nodeMarkup: string | ContentForMarkup, +): nodeMarkup is ContentForMarkup { + if (typeof nodeMarkup === 'string') { return false; } - return true; + return nodeMarkup.contentForType.value === 'block'; } diff --git a/packages/theme-check-node/configs/all.yml b/packages/theme-check-node/configs/all.yml index 338c311b9..ba50f08f9 100644 --- a/packages/theme-check-node/configs/all.yml +++ b/packages/theme-check-node/configs/all.yml @@ -97,6 +97,9 @@ UnknownFilter: UnusedAssign: enabled: true severity: 1 +ValidContentForArguments: + enabled: true + severity: 0 ValidHTMLTranslation: enabled: true severity: 1 diff --git a/packages/theme-check-node/configs/recommended.yml b/packages/theme-check-node/configs/recommended.yml index 7cc549033..9684c6d08 100644 --- a/packages/theme-check-node/configs/recommended.yml +++ b/packages/theme-check-node/configs/recommended.yml @@ -78,6 +78,9 @@ UnknownFilter: UnusedAssign: enabled: true severity: 1 +ValidContentForArguments: + enabled: true + severity: 0 ValidHTMLTranslation: enabled: true severity: 1 diff --git a/packages/theme-language-server-common/src/completions/params/LiquidCompletionParams.spec.ts b/packages/theme-language-server-common/src/completions/params/LiquidCompletionParams.spec.ts index 0ef50c42c..5423accc4 100644 --- a/packages/theme-language-server-common/src/completions/params/LiquidCompletionParams.spec.ts +++ b/packages/theme-language-server-common/src/completions/params/LiquidCompletionParams.spec.ts @@ -128,6 +128,20 @@ describe('Module: LiquidCompletionParams', async () => { } }); + it(`returns a String node when you're in the middle of it`, async () => { + const contexts = [ + `{% render '█' %}`, + `{% render 'snip', var: '█' %}`, + `{% content_for '█' %}`, + `{% content_for 'block', id: '█' %}`, + ]; + for (const context of contexts) { + const { completionContext } = createLiquidParamsFromContext(context); + const { node } = completionContext!; + expectPath(node, 'type', context).to.eql('String'); + } + }); + it('returns a tag', async () => { const contexts = [ `{% t█ %}`, @@ -172,6 +186,7 @@ describe('Module: LiquidCompletionParams', async () => { `{% cycle a█ %}`, `{% cycle 'foo', a█ %}`, `{% cycle 'foo': a█ %}`, + `{% content_for 'foo', v: █ %}`, `{% render 'snip', var: a█ %}`, `{% render 'snip' for col█ as item %}`, `{% render 'snip' with object█ as name %}`, @@ -224,6 +239,7 @@ describe('Module: LiquidCompletionParams', async () => { `{% cycle █ %}`, `{% cycle 'foo', █ %}`, `{% cycle 'foo': █ %}`, + `{% content_for 'snip', var: █ %}`, `{% render 'snip', var: █ %}`, `{% render 'snip' for █ as item %}`, `{% render 'snip' with █ as name %}`, diff --git a/packages/theme-language-server-common/src/completions/params/LiquidCompletionParams.ts b/packages/theme-language-server-common/src/completions/params/LiquidCompletionParams.ts index 53cce3311..75d6e1e75 100644 --- a/packages/theme-language-server-common/src/completions/params/LiquidCompletionParams.ts +++ b/packages/theme-language-server-common/src/completions/params/LiquidCompletionParams.ts @@ -363,11 +363,23 @@ function findCurrentNode( break; } + case NodeTypes.ContentForMarkup: { + if (isNotEmpty(current.args)) { + finder.current = last(current.args); + } else if (isCovered(cursor, current.contentForType.position)) { + finder.current = current.contentForType; + } + + break; + } + case NodeTypes.RenderMarkup: { if (isNotEmpty(current.args)) { finder.current = last(current.args); } else if (current.variable && isCovered(cursor, current.variable.position)) { finder.current = current.variable; + } else if (current.snippet && isCovered(cursor, current.snippet.position)) { + finder.current = current.snippet; } break; diff --git a/packages/theme-language-server-common/src/completions/providers/RenderSnippetCompletionProvider.ts b/packages/theme-language-server-common/src/completions/providers/RenderSnippetCompletionProvider.ts index e6a928d1e..828143c98 100644 --- a/packages/theme-language-server-common/src/completions/providers/RenderSnippetCompletionProvider.ts +++ b/packages/theme-language-server-common/src/completions/providers/RenderSnippetCompletionProvider.ts @@ -11,14 +11,20 @@ export class RenderSnippetCompletionProvider implements Provider { async completions(params: LiquidCompletionParams): Promise { if (!params.completionContext) return []; - const { node } = params.completionContext; + const { node, ancestors } = params.completionContext; + const parentNode = ancestors.at(-1); - if (!node || node.type !== NodeTypes.RenderMarkup || node.snippet.type !== NodeTypes.String) { + if ( + !node || + !parentNode || + node.type !== NodeTypes.String || + parentNode.type !== NodeTypes.RenderMarkup + ) { return []; } const options = await this.getSnippetNamesForURI(params.textDocument.uri); - const partial = node.snippet.value; + const partial = node.value; return options .filter((option) => option.startsWith(partial))