Skip to content

Commit c991874

Browse files
graygilmoregraygilmore
authored
Merge pull request #1162 from Shopify/gg-unique-static-block-id-variant-awareness
Support arbitrary params in UniqueStaticBlockId duplicate detection
2 parents 6f0dcae + e10da7c commit c991874

3 files changed

Lines changed: 121 additions & 7 deletions

File tree

‎.changeset/brave-dogs.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme-check-common': patch
3+
---
4+
5+
Fix UniqueStaticBlockId false positive when same block id is used with different variants

‎packages/theme-check-common/src/checks/unique-static-block-id/index.spec.ts‎

Lines changed: 88 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ describe('Module: UniqueStaticBlockId', () => {
1717
);
1818
});
1919

20-
it('should not report an error when two static blocks have differnt id', async () => {
20+
it('should not report an error when two static blocks have different id', async () => {
2121
const sourceCode = `
2222
{% content_for "block", type: "text", id: "static-block1" %}
2323
{% content_for "block", type: "text", id: "static-block2" %}
@@ -52,4 +52,91 @@ describe('Module: UniqueStaticBlockId', () => {
5252

5353
expect(offenses).to.have.length(0);
5454
});
55+
56+
it('should not report an error when blocks share the same id but have different arbitrary params', async () => {
57+
const sourceCode = `
58+
{% content_for "block", type: "text", id: "header-menu", variant: "mobile" %}
59+
{% content_for "block", type: "text", id: "header-menu", variant: "navigation_bar" %}
60+
{% content_for "block", type: "text", id: "header-menu" %}
61+
`;
62+
63+
const offenses = await runLiquidCheck(UniqueStaticBlockId, sourceCode);
64+
65+
expect(offenses).to.have.length(0);
66+
});
67+
68+
it('should report an error when blocks share the same id and same arbitrary params', async () => {
69+
const sourceCode = `
70+
{% content_for "block", type: "text", id: "header-menu", variant: "mobile" %}
71+
{% content_for "block", type: "text", id: "header-menu", variant: "mobile" %}
72+
`;
73+
74+
const offenses = await runLiquidCheck(UniqueStaticBlockId, sourceCode);
75+
76+
expect(offenses).to.have.length(1);
77+
expect(offenses[0].message).to.equal(
78+
"The id 'header-menu' is already being used by another static block",
79+
);
80+
});
81+
82+
it('should not report an error when blocks share the same id but have different values for the same param', async () => {
83+
const sourceCode = `
84+
{% content_for "block", type: "text", id: "sidebar", color: "blue" %}
85+
{% content_for "block", type: "text", id: "sidebar", color: "red" %}
86+
`;
87+
88+
const offenses = await runLiquidCheck(UniqueStaticBlockId, sourceCode);
89+
90+
expect(offenses).to.have.length(0);
91+
});
92+
93+
it('should report an error when blocks share the same id and multiple matching params', async () => {
94+
const sourceCode = `
95+
{% content_for "block", type: "text", id: "sidebar", color: "blue", size: "large" %}
96+
{% content_for "block", type: "text", id: "sidebar", color: "blue", size: "large" %}
97+
`;
98+
99+
const offenses = await runLiquidCheck(UniqueStaticBlockId, sourceCode);
100+
101+
expect(offenses).to.have.length(1);
102+
expect(offenses[0].message).to.equal(
103+
"The id 'sidebar' is already being used by another static block",
104+
);
105+
});
106+
107+
it('should report an error when blocks have same params in different order', async () => {
108+
const sourceCode = `
109+
{% content_for "block", type: "text", id: "sidebar", color: "blue", size: "large" %}
110+
{% content_for "block", type: "text", id: "sidebar", size: "large", color: "blue" %}
111+
`;
112+
113+
const offenses = await runLiquidCheck(UniqueStaticBlockId, sourceCode);
114+
115+
expect(offenses).to.have.length(1);
116+
expect(offenses[0].message).to.equal(
117+
"The id 'sidebar' is already being used by another static block",
118+
);
119+
});
120+
121+
it('should not report an error when any arbitrary param is a variable lookup', async () => {
122+
const sourceCode = `
123+
{% content_for "block", type: "text", id: "header-menu", variant: "mobile" %}
124+
{% content_for "block", type: "text", id: "header-menu", variant: some_variable %}
125+
`;
126+
127+
const offenses = await runLiquidCheck(UniqueStaticBlockId, sourceCode);
128+
129+
expect(offenses).to.have.length(0);
130+
});
131+
132+
it('should not report an error when two blocks use the same variable lookup', async () => {
133+
const sourceCode = `
134+
{% content_for "block", type: "text", id: "header-menu", variant: some_variable %}
135+
{% content_for "block", type: "text", id: "header-menu", variant: some_variable %}
136+
`;
137+
138+
const offenses = await runLiquidCheck(UniqueStaticBlockId, sourceCode);
139+
140+
expect(offenses).to.have.length(0);
141+
});
55142
});

‎packages/theme-check-common/src/checks/unique-static-block-id/index.ts‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,16 @@
11
import { NamedTags, NodeTypes } from '@shopify/liquid-html-parser';
22
import { LiquidCheckDefinition, Severity, SourceCodeType } from '../../types';
3+
import {
4+
REQUIRED_CONTENT_FOR_ARGUMENTS,
5+
RESERVED_CONTENT_FOR_ARGUMENTS,
6+
} from '../../tags/content-for';
37
import { isContentForBlock } from '../../utils/markup';
48

9+
const FRAMEWORK_ARGS = new Set([
10+
...REQUIRED_CONTENT_FOR_ARGUMENTS,
11+
...RESERVED_CONTENT_FOR_ARGUMENTS,
12+
]);
13+
514
export const UniqueStaticBlockId: LiquidCheckDefinition = {
615
meta: {
716
code: 'UniqueStaticBlockId',
@@ -19,8 +28,7 @@ export const UniqueStaticBlockId: LiquidCheckDefinition = {
1928
},
2029

2130
create(context) {
22-
const usedIds: Set<string> = new Set();
23-
const idRegex = /id:\s*["'](\S+)["']/;
31+
const usedCompositeKeys: Set<string> = new Set();
2432
return {
2533
async LiquidTag(node) {
2634
if (node.name !== NamedTags.content_for) {
@@ -42,17 +50,31 @@ export const UniqueStaticBlockId: LiquidCheckDefinition = {
4250
return; // covered by VariableContentForArguments
4351
}
4452

45-
const id = idValueNode.value;
53+
const idValue = idValueNode.value;
54+
55+
const arbitraryArgs = node.markup.args.filter((arg) => !FRAMEWORK_ARGS.has(arg.name));
56+
57+
const argParts: string[] = [];
58+
for (const arg of arbitraryArgs) {
59+
if (arg.value.type !== NodeTypes.String) {
60+
return;
61+
}
62+
argParts.push(`${arg.name}=${arg.value.value}`);
63+
}
64+
argParts.sort();
65+
const argsSuffix = argParts.join(',');
66+
67+
const compositeKey = `${idValue}::${argsSuffix}`;
4668

47-
if (usedIds.has(id)) {
69+
if (usedCompositeKeys.has(compositeKey)) {
4870
context.report({
49-
message: `The id '${id}' is already being used by another static block`,
71+
message: `The id '${idValue}' is already being used by another static block`,
5072
startIndex: idValueNode.position.start,
5173
endIndex: idValueNode.position.end,
5274
suggest: [],
5375
});
5476
} else {
55-
usedIds.add(id);
77+
usedCompositeKeys.add(compositeKey);
5678
}
5779
},
5880
};

0 commit comments

Comments
 (0)