-
Notifications
You must be signed in to change notification settings - Fork 30
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add duplicate settings ID detection for theme checker
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
- Loading branch information
Zacarie Carr
authored and
Zacarie Carr
committed
Jan 10, 2025
1 parent
dc9c6da
commit 8a08b83
Showing
4 changed files
with
1,303 additions
and
0 deletions.
There are no files selected for viewing
24 changes: 24 additions & 0 deletions
24
packages/theme-check-common/src/checks/unique-settings-id/index.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { expect, describe, it } from 'vitest'; | ||
import { UniqueSettingIds } from './index'; | ||
import { highlightedOffenses, runJSONCheck } from '../../test'; | ||
import { invalidJson, validJson } from './test-data'; | ||
|
||
describe('Module: UniqueSettingIds', () => { | ||
it("Should report an error for duplicate id's in settings_schema (0)", async () => { | ||
const offenses = await runJSONCheck(UniqueSettingIds, invalidJson, 'file.json'); | ||
|
||
expect(offenses).to.have.length(1); | ||
expect(offenses[0].message).to.equal('Duplicate setting id found: "nosto_account_id"'); | ||
|
||
const highlights = highlightedOffenses({ 'file.json': invalidJson }, offenses); | ||
|
||
expect(highlights).to.have.length(1); | ||
expect(highlights[0]).to.equal('"id": "nosto_account_id"'); | ||
}); | ||
|
||
it('should not report any errors for valid file', async () => { | ||
const offenses = await runJSONCheck(UniqueSettingIds, validJson); | ||
|
||
expect(offenses).to.be.empty; | ||
}); | ||
}); |
84 changes: 84 additions & 0 deletions
84
packages/theme-check-common/src/checks/unique-settings-id/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
import { | ||
isArrayNode, | ||
isLiteralNode, | ||
isObjectNode, | ||
isPropertyNode, | ||
Severity, | ||
SourceCodeType, | ||
} from '../../types'; | ||
|
||
import type { ArrayNode, PropertyNode, JSONCheckDefinition } from '../../types'; | ||
|
||
export const UniqueSettingIds: JSONCheckDefinition = { | ||
meta: { | ||
code: 'UniqueSettingId', | ||
name: 'Prevent duplicate Ids in setting_schema', | ||
docs: { | ||
description: 'This check is aimed at eliminating duplicate Ids in settings_schema.json', | ||
recommended: true, | ||
// url: 'https://shopify.dev/docs/storefronts/themes/tools/theme-check/checks/valid-schema', | ||
}, | ||
type: SourceCodeType.JSON, | ||
severity: Severity.ERROR, | ||
schema: {}, | ||
targets: [], | ||
}, | ||
|
||
create(context) { | ||
return { | ||
async onCodePathEnd(file) { | ||
if (isArrayNode(file.ast)) { | ||
const settingIds: PropertyNode[] = []; | ||
|
||
/* Find and loop through all of our nodes that have an id value and find their key value */ | ||
for (const child of file.ast.children) { | ||
if (isObjectNode(child) && child.children) { | ||
const settingsNode = child.children.find((node) => node.key.value === 'settings'); | ||
|
||
if (settingsNode && settingsNode.value && isArrayNode(settingsNode.value)) { | ||
for (const setting of settingsNode.value.children) { | ||
if (isObjectNode(setting) && setting.children) { | ||
const idNode = setting.children.find((node) => node.key.value === 'id'); | ||
if (isPropertyNode(idNode)) { | ||
settingIds.push(idNode); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
/* Check for dupes */ | ||
const idMap = new Map<string, PropertyNode[]>(); | ||
for (const node of settingIds) { | ||
if (isLiteralNode(node.value)) { | ||
const id = node.value.value; | ||
if (typeof id === 'string') { | ||
if (!idMap.has(id)) { | ||
idMap.set(id, []); | ||
} | ||
idMap.get(id)!.push(node); | ||
} | ||
} | ||
} | ||
|
||
const duplicates: [string, PropertyNode[]][] = Array.from(idMap.entries()).filter( | ||
([_, nodes]) => nodes.length > 1, | ||
); | ||
|
||
if (duplicates.length > 0) { | ||
for (const [id, nodes] of duplicates) { | ||
const lastNodeFound = nodes[nodes.length - 1]; | ||
|
||
context.report({ | ||
message: `Duplicate setting id found: "${id}"`, | ||
startIndex: lastNodeFound.loc.start.offset, | ||
endIndex: lastNodeFound.loc.end.offset, | ||
}); | ||
} | ||
} | ||
} | ||
}, | ||
}; | ||
}, | ||
}; |
Oops, something went wrong.