Skip to content

Commit ed3c437

Browse files
authored
[Theme Check] Add check for duplicate settings id's (#696)
Closes Shopify/developer-tools-team#263 Implements a new validation check to prevent duplicate setting IDs across theme configurations. This helps theme developers identify and fix duplicate IDs early in development, preventing potential conflicts and overrides. The implementation includes: - New JSONCheckDefinition for unique setting ID validation - Test coverage for valid and invalid theme configurations - Detection logic for finding duplicate IDs in settings schema See: Ticket: 49020903 Shopify/cli#4187 ## What are you adding in this PR? This PR adds a new theme check validator that detects duplicate setting IDs in theme configurations. The validator scans settings_schema.json files and reports an error when it finds multiple settings using the same ID value, preventing potential runtime conflicts. Key changes: - New UniqueSettingIds check that validates setting ID uniqueness - Integration with the existing theme checker infrastructure - Test coverage for both valid and invalid scenarios - Error reporting with precise location information ## What's next? Any followup issues? Potential follow-ups: - Consider extending the check to validate uniqueness across sections - Add documentation for theme developers about ID uniqueness requirements - Consider adding quick-fix suggestions for duplicate IDs ## What did you learn? Working with AST traversal for JSON validation showed that we need a consistent approach to handle nested property validation. The existing node type checks provided a good pattern to follow. ## Before you deploy <!-- Delete the checklists you don't need --> <!-- Check changes --> - [x] This PR includes a new checks or changes the configuration of a check - [x] I included a minor bump `changeset` - [x] It's in the `allChecks` array in `src/checks/index.ts` - [x] I ran `yarn build` and committed the updated configuration files
2 parents fced038 + 913d538 commit ed3c437

File tree

8 files changed

+1316
-0
lines changed

8 files changed

+1316
-0
lines changed

.changeset/late-pears-check.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@shopify/theme-check-common': minor
3+
---
4+
5+
Add unique settings ID validator to prevent duplicate IDs in theme configurations

packages/theme-check-common/src/checks/index.ts

+2
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ import { ValidSchemaName } from './valid-schema-name';
4545
import { ValidStaticBlockType } from './valid-static-block-type';
4646
import { VariableName } from './variable-name';
4747
import { AppBlockMissingSchema } from './app-block-missing-schema';
48+
import { UniqueSettingIds } from './unique-settings-id';
4849

4950
export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [
5051
AppBlockValidTags,
@@ -80,6 +81,7 @@ export const allChecks: (LiquidCheckDefinition | JSONCheckDefinition)[] = [
8081
TranslationKeyExists,
8182
UnclosedHTMLElement,
8283
UndefinedObject,
84+
UniqueSettingIds,
8385
UniqueStaticBlockId,
8486
UnknownFilter,
8587
UnusedAssign,
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { expect, describe, it } from 'vitest';
2+
import { UniqueSettingIds } from './index';
3+
import { highlightedOffenses, runJSONCheck } from '../../test';
4+
import { invalidJson, validJson } from './test-data';
5+
6+
describe('Module: UniqueSettingIds', () => {
7+
it("Should report an error for duplicate id's in settings_schema (0)", async () => {
8+
const offenses = await runJSONCheck(UniqueSettingIds, invalidJson, 'file.json');
9+
10+
expect(offenses).to.have.length(1);
11+
expect(offenses[0].message).to.equal('Duplicate setting id found: "nosto_account_id"');
12+
13+
const highlights = highlightedOffenses({ 'file.json': invalidJson }, offenses);
14+
15+
expect(highlights).to.have.length(1);
16+
expect(highlights[0]).to.equal('"id": "nosto_account_id"');
17+
});
18+
19+
it('should not report any errors for valid file', async () => {
20+
const offenses = await runJSONCheck(UniqueSettingIds, validJson);
21+
22+
expect(offenses).to.be.empty;
23+
});
24+
});
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
import {
2+
isArrayNode,
3+
isLiteralNode,
4+
isObjectNode,
5+
isPropertyNode,
6+
Severity,
7+
SourceCodeType,
8+
} from '../../types';
9+
10+
import type { ArrayNode, PropertyNode, JSONCheckDefinition } from '../../types';
11+
12+
export const UniqueSettingIds: JSONCheckDefinition = {
13+
meta: {
14+
code: 'UniqueSettingId',
15+
name: 'Prevent duplicate Ids in setting_schema',
16+
docs: {
17+
description: 'This check is aimed at eliminating duplicate Ids in settings_schema.json',
18+
recommended: true,
19+
// url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/valid-schema',
20+
},
21+
type: SourceCodeType.JSON,
22+
severity: Severity.ERROR,
23+
schema: {},
24+
targets: [],
25+
},
26+
27+
create(context) {
28+
return {
29+
async onCodePathEnd(file) {
30+
if (isArrayNode(file.ast)) {
31+
const settingIds: PropertyNode[] = [];
32+
33+
/* Find and loop through all of our nodes that have an id value and find their key value */
34+
for (const child of file.ast.children) {
35+
if (isObjectNode(child) && child.children) {
36+
const settingsNode = child.children.find((node) => node.key.value === 'settings');
37+
38+
if (settingsNode && settingsNode.value && isArrayNode(settingsNode.value)) {
39+
for (const setting of settingsNode.value.children) {
40+
if (isObjectNode(setting) && setting.children) {
41+
const idNode = setting.children.find((node) => node.key.value === 'id');
42+
if (isPropertyNode(idNode)) {
43+
settingIds.push(idNode);
44+
}
45+
}
46+
}
47+
}
48+
}
49+
}
50+
51+
/* Check for dupes */
52+
const idMap = new Map<string, PropertyNode[]>();
53+
for (const node of settingIds) {
54+
if (isLiteralNode(node.value)) {
55+
const id = node.value.value;
56+
if (typeof id === 'string') {
57+
if (!idMap.has(id)) {
58+
idMap.set(id, []);
59+
}
60+
idMap.get(id)!.push(node);
61+
}
62+
}
63+
}
64+
65+
const duplicates: [string, PropertyNode[]][] = Array.from(idMap.entries()).filter(
66+
([_, nodes]) => nodes.length > 1,
67+
);
68+
69+
if (duplicates.length > 0) {
70+
for (const [id, nodes] of duplicates) {
71+
const lastNodeFound = nodes[nodes.length - 1];
72+
73+
context.report({
74+
message: `Duplicate setting id found: "${id}"`,
75+
startIndex: lastNodeFound.loc.start.offset,
76+
endIndex: lastNodeFound.loc.end.offset,
77+
});
78+
}
79+
}
80+
}
81+
},
82+
};
83+
},
84+
};

0 commit comments

Comments
 (0)