Skip to content

Remove swatches input#425

Merged
hta218 merged 9 commits into
mainfrom
dev
Jan 6, 2026
Merged

Remove swatches input#425
hta218 merged 9 commits into
mainfrom
dev

Conversation

@hta218
Copy link
Copy Markdown
Member

@hta218 hta218 commented Jan 6, 2026

No description provided.

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Jan 6, 2026

⚠️ No Changeset found

Latest commit: 2bdfa96

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@hta218 hta218 changed the title Remove 'swatches' input Remove swatches input Jan 6, 2026
@claude
Copy link
Copy Markdown

claude Bot commented Jan 6, 2026

📋 Code Review: Remove swatches input

Summary

This PR removes the unused swatches input type from the Weaverse SDK type system. The changes are minimal and focused on cleaning up deprecated code.

✅ Strengths

  1. Minimal & Focused Changes - Only touches necessary files:

    • packages/core/src/types.ts: Removed swatches from InputType union
    • packages/schema/src/index.ts: Removed swatches literal from Zod schema
    • Properly updates all package versions and CHANGELOGs
  2. Consistent Versioning - All fixed packages (core, react, hydrogen) are synchronized to version 5.8.8, following the project's version policy correctly.

  3. Complete Documentation - CHANGELOGs are properly updated across all affected packages.

🔍 Areas for Consideration

1. Changeset Configuration Change

-  ignore: [@weaverse/pilot]
+  ignore: []

This removes the pilot template from the changeset ignore list. This is a significant configuration change that should be intentional:

  • If intentional: Ensure you want pilot to be included in versioning going forward
  • If unintentional: This should be reverted to maintain existing behavior

2. Shopify Package Build Artifacts
The PR includes new build artifacts in packages/shopify/dist/ (~14k lines). According to CLAUDE.md:

Note: @weaverse/shopify has been archived and is no longer maintained.

Questions:

  • Should the shopify package be rebuilt as part of this change?
  • If shopify is archived, should its build artifacts be in this PR?
  • The shopify package code still contains swatches references - is this intentional given the archived status?

3. Breaking Change Considerations
While this removes an unused input type, it's worth verifying:

  • Good: Type system change removes swatches from allowed types
  • ⚠️ Consider: Are there any existing implementations using swatches? If yes, this would be a breaking change for them
  • 📝 Recommendation: If the input type was never used or publicly documented, consider adding a note in the CHANGELOG confirming this

4. Schema Package Version
The schema package is independently versioned and moves from 0.7.3 → 0.7.4. This is correct per project conventions.

🎯 Recommendations

  1. Review Changeset Config: Verify if removing @weaverse/pilot from ignore list is intentional
  2. Shopify Package:
    • If shopify should be rebuilt: Consider whether swatches references should also be removed
    • If shopify is truly archived: Consider excluding it from this PR entirely
  3. Add Migration Note: Briefly mention in CHANGELOG that swatches was experimental/unused (if accurate)
  4. Consider Deprecation Path: If swatches might be in use anywhere, consider a deprecation cycle before removal

🏆 Overall Assessment

Code Quality: ✅ Good

  • Clean, minimal changes
  • Proper version synchronization
  • Good CHANGELOG updates

Best Practices: ✅ Follows conventions

  • Consistent with project structure
  • Maintains type safety

Risk Level: 🟢 Low

  • Straightforward type removal
  • No runtime logic changes

Approval Recommendation: ✅ Approve with minor suggestions

This is a well-executed cleanup PR. The main considerations are around the changeset config change and whether the shopify package should be included in this change.

@claude
Copy link
Copy Markdown

claude Bot commented Jan 6, 2026

Code Review for PR #425: Remove 'swatches' input

Summary

This PR removes the unused 'swatches' input type from the Weaverse SDK packages and bumps versions for @weaverse/core, @weaverse/react, @weaverse/hydrogen to 5.8.8 and @weaverse/schema to 0.7.4.

✅ Strengths

  1. Clean removal of unused code: The 'swatches' input type has been properly removed from the schema definitions
  2. Proper version synchronization: All fixed group packages (core, react, hydrogen) are versioned together at 5.8.8
  3. CHANGELOG entries: All packages have updated CHANGELOG.md files documenting the change
  4. Consistent with architecture: The change aligns with the type system architecture where schema is the source of truth

🔍 Areas for Review

1. Verification of 'swatches' removal

I noticed the CHANGELOG mentions removing 'swatches' input, but I couldn't find evidence of where 'swatches' was defined in the source code:

  • packages/schema/src/index.ts - No 'swatches' in the inputTypeSchema union
  • packages/core/src/types.ts - No 'swatches' in the InputType union

Question: Was 'swatches' already removed in a previous commit, or is it only present in the archived shopify package? The archived package at archived/shopify/src/types/weaverse-shopify.ts:121 still references 'product-swatches'.

2. Impact assessment

  • The archived @weaverse/shopify package (which is in /archived/shopify and no longer maintained) still references swatches-related types
  • The packages/shopify/dist/ files in this PR include built files that still reference 'swatches'

Clarification needed: Are these dist files expected to be here? The packages/shopify directory is in the archived section but dist files are included in this PR.

3. Breaking change consideration

While this is marked as a patch change (5.8.7 → 5.8.8), removal of an input type could be considered breaking if:

  • Users have existing schemas using the 'swatches' type
  • The schema validation would now fail for those schemas

Recommendation: If this was never publicly documented or used, patch version is appropriate. If it was in use, consider a major version bump or at least document the breaking change clearly.

4. Documentation updates

The packages/schema/README.md lists all supported input types but doesn't mention swatches, which is consistent with its removal. This is good.

📋 Code Quality Observations

No issues found with:

  • Type safety: TypeScript types are properly aligned
  • Schema validation: Zod schemas are consistent
  • Version management: Follows the fixed group pattern correctly
  • Build artifacts: Changes to bun.lock and pnpm-lock.yaml are expected for version bumps

🔍 Test Coverage

Question: Are there tests that verify:

  • The 'swatches' type is no longer accepted by inputTypeSchema?
  • Existing schemas using valid input types continue to work?
  • Migration guidance for users who might have used 'swatches' (if any)?

✅ Recommendations

  1. Clarify the scope: If 'swatches' was only ever in the archived package and not in the active SDK, the changelog could be clearer about this
  2. Test the validation: Add a test case verifying that 'swatches' type is rejected by the schema validation
  3. Migration note: If any users could have used 'swatches', provide a migration path
  4. Clean up dist files: Consider whether the packages/shopify/dist/ files should be in this PR

🎯 Overall Assessment

Status: ✅ LGTM with minor clarifications

The change is straightforward and well-executed. The main areas for clarification are around whether 'swatches' was actively used and if any migration guidance is needed.


Would you like me to help add tests for this change or update any documentation?

@hta218 hta218 merged commit 4c89f8b into main Jan 6, 2026
7 checks passed
@claude
Copy link
Copy Markdown

claude Bot commented Jan 6, 2026

🔍 PR Review: Remove swatches input type

Summary

This PR removes the unused swatches input type from the Weaverse SDK schema across all packages (core, react, hydrogen, schema). This is a straightforward cleanup change that removes dead code from the type system.


Strengths

1. Clear, Focused Change

  • Removes a single unused feature across all affected packages
  • Consistent update across all package CHANGELOGs
  • Version bump follows project conventions (5.8.7 → 5.8.8)

2. Proper Documentation

  • All four package CHANGELOGs updated with clear description
  • Change is documented as a patch release (appropriate for removal of unused feature)

3. Type System Consistency

  • Removal aligns with the documented input types in packages/schema/README.md
  • Maintains consistency between Zod schema and TypeScript types
  • The swatches type was never listed in the official documentation

⚠️ Areas for Improvement

1. Missing Migration Guide

Severity: Low
Impact: Developers may have used the undocumented swatches type

The PR removes the swatches input type, but there's no indication of whether this was ever officially supported or if any users might be using it.

Recommendation: If swatches was ever publicly documented (even briefly), consider adding a BREAKING CHANGE note to the CHANGELOG or providing a migration guide.

2. No Test Coverage Verification

Severity: Medium
Impact: Unclear if tests were updated or run

The PR doesn't indicate whether tests were updated to remove swatches references or if all tests pass after the change.

Recommendation: Before merging, verify npm run typecheck and npm run test pass.

3. Potential Backward Compatibility Concern

Severity: Low (depends on usage)
Impact: Runtime errors if schemas use swatches

Since Zod validates schemas at runtime, any existing schemas using type: 'swatches' will now fail validation with a cryptic error.

Recommendation: Consider adding a helpful error message in the Zod schema validation.


🧪 Testing Recommendations

Before this PR is merged (if not already done):

  1. Type Checking: npm run typecheck
  2. Unit Tests: npm run test
  3. Integration Tests: Check that existing components in templates/pilot don't use swatches

📝 Overall Assessment

Verdict:LGTM with minor suggestions

This is a clean, straightforward removal of dead code. The change is well-executed across all packages with proper versioning and CHANGELOG updates.

Primary Concern: Lack of test verification and potential user impact if swatches was ever used.

Recommended Actions:

  1. ✅ Verify all tests pass
  2. ✅ Run type checking
  3. ℹ️ Consider adding a helpful error message for removed types
  4. ℹ️ If swatches was ever documented, add migration guide

Merge Readiness: ✅ Ready to merge after running tests


💡 Additional Thoughts

The removal aligns well with the project's philosophy of keeping the type system clean and maintainable. The swatches type was never in the official documentation (packages/schema/README.md:18-26), which suggests it may have been internal or experimental.

If this is the case, consider adding a comment in the schema definition indicating which types are stable/public vs. internal/experimental to prevent similar confusion in the future.

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.

1 participant