Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Theme Check] Add check for duplicate settings id's #696

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zacariec
Copy link

@zacariec zacariec commented Jan 10, 2025

Closes https://github.com/Shopify/develop-advanced-edits/issues/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

  • This PR includes a new checks or changes the configuration of a check
  • I included a minor bump changeset
  • It's in the allChecks array in src/checks/index.ts
  • I ran yarn build and committed the updated configuration files

@zacariec zacariec requested a review from a team as a code owner January 10, 2025 00:38
@zacariec zacariec changed the title Issues/263 [Theme Check] Add check for duplicate settings id's Jan 10, 2025
Copy link
Contributor

@aswamy aswamy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Code looking good
✅ Tophatting went well

Comment on lines +42 to +44
if (isPropertyNode(idNode)) {
settingIds.push(idNode);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also move most of the "Check for dupes" code here instead of using a "settingIds" variable.

I guess you didn't want to make the code here too nested and dirty 😅

Suggested change
if (isPropertyNode(idNode)) {
settingIds.push(idNode);
}
if (isPropertyNode(idNode) && isLiteralNode(idNode.value)) {
const id = idNode.value.value;
if (typeof id === 'string') {
if (!idMap.has(id)) {
idMap.set(id, []);
}
idMap.get(id)!.push(idNode);
}
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aswamy yep spot on, not a fan of nests and rather use guard clauses but due to the nature of this particular paradigm guard clauses don't fit. Didn't see a style guide from Shopify anywhere either.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants