Walkthrough

Adds top-level optional peer dependency declarations and metadata to package.json, moves webpack-merge to direct dependencies, converts runtime webpack imports to TypeScript type-only imports (with ts-ignore), adds docs and contributor guidance about optional peers, introduces tests and a shell script validating installer behavior, and switches CI and dummy package manager to Yarn.

Changes

Sequence Diagram(s)

(Not generated — changes are dependency metadata, type-only imports, docs, and tests; no new runtime control flow requiring a sequence diagram.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Don't update Bundler in generator specs #539 — Related: modifies spec/generator_specs/generator_spec.rb test setup, touching the same generator test area.
• Add rake shakapacker:doctor command for configuration checks #609 — Closely related: introduces/updates Doctor diagnostics and tests that this change extends with optional-peer checks.

Suggested reviewers

• tomdracz
• G-Rath

Poem

I nudged peers gently, optional and light,
Types say "import type" and skip runtime fright.
Docs rustle, tests hop, installs keep the cheer—
A rabbit's small triumph, tools welcome here. 🥕🐇

Pre-merge checks and finishing touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review for PR #615: Restore optional peer dependencies

Overall Assessment

This PR provides an elegant solution to the peer dependency warnings issue by properly utilizing peerDependenciesMeta to mark dependencies as optional. This is the correct approach that maintains version compatibility information while preventing installation warnings.

✅ Strengths

1. Correct use of npm/yarn features: Using peerDependenciesMeta to mark dependencies as optional is the standard approach for this scenario.

2. Comprehensive dependency restoration: All previously removed peer dependencies have been properly restored with appropriate version ranges.

3. TypeScript handling: The addition of @ts-ignore comments is the appropriate solution for optional dependencies that TypeScript might not find.

4. Testing coverage: Good to see that the changes have been tested with both npm and yarn, and all tests are passing.

🎯 Suggestions for Improvement

1. Version range consistency: Some dependencies use more permissive version ranges than others. For example:

   • sass uses ^1.0.0 which is very permissive
   • Consider if a more specific range like ^1.50.0 would be more appropriate based on actual compatibility

2. Documentation: Consider adding a comment in the package.json or updating the README to explain why all peer dependencies are marked as optional. This will help future contributors understand the design decision.

3. TypeScript comments specificity: The @ts-ignore comments could be more specific:

   // @ts-expect-error - webpack-merge is an optional peer dependency

   This provides better context than @ts-ignore and will fail if the error is resolved.

⚠️ Minor Observations

1. Duplicate version ranges: The peer dependencies duplicate the exact version ranges from devDependencies. While this works, ensure these stay synchronized during future updates.

2. Future maintenance: Consider creating a script or documentation to help maintain version range consistency between devDependencies and peerDependencies.

🔒 Security Considerations

No security issues identified. The changes only affect package metadata and TypeScript comments.

✔️ Conclusion

This is a well-executed fix that properly addresses issue #565. The approach is technically correct and follows npm/yarn best practices. The minor suggestions above are for consideration but not blocking issues.

Approved - Ready to merge once CI passes.

Code Review for PR #615

Thank you for this thoughtful solution to the peer dependencies issue! The approach of using peerDependenciesMeta to mark dependencies as optional is the correct way to handle this situation. Here's my detailed review:

✅ Strengths

1. Correct Solution: Using peerDependenciesMeta with optional: true is the proper npm/yarn way to handle optional peer dependencies. This approach maintains visibility of supported packages and versions while avoiding installation warnings.

2. Comprehensive Coverage: All peer dependencies have been properly configured with matching entries in both peerDependencies and peerDependenciesMeta.

3. TypeScript Handling: Good use of @ts-ignore comments for optional dependencies with clear explanatory comments.

4. Version Ranges: Appropriate version ranges that provide flexibility while ensuring compatibility.

📋 Code Quality & Best Practices

✅ All TypeScript files properly annotated with @ts-ignore comments
✅ Consistent formatting in package.json
✅ Clear and descriptive comments explaining why @ts-ignore is needed
✅ Changes are focused and minimal - no extraneous modifications

🔍 Potential Considerations

1. Documentation: Consider adding a note in the README or contributing guide about the optional peer dependencies structure, explaining that users only need to install the tools they actually use.

2. Migration Guide: For users upgrading, it might be helpful to document that they can now safely ignore peer dependency warnings during installation.

3. Testing Matrix: The PR mentions testing with npm and yarn - excellent! Consider documenting the test scenarios in CI to ensure this continues working across package managers.

🛡️ Security

✅ No security concerns identified
✅ Version constraints are appropriate and don't pin to vulnerable versions

⚡ Performance

✅ No performance impact - this is purely a dependency configuration change
✅ Optional dependencies mean users only install what they need, potentially reducing install size

🧪 Test Coverage

The PR author mentions manual testing with npm, yarn, TypeScript, RSpec, and RuboCop - all passing. This is excellent coverage for this type of change.

💡 Minor Suggestion

Consider adding a comment block above the peerDependencies section in package.json explaining the rationale:

// All peer dependencies are marked as optional via peerDependenciesMeta
// This allows users to install only the tools they need without warnings
"peerDependencies": {

✅ Overall Assessment

This is a well-executed fix that properly addresses issue #565. The implementation follows npm best practices and maintains backward compatibility while improving the developer experience. The code changes are minimal, focused, and correct.

Approved 👍 - This PR is ready to merge.

Great work on finding the right balance between providing dependency information and avoiding unnecessary installation warnings!

Code Review for PR #615

Thank you for implementing the proper solution using peerDependenciesMeta to mark dependencies as optional. This is indeed the correct approach for handling optional peer dependencies.

Strengths

1. Correct approach: Using peerDependenciesMeta with optional: true is the standard way to declare optional peer dependencies, avoiding installation warnings while maintaining version constraints.

2. Comprehensive coverage: All build tool dependencies are properly included as optional peers, covering webpack, rspack, babel, and various loaders.

3. TypeScript handling: Good use of @ts-ignore comments to prevent TypeScript errors when optional packages are not installed.

4. Backwards compatibility: The version ranges match or are broader than the devDependencies, ensuring compatibility.

Observations & Suggestions

1. Version range consistency: The peer dependency version ranges are generally broader than the devDependencies. This is good for flexibility, though you may want to ensure these broader ranges are tested.

2. Missing test coverage: Consider adding tests to validate that the package works correctly when various optional dependencies are not installed. This would help catch regressions.

3. Documentation update: Consider updating the README or documentation to explain which peer dependencies are needed for different configurations (webpack vs rspack, babel vs swc/esbuild).

Minor Issues

1. Duplicate declaration: webpack-merge appears in both peerDependencies and devDependencies. The peer range is broader - ensure v6 compatibility is tested.

2. TypeScript comments consistency: The @ts-ignore comments have slightly different wording between files. Consider making the comment format consistent.

Potential Concerns

1. Breaking change potential: While this should not be a breaking change, users who were relying on the absence of peer dependency warnings as a signal might need to adjust their CI/CD pipelines if they have strict peer dependency checking enabled.

2. Version constraint enforcement: With all dependencies marked as optional, there is no automatic enforcement of version constraints unless the user actually installs the dependency. Consider documenting minimum supported versions clearly.

Recommendation

APPROVED - This is a well-implemented solution that properly addresses issue #565. The use of peerDependenciesMeta is the correct approach, and the implementation is thorough.

Consider the documentation and testing suggestions for future improvements, but they do not block this PR.

Great work on finding and implementing the proper solution!

Code Review for PR #615: Restore Optional Peer Dependencies

✅ Strengths

1. Excellent Solution Architecture

• The approach of using peerDependenciesMeta to mark dependencies as optional is the correct and idiomatic way to handle this in npm/yarn ecosystems
• This properly solves the issue of installation warnings while maintaining version constraint documentation

2. Comprehensive Dependency Coverage

• All relevant build tools and loaders are properly included with appropriate version ranges
• Version ranges are well-thought-out (e.g., ^5.76.0 for webpack, multiple major versions for loaders)
• Good separation between webpack and rspack ecosystems

3. TypeScript Handling

• Proper use of @ts-ignore comments for optional dependencies in the affected files
• Comments clearly explain why the ignore is needed

🔍 Observations & Suggestions

1. Version Range Consistency

• Some dependencies use very wide version ranges (e.g., esbuild with 11 major versions). While this provides flexibility, consider if narrower ranges might prevent compatibility issues
• Suggestion: Document somewhere (perhaps in CONTRIBUTING.md) which versions are actively tested in CI

2. Documentation Enhancement

• Consider adding a section to the README about optional dependencies and what each group provides (Babel vs SWC vs ESBuild, Webpack vs Rspack)
• This would help users understand which dependencies they actually need to install

3. Runtime Detection

• The codebase might benefit from runtime checks for optional dependencies before attempting to use them, rather than relying solely on TypeScript ignores
• Example: Check if webpack is available before attempting webpack-specific operations

✔️ Code Quality

• Clean implementation following project conventions
• Proper formatting and structure in package.json
• TypeScript ignore comments are targeted and well-explained

✔️ Security & Performance

• No security concerns identified
• No performance impact - this is purely a dependency declaration change
• Actually improves install performance by avoiding unnecessary dependency installations

✔️ Testing

• PR author confirms all tests pass
• No runtime changes, so existing test coverage should be sufficient

📋 Summary

APPROVED ✅

This is a well-executed fix that properly addresses the peer dependency warning issue while maintaining good documentation of version constraints. The use of peerDependenciesMeta is the industry-standard solution for this problem.

Minor suggestion for future improvement: Consider adding developer documentation about the optional dependency strategy and which combinations are recommended for different use cases.

Great work on implementing this the right way! 🎉

Code Review for PR #615

Thank you for this well-thought-out PR! I've reviewed the changes and have the following feedback:

✅ Strengths

1. Excellent use of peerDependenciesMeta: This is indeed the correct approach for handling optional peer dependencies. This solves the original issue (Can we remove babel from peerDependencies? #565) elegantly without breaking compatibility.

2. Comprehensive coverage: All build tools and loaders are properly marked as optional, supporting the flexible nature of Shakapacker where users can choose between webpack/rspack and different loaders.

3. TypeScript handling: Good use of @ts-ignore comments with clear explanations for why they're needed. This prevents TypeScript errors when optional packages aren't installed.

4. Version ranges: The peer dependency version ranges are appropriately broad, allowing users flexibility while maintaining compatibility constraints.

🎯 Code Quality & Best Practices

The implementation follows Node.js/npm best practices for optional peer dependencies. The approach is clean and maintainable.

✅ Testing Coverage

According to the PR description, you've tested with:

• npm install (no warnings) ✅
• yarn install (no warnings) ✅
• TypeScript builds ✅
• RSpec tests pass ✅
• RuboCop passes ✅

💡 Suggestions for Consideration

1. Documentation: Consider adding a note to the README or documentation about these optional peer dependencies. Users should understand:

   • Which packages are needed for which features (e.g., babel packages for babel loader, rspack packages for rspack support)
   • That warnings are suppressed but constraints still apply when packages are installed

2. Migration guide: For users upgrading, it might be helpful to document that they can now safely ignore peer dependency warnings.

3. Version consistency: The peer dependency versions mostly match the devDependencies, which is good. However, I noticed babel-loader in devDependencies is ^8.2.4 but in peerDependencies includes ^8.2.4 || ^9.0.0 || ^10.0.0. This broader range is good for flexibility, but ensure it's tested.

✅ Security & Performance

No security concerns identified. This change has no runtime performance impact as it only affects package metadata.

✅ Overall Assessment

This is a high-quality PR that properly addresses the issue. The implementation is correct, follows best practices, and maintains backward compatibility while solving the original problem. The code is clean and the changes are well-documented in the PR description.

Recommendation: Ready to merge after considering the documentation suggestions above.

Great work! 🎉

Code Review

✅ Strengths

1. Proper Solution: Using peerDependenciesMeta to mark dependencies as optional is the correct approach for this issue. This maintains version constraints while eliminating installation warnings.

2. Comprehensive Coverage: All peer dependencies are properly configured with their optional status, ensuring consistency.

3. TypeScript Handling: The addition of @ts-ignore comments for optional dependencies is appropriate and prevents TypeScript errors when packages aren't installed.

4. Version Ranges: The specified version ranges are appropriately broad to support various setups while maintaining compatibility.

🔍 Observations & Suggestions

1. Duplicate Dependencies: Several packages appear in both devDependencies and peerDependencies (webpack, webpack-merge, webpack-subresource-integrity). This is acceptable but worth noting that the version ranges in peerDependencies are broader.

2. Version Range Consistency:

   • Some packages like esbuild have very broad ranges (0.14.0 through 0.24.0)
   • Consider if all these versions are truly tested and supported
   • The sass range ^1.0.0 might be too broad - sass has had breaking changes between major versions

3. Documentation: Consider updating the README or adding a note about which packages are required vs optional based on the chosen bundler configuration.

🐛 Potential Issues

None identified. The implementation follows best practices for optional peer dependencies.

🔒 Security

No security concerns. The changes are limited to dependency metadata.

✅ Test Coverage

According to the PR description, testing was comprehensive:

• npm/yarn installation without warnings ✅
• TypeScript compilation ✅
• RSpec tests passing ✅
• RuboCop passing ✅

💡 Minor Suggestions

1. Consider adding a test case in spec/shakapacker/doctor_spec.rb to verify that the doctor command handles optional peer dependencies correctly.

2. The @ts-ignore comments could potentially be replaced with more specific type guards or conditional imports in the future, but this is not critical.

Verdict

Approved ✅ - This is a well-implemented solution that properly addresses issue #565. The use of peerDependenciesMeta is the correct approach, and the implementation is thorough and consistent.

The change successfully eliminates installation warnings while maintaining clear documentation of supported package versions, which will help with future upgrades.

Code Review for PR #615: Restore optional peer dependencies

✅ Overall Assessment

This is a well-thought-out solution that properly addresses the peer dependency warnings issue (#565) using the correct npm/yarn mechanism (peerDependenciesMeta). The approach maintains version constraints while eliminating installation warnings.

👍 Strengths

1. Correct approach: Using peerDependenciesMeta with optional: true is the proper npm/yarn way to handle optional peer dependencies. This is superior to removing them entirely.

2. Comprehensive coverage: All relevant build tools and loaders are properly declared with appropriate version ranges that match the devDependencies.

3. TypeScript handling: Good use of @ts-ignore comments with clear explanations for why they're needed (optional peer dependencies).

4. Version flexibility: The version ranges are appropriately broad (e.g., ^5.76.0 for webpack, multiple major versions for loaders) which provides good flexibility for users.

5. Dual bundler support: Properly includes both webpack and rspack ecosystems.

🔍 Observations & Suggestions

1. Version alignment: The peer dependency versions generally align well with devDependencies. For example:

   • webpack: devDep uses 5.93.0, peer allows ^5.76.0 ✅
   • webpack-merge: both use ^5.8.0 with peer also allowing ^6.0.0 ✅

2. TypeScript comments consistency: All TypeScript files that import webpack types have the appropriate @ts-ignore comments. Good consistency.

3. No runtime impact: Since all peers are marked optional, this change won't break existing installations and only enforces constraints when packages are actually installed.

⚠️ Minor Considerations

1. Maintenance burden: Having duplicate version specifications in both peerDependencies and peerDependenciesMeta could be a maintenance point. Consider adding a comment in package.json explaining that ALL peer deps must have a corresponding entry in peerDependenciesMeta.

2. Documentation: Consider updating the README or documentation to explain that all peer dependencies are optional and users only need to install what they actually use.

✅ Testing Verification

According to the PR description, appropriate testing has been done:

• npm install shows no warnings ✅
• yarn install shows no warnings ✅
• TypeScript builds successfully ✅
• RSpec tests pass ✅
• RuboCop passes ✅

🎯 Conclusion

This PR correctly solves the peer dependency warning issue while maintaining proper dependency tracking. The implementation follows npm/yarn best practices and maintains backward compatibility.

Recommendation: APPROVE ✅

The change elegantly solves the problem stated in issue #565 without compromising the ability to track supported versions or breaking existing installations.

Code Review

Thank you for this PR! The approach of restoring peer dependencies with peerDependenciesMeta is the correct solution. Here's my feedback:

✅ Strengths

1. Proper use of peerDependenciesMeta: Marking all peer dependencies as optional is the right approach to avoid installation warnings while maintaining version constraints for users who do install these packages.

2. Comprehensive dependency coverage: You've included all relevant build tools with appropriate version ranges that allow flexibility while ensuring compatibility.

3. TypeScript handling: Adding @ts-ignore comments for optional peer dependencies is a pragmatic solution that prevents compilation errors when packages aren't installed.

4. Good testing coverage: The PR description shows thorough testing with multiple package managers.

🎯 Suggestions for Improvement

1. Consider documenting the TypeScript workaround: The @ts-ignore comments might confuse future contributors. Consider adding a more descriptive comment explaining why they're needed:

   // @ts-ignore - webpack is an optional peer dependency that may not be installed

2. Version range consistency: The version ranges look good, but consider if webpack-merge needs the same ^6.0.0 range support that you've added to its peer dependency entry (currently the devDependency only has ^5.8.0).

3. Documentation update: Consider updating the README to explain that while these are peer dependencies, they're all marked as optional, so users won't see warnings if they don't install them.

✅ Security & Performance

• No security concerns identified
• No performance impact as this is a packaging change only
• The approach correctly maintains backward compatibility

✅ Best Practices

• Follows NPM's recommended approach for optional peer dependencies
• Maintains clean separation between required and optional dependencies
• Version ranges are appropriately flexible

Minor Notes

• All files correctly end with trailing newlines ✅
• The change is focused and minimal, avoiding unnecessary diffs ✅

Overall, this is a well-executed fix that properly addresses the issue while maintaining good package hygiene. The use of peerDependenciesMeta is exactly the right solution for this problem.

Approved 👍

Code Review for PR #615: Restore Optional Peer Dependencies

✅ Overall Assessment

This PR provides an excellent solution to the peer dependency warnings issue by properly utilizing peerDependenciesMeta to mark dependencies as optional. This is the correct npm/yarn approach that maintains version compatibility information while preventing installation warnings.

👍 Strengths

1. Correct Solution: Using peerDependenciesMeta with optional: true is the idiomatic way to handle optional peer dependencies. This approach maintains visibility of supported packages and versions while avoiding installation warnings.

2. Comprehensive Coverage: All relevant build tools are properly configured - webpack, rspack, babel, swc, esbuild, and various loaders with appropriate version ranges.

3. TypeScript Handling: Good use of @ts-ignore comments with clear explanations for why they're needed on optional dependency imports.

4. Version Flexibility: Appropriate version ranges that provide flexibility (e.g., multiple major versions for stable loaders) while ensuring compatibility.

5. Backwards Compatible: This change is non-breaking and properly addresses issue Can we remove babel from peerDependencies? #565.

🎯 Suggestions for Improvement

1. Consider using @ts-expect-error instead of @ts-ignore: This provides better type safety as it will fail if the error is resolved:

   // @ts-expect-error - webpack is an optional peer dependency
   import { Configuration } from "webpack"

2. Documentation: Consider adding a note in the README explaining:

   • Which peer dependencies are needed for different configurations (webpack vs rspack)
   • That users only need to install the tools they actually use
   • Migration guide for users upgrading

3. Version Range Review: The esbuild dependency supports 11 major versions (0.14-0.24). While flexible, consider if a narrower range might prevent compatibility issues.

⚠️ Minor Observations

1. webpack-merge duplication: This package appears in both devDependencies (^5.8.0) and peerDependencies (^5.8.0 || ^6.0.0). The peer range is broader - ensure v6 compatibility is tested.

2. Type-only imports: Consider using import type for type-only imports in TypeScript files to make intent clearer:

   // @ts-ignore - webpack is an optional peer dependency
   import type { Configuration } from "webpack"

🔒 Security Considerations

No security issues identified. The webpack minimum version (^5.76.0) appropriately addresses known vulnerabilities in earlier versions.

✔️ Conclusion

This is a well-executed fix that properly addresses the peer dependency warning issue. The implementation follows npm/yarn best practices and maintains backwards compatibility.

Approved ✅ - Ready to merge once CI passes.

Excellent work on finding the right balance between providing dependency information and avoiding unnecessary installation warnings!

Code Review for PR #615: Restore Optional Peer Dependencies

Thank you for implementing this well-thought-out solution to address issue #565! Using peerDependenciesMeta to mark dependencies as optional is indeed the correct approach.

✅ Strengths

1. Correct Solution Architecture: Using peerDependenciesMeta with optional: true is the idiomatic npm/yarn approach for optional peer dependencies. This maintains version compatibility information while preventing installation warnings.

2. Comprehensive Coverage: All relevant build tools are properly included - webpack, rspack, babel, swc, esbuild, and their associated loaders with appropriate version ranges.

3. TypeScript Handling: Proper use of @ts-ignore comments with clear explanations for why they're needed on optional dependencies.

4. Documentation Update: Good addition to the README explaining the optional peer dependencies design decision.

📋 Code Quality

✅ Follows project conventions per CLAUDE.md
✅ All files have proper trailing newlines
✅ TypeScript builds successfully with the @ts-ignore comments
✅ No extraneous changes - focused and minimal diff

🎯 Minor Suggestions for Future Consideration

1. TypeScript Comment Specificity: Consider using @ts-expect-error instead of @ts-ignore for better type safety:

   // @ts-expect-error - webpack is an optional peer dependency

   This will fail if the error is resolved, alerting you to remove the comment.

2. Version Range Documentation: Some dependencies have very broad version ranges (e.g., esbuild supports 11 major versions). While this provides flexibility, consider documenting tested version combinations in the README.

3. Future Maintenance: Consider creating a script to ensure version ranges stay synchronized between devDependencies and peerDependencies during updates.

🔍 Observations

• The peer dependency version ranges are appropriately broader than or equal to devDependencies, providing flexibility
• All 26 peer dependencies have matching peerDependenciesMeta entries
• The solution properly addresses both webpack and rspack configurations

🔒 Security & Performance

✅ No security concerns - changes only affect package metadata
✅ No performance impact - users only install what they need
✅ Webpack minimum version ^5.76.0 includes important security fixes

✅ Conclusion

This is an excellent implementation that properly solves the peer dependency warnings issue while maintaining clear version constraints for users who do install the dependencies. The approach follows npm/yarn best practices and improves the developer experience.

APPROVED ✨ - Ready to merge!

Great work on finding the right balance between providing dependency information and avoiding unnecessary installation warnings. This change will significantly improve the installation experience for users who don't need all the optional build tools.

Code Review

Thank you for this PR that properly restores peer dependencies with optional configuration! This is a well-thought-out solution to the package installation warning issue. Here's my comprehensive review:

✅ Strengths

1. Correct Approach: Using peerDependenciesMeta to mark all peer dependencies as optional is the right solution. This allows tracking supported package versions without forcing installation warnings.

2. Comprehensive Coverage: All relevant build tools and loaders are included with appropriate version ranges that accommodate different user setups.

3. TypeScript Handling: Good use of @ts-ignore comments for optional peer dependency imports. The comments clearly explain why they're needed.

4. Testing: The included test script (test/peer-dependencies.sh) is thorough, testing npm, yarn, and pnpm installations for warnings.

🔍 Observations & Suggestions

Code Quality

• The implementation follows the existing code conventions well
• All files properly end with trailing newlines as required by the project's linting rules ✓

Version Ranges

• Version ranges for peer dependencies are appropriately broad (using || for multiple major versions)
• This provides good flexibility for users while maintaining compatibility constraints

TypeScript Approach

• The @ts-ignore comments are placed correctly before each optional import
• Consider exploring module augmentation or conditional type imports as a future enhancement to avoid @ts-ignore, though the current approach is pragmatic

⚠️ Minor Considerations

1. Documentation: The PR mentions updates to CONTRIBUTING.md, README.md, and docs/v9_upgrade.md. Make sure these clearly explain:

   • Which packages users need to install based on their bundler choice (webpack vs rspack)
   • The minimum required packages for a basic setup

2. Test Script Portability: The test script uses mktemp -d which might not be available on all systems (particularly Windows). Consider adding a fallback or documenting this as a Unix-only test.

3. Performance: With all peer dependencies marked as optional, ensure tree-shaking still works effectively to avoid bundling unused code.

✅ Testing Validation

Based on the PR description, all critical tests pass:

• npm install shows no warnings ✓
• yarn install shows no warnings ✓
• TypeScript builds successfully ✓
• RSpec tests pass ✓
• RuboCop passes ✓

🎯 Overall Assessment

APPROVED - This is a solid implementation that solves the peer dependency warning issue while maintaining version tracking capabilities. The use of peerDependenciesMeta is the correct approach, and the implementation is thorough and well-tested.

The changes properly balance user experience (no installation warnings) with maintainability (clear version constraints). This should resolve #565 effectively.

Great work on finding the right balance between user experience and package maintenance! 🚀

Code Review for PR #615: Restore optional peer dependencies with proper configuration

✅ Strengths

1. Excellent Solution Architecture: Using peerDependenciesMeta to mark dependencies as optional is the proper way to handle this situation. This approach maintains version constraints while eliminating installation warnings.

2. Backward Compatibility: The change preserves backward compatibility - existing projects will continue to work without modification, and new projects get the benefits of optional dependencies.

3. Comprehensive Documentation: Great job documenting the change in multiple places (README.md, CONTRIBUTING.md, and v9_upgrade.md). The documentation clearly explains the benefits and implications.

4. Proper TypeScript Handling: The addition of @ts-ignore comments for optional packages is the right approach to prevent TypeScript errors when optional dependencies aren't installed.

5. Test Coverage: The new test/peer-dependencies.sh script provides good coverage for testing across multiple package managers (npm, yarn, pnpm).

🔍 Observations & Suggestions

1. webpack-merge Dependency Move: I notice webpack-merge was moved from devDependencies to dependencies. This makes sense since it's required for the core functionality, but consider documenting this change in the PR description as it's a subtle but important change.

2. Test Script Considerations:

   • The test script looks solid but consider adding it to CI to ensure future changes don't break this functionality
   • The script assumes the current directory is the shakapacker root - consider adding a check or making it more flexible

3. Version Ranges: The peer dependency version ranges are quite broad (e.g., esbuild supports versions 0.14 through 0.24). This is good for flexibility but ensure these ranges are tested periodically.

4. Missing SWC Core: I notice @swc/core is not in the peer dependencies list, though swc-loader is. If users choose SWC, they'll need @swc/core as well. Consider adding it as an optional peer dependency.

🛡️ Security & Performance

• ✅ No security concerns identified
• ✅ No performance impact - this is a metadata-only change
• ✅ Follows the project's linting rules (files end with trailing newlines)

✨ Code Quality

• ✅ Clean implementation following best practices
• ✅ Consistent code style matching the existing codebase
• ✅ No unnecessary complexity introduced

🎯 Overall Assessment

Approved - This is a well-thought-out solution that elegantly solves the peer dependency warning issue while maintaining flexibility and backward compatibility. The implementation is clean, well-documented, and thoroughly tested.

The only minor suggestion would be to consider adding @swc/core to the optional peer dependencies list for completeness, but this isn't blocking.

Great work on this PR! 👍

Code Review for PR #615: Restore Optional Peer Dependencies

Thank you for this well-thought-out solution! Using peerDependenciesMeta to mark dependencies as optional is indeed the correct approach to resolve the installation warnings issue while maintaining version visibility.

✅ Strengths

1. Correct NPM/Yarn Pattern: Using peerDependenciesMeta with optional: true is the standard way to handle optional peer dependencies. This eliminates installation warnings while preserving version constraints.

2. Comprehensive Coverage: All build tools are properly included - webpack, rspack, babel, swc, esbuild, and associated loaders with appropriate version ranges.

3. TypeScript Handling: Good use of type-only imports and @ts-ignore comments where needed, with clear explanatory comments.

4. Documentation: Excellent additions to CONTRIBUTING.md and v9_upgrade.md explaining the approach and testing methodology.

5. Testing: The included test script (test/peer-dependencies.sh) provides automated validation across multiple package managers.

📋 Code Quality

✅ Follows project conventions per CLAUDE.md
✅ Consistent formatting and structure
✅ No unnecessary changes or scope creep
✅ Files end with trailing newlines as required

🔍 Suggestions for Improvement

1. Type-only Imports: Consider using import type for TypeScript imports that are only used as types:

   // Instead of:
   import { Configuration } from "webpack"
   
   // Use:
   import type { Configuration } from "webpack"

   This makes the intent clearer and ensures proper tree-shaking.

2. webpack-merge Placement: Currently webpack-merge is in both dependencies and peerDependencies. Since it's a required dependency for the package to function, it should remain only in dependencies and be removed from peerDependencies.

3. Version Range Documentation: Consider adding a comment in package.json explaining why some packages have very broad version ranges (like esbuild with 11 major versions). This helps future maintainers understand the compatibility testing scope.

🛡️ Security & Performance

✅ No security issues identified
✅ Version constraints avoid known vulnerable versions
✅ Optional dependencies reduce install size for users who don't need all tools

🧪 Test Coverage

The manual testing mentioned (npm, yarn, TypeScript, RSpec, RuboCop) plus the automated test script provide good coverage. The test script is particularly valuable for CI/CD integration.

⚠️ Minor Notes

1. Migration Impact: While not breaking, users with strict peer dependency checking in CI might need to adjust their configurations.

2. Maintenance Consideration: Keep devDependencies and peerDependencies version ranges synchronized during future updates.

✅ Overall Assessment

This is an excellent implementation that properly addresses issue #565. The solution is technically correct, follows npm/yarn best practices, and improves developer experience without breaking changes.

The use of optional peer dependencies is the right balance between:

• Preventing annoying installation warnings
• Maintaining clear version compatibility documentation
• Allowing users to install only what they need

Status: APPROVED ✅

Great work on researching and implementing the proper solution! The code is ready to merge once CI passes.

🤖 Generated with Claude Code

Code Review for PR #615: Restore Optional Peer Dependencies

I have conducted a thorough review of this PR. Here is my detailed feedback:

✅ Strengths

1. Excellent Solution Architecture: Using peerDependenciesMeta to mark all peer dependencies as optional is the correct approach. This maintains version constraints while eliminating installation warnings.

2. Comprehensive Documentation: The PR adds helpful documentation in multiple places (README.md, CONTRIBUTING.md, v9_upgrade.md) explaining the optional peer dependencies strategy clearly.

3. Good Test Coverage: The new RSpec test file (optional_dependencies_spec.rb) and shell script (test/peer-dependencies.sh) provide thorough validation of the changes.

4. Correct webpack-merge Handling: Moving webpack-merge from devDependencies to dependencies is the right decision since it is used directly by the package runtime code.

🔍 Code Quality & Best Practices

1. TypeScript Handling: The @ts-ignore comments are properly formatted and consistent across all files needing them. The use of import type for optional dependencies is correct.

2. Version Range Compatibility: The peer dependency version ranges are appropriately broad to support multiple major versions (e.g., babel-loader: "^8.2.4 || ^9.0.0 || ^10.0.0").

3. Follows Project Guidelines: All files end with trailing newlines as required by CLAUDE.md.

⚠️ Potential Issues to Consider

1. Missing @swc/core in peerDependencies: While swc-loader is listed as a peer dependency, @swc/core (which swc-loader requires) is not. This could lead to confusion for users setting up SWC. Consider adding it with appropriate version ranges.

2. Test Script Portability: The shell test script (test/peer-dependencies.sh) uses bash-specific features and may not work on all systems. Consider adding a Node.js-based test as a more portable alternative.

3. Documentation Examples: The README examples show specific version numbers that might become outdated. Consider adding a note that users should check for the latest compatible versions.

🚀 Performance Considerations

• No Performance Impact: The changes are configuration-only and will not affect runtime performance.
• Reduced Installation Size: Users benefit from smaller installations by only including needed dependencies.

🔒 Security Considerations

• No Security Issues: The changes do not introduce any security vulnerabilities.
• Dependency Management: The optional peer dependencies approach actually improves security by allowing users to upgrade dependencies independently.

💡 Suggestions for Improvement

1. Add @swc/core to peer dependencies for completeness when using SWC.

2. Consider a migration helper: A small utility script that analyzes a user current setup and suggests which peer dependencies to install might be helpful.

3. CI Validation: Consider adding a GitHub Actions job that tests installation with different package managers to catch any future regressions.

✅ Overall Assessment

This is a well-implemented PR that properly addresses the peer dependency warnings issue while maintaining backward compatibility. The solution is elegant, follows best practices, and is thoroughly tested. The documentation updates are comprehensive and will help users understand the new approach.

Recommendation: APPROVE with the minor suggestion to add @swc/core to peer dependencies for completeness.

Great work on implementing this clean solution to a common package management challenge! 👍

Code Review for PR #615: Restore Optional Peer Dependencies

Overall Assessment

This PR provides an excellent solution to the peer dependency warnings issue by properly utilizing peerDependenciesMeta to mark dependencies as optional. This is the correct, idiomatic approach that maintains version compatibility information while preventing installation warnings.

Strengths

1. Correct Architecture: Using peerDependenciesMeta with optional: true is the standard npm/yarn approach for this scenario. This elegantly solves issue Can we remove babel from peerDependencies? #565.

2. Comprehensive Coverage: All build tools (webpack, rspack, babel, swc, esbuild) and their loaders are properly configured as optional peer dependencies with appropriate version ranges.

3. TypeScript Handling: Good use of @ts-ignore comments with clear explanations for optional dependencies. This prevents TypeScript errors when packages aren't installed.

4. Important Fix: Moving webpack-merge from devDependencies to dependencies ensures it's always available, which is correct since Shakapacker's core functionality requires it.

5. Testing: Excellent testing coverage with npm, yarn, TypeScript, RSpec, and RuboCop all passing.

Code Quality

• Follows project conventions (trailing newlines, formatting)
• Clean, focused changes without extraneous modifications
• Proper documentation updates in CONTRIBUTING.md, README.md, and v9_upgrade.md
• Well-structured test coverage for optional dependency scenarios

Minor Observations & Suggestions

1. Version Ranges: Some dependencies have very wide version ranges (e.g., esbuild supports 11 major versions). While this provides flexibility, consider documenting which versions are actively tested in CI.

2. TypeScript Comments: Consider using @ts-expect-error instead of @ts-ignore for better type safety.

3. Test Shell Script: The new test/peer-dependencies.sh script is excellent for validation. Consider adding it to CI to ensure this behavior is maintained.

Security & Performance

• No security concerns identified
• Version constraints don't pin to vulnerable versions
• Actually improves install performance by allowing users to skip unnecessary dependencies

Potential Issues

None identified. The approach is solid and backward compatible.

Future Enhancements

1. Consider runtime checks for optional dependencies before use, rather than relying solely on TypeScript ignores
2. Add a compatibility matrix in the docs showing which peer dependency combinations are tested
3. Consider a CLI command or script to help users install the right dependencies for their configuration

Conclusion

APPROVED - This is a well-executed fix that properly addresses the peer dependency warning issue while maintaining excellent backward compatibility. The implementation follows npm/yarn best practices and the code quality meets the project's high standards.

Great work on finding the right balance between providing dependency information and avoiding unnecessary installation warnings! This will significantly improve the developer experience for Shakapacker users.

The minor suggestions above are for consideration but not blocking. This PR is ready to merge once CI passes.

Code Review for PR #615: Restore Optional Peer Dependencies

🎯 Overall Assessment

This PR effectively addresses the peer dependency warnings issue (#565) by properly implementing optional peer dependencies using peerDependenciesMeta. The solution is well-architected and follows npm best practices.

✅ Strengths

1. Correct Implementation Pattern: The use of peerDependenciesMeta to mark all peer dependencies as optional is the proper npm solution for this issue. This eliminates installation warnings while maintaining version constraints.

2. Type Safety: Excellent use of TypeScript type-only imports (import type) to prevent runtime errors when optional packages aren't installed. The @ts-ignore comments are appropriately placed and consistently formatted.

3. Comprehensive Documentation: Outstanding documentation additions in multiple files:

   • New docs/optional-peer-dependencies.md with detailed explanations
   • Updates to README.md with configuration examples
   • Migration guide updates in docs/v9_upgrade.md
   • CONTRIBUTING.md updates for developers

4. Thorough Test Coverage: Excellent test suite additions:

   • spec/shakapacker/optional_dependencies_spec.rb validates package.json structure
   • spec/shakapacker/doctor_optional_peer_spec.rb ensures doctor command handles optional deps
   • test/peer-dependencies.sh for installation warning tests
   • Tests verify shakapacker loads without optional dependencies at runtime

5. webpack-merge as Direct Dependency: Smart move making webpack-merge a direct dependency since it's always required regardless of bundler choice.

🔍 Minor Observations

1. Version Ranges: The peer dependency version ranges are appropriately broad (e.g., ^5.76.0 for webpack), allowing flexibility while maintaining compatibility. Good forward-thinking with ranges like ^5.0.0 || ^6.0.0 for future compatibility.

2. Doctor Command: While the PR mentions doctor command updates, ensure the doctor fully recognizes webpack-merge as a direct dependency now (not checking it as a missing peer dep).

3. File Endings: All modified files correctly maintain trailing newlines per project requirements.

🚀 Performance Considerations

• ✅ No runtime performance impact - type-only imports are erased during compilation
• ✅ Reduced installation time and disk usage by not forcing unnecessary dependencies
• ✅ Smaller node_modules for users who only need specific configurations

🔒 Security

• ✅ No security concerns identified
• ✅ No credentials or sensitive information exposed
• ✅ Dependencies maintain appropriate version constraints

📝 Recommendations

1. Consider CI Matrix Testing: Add CI jobs that test specific configuration combinations (webpack+babel, webpack+swc, rspack+swc) to ensure each works independently.

2. Doctor Command Enhancement: Consider updating the doctor command to provide clearer guidance about which dependencies are needed for the user's specific configuration.

3. Installation Script: The installer could potentially detect existing configurations and suggest optimal dependency sets during migration.

✨ Conclusion

This is a high-quality PR that properly solves the peer dependency warning issue while maintaining backward compatibility and providing excellent documentation. The implementation follows best practices and includes comprehensive testing. Approved for merge pending CI checks passing.

Great work on the thorough implementation and documentation! 👏

🤖 Generated with Claude Code

Code Review for PR #615: Restore optional peer dependencies with proper configuration

Summary

This PR successfully addresses the peer dependency warning issue by properly configuring all peer dependencies as optional using peerDependenciesMeta. The implementation is solid and follows best practices for package dependency management.

✅ Strengths

1. Correct Implementation: The use of peerDependenciesMeta to mark all peer dependencies as optional is the industry-standard approach. This eliminates installation warnings while maintaining version constraints.

2. Comprehensive Documentation: Excellent documentation additions across multiple files (README, CONTRIBUTING, new docs file) that clearly explain the optional peer dependency approach and provide configuration examples.

3. Type Safety Maintained: The addition of @ts-ignore comments with clear explanations for TypeScript imports ensures the package works correctly even when optional dependencies aren't installed.

4. Thorough Testing: Good test coverage with:

   • optional_dependencies_spec.rb - validates package.json structure
   • doctor_optional_peer_spec.rb - ensures doctor command handles optional deps
   • peer-dependencies.sh - tests installation with multiple package managers

5. Backward Compatibility: Moving webpack-merge to direct dependencies ensures existing code continues to work.

🔍 Observations & Suggestions

1. TypeScript Comments Consistency: The TypeScript ignore comments are correctly placed and include explanatory text. Consider standardizing the format:

   // @ts-ignore: webpack is an optional peer dependency (using type-only import)

2. Version Range Strategy: The peer dependency version ranges are appropriately broad (e.g., multiple major versions supported with || operator), which provides good flexibility for users.

3. Generator Spec Enhancement: Good addition to update the package.json in generator specs to use the local shakapacker path - this ensures tests work correctly with the new structure.

📋 Minor Considerations

1. Doctor Command: The doctor command might still report webpack-merge as missing since it's checking the old peer dependencies list. This could be addressed in a follow-up PR to update the doctor logic.

2. Test Coverage: The test script peer-dependencies.sh is well-written but could benefit from being integrated into CI to ensure no regressions.

3. Documentation Clarity: The migration guide clearly states "No action required" for existing users, which is excellent for reducing upgrade friction.

✅ Testing Validation

Based on the PR description, all key testing scenarios have been covered:

• ✅ npm install - no warnings
• ✅ yarn install - no warnings
• ✅ TypeScript builds successfully
• ✅ RSpec tests pass
• ✅ RuboCop passes

🎯 Conclusion

This PR effectively solves issue #565 by implementing optional peer dependencies correctly. The approach:

• Eliminates installation warnings for all package managers
• Maintains clear visibility of supported package versions
• Provides maximum flexibility for users to choose their toolchain
• Preserves backward compatibility

The code quality is high, documentation is comprehensive, and the implementation follows Node.js ecosystem best practices.

Recommendation: ✅ Approve

The changes are well-thought-out and properly implemented. This is a significant improvement to the package's dependency management that will benefit all users.

🤖 Generated with Claude Code

Code Review for PR #615: Restore Optional Peer Dependencies

Thank you for this comprehensive implementation of optional peer dependencies. This is a significant improvement that addresses the installation warning issues while maintaining clear version constraints. Here's my detailed review:

✅ Strengths

1. Excellent Solution Architecture

   • Using peerDependenciesMeta to mark all peer dependencies as optional is the correct approach
   • Properly maintains version constraints while avoiding installation warnings
   • Clean separation between direct dependencies (webpack-merge) and optional peers

2. Code Quality

   • TypeScript handling with @ts-ignore comments and type-only imports is appropriate
   • Consistent formatting and follows project conventions (trailing newlines preserved)
   • No extraneous changes or unnecessary modifications

3. Documentation

   • Comprehensive documentation added (CONTRIBUTING.md, README.md, docs/optional-peer-dependencies.md)
   • Clear examples for different configuration scenarios
   • Helpful migration guide for v8 to v9 upgrades

4. Testing Approach

   • Good test coverage with spec files for optional dependencies validation
   • Proper handling of different package manager scenarios

🔍 Observations & Suggestions

1. webpack-merge Dependency

   • Good move making it a direct dependency since it's always required
   • Consider updating the doctor command to recognize this change (currently still checks as peer)

2. Version Ranges

   • Peer dependency ranges are appropriately broad for flexibility
   • Consider documenting the rationale for specific version floors (e.g., webpack ^5.76.0)

3. Performance Consideration

   • The optional peer approach will reduce install times and disk usage ✅
   • No runtime performance impact due to type-only imports ✅

🛡️ Security & Best Practices

• No security concerns identified
• Follows npm best practices for optional dependencies
• No credential or sensitive data exposure

📋 Minor Suggestions

1. Consider adding a script to validate peer/dev dependency version sync:

   "scripts": {
     "validate:deps": "node scripts/validate-dependency-versions.js"
   }

2. The doctor_optional_peer_spec.rb test file appears to be missing from the PR diff. Consider adding it if not already included.

3. For the TypeScript ignores, consider using a more specific comment format for easier future updates:

   // @ts-ignore: webpack is optional peer - type-only import

✅ Overall Assessment

This is a well-executed PR that solves the peer dependency warning issue elegantly. The implementation is clean, thoroughly documented, and maintains backward compatibility. The approach of making all peers optional while keeping version constraints is the right balance between flexibility and safety.

Recommendation: APPROVED ✅

Great work on this implementation! The optional peer dependencies approach will significantly improve the developer experience while maintaining the flexibility Shakapacker users need.

🤖 Generated with Claude Code

Code Review: Optional Peer Dependencies

This PR successfully implements optional peer dependencies using peerDependenciesMeta, addressing issue #565.

Strengths

1. Excellent package.json structure - all peer deps marked optional
2. Proper TypeScript type-only imports prevent runtime failures
3. Comprehensive test coverage across multiple scenarios
4. Excellent documentation with migration guide

Issues Found

1. Inconsistent Code Style in base.js (Severity: Low)

• Unnecessary formatting changes that don't align with base.ts
• Recommendation: Revert base.js to match base.ts logic

2. Error Message Inconsistency

• TypeScript version has detailed error messages, JavaScript version doesn't
• Recommendation: Sync error messages between files

3. Unintended Code Changes

• Per CLAUDE.md: Keep changes focused and minimal
• base.js has unrelated spacing/structure modifications

Potential Concerns

1. Missing @swc/core in peer dependencies? (swc-loader is there but not @swc/core)
2. Doctor command needs update to recognize webpack-merge as direct dependency

Security & Performance

• No security issues
• Positive performance: smaller node_modules, faster installation

Recommendations

Must Fix:

• Revert unnecessary formatting in package/environments/base.js
• Sync error messages between base.js and base.ts

Should Consider:

• Add @swc/core to peerDependencies
• Update Doctor command in follow-up PR

Conclusion

High-quality PR, well-tested and documented. Main issues are cosmetic. Rating: 8.5/10

Review by Claude Code

PR Review: Optional Peer Dependencies Implementation

This PR successfully addresses the peer dependency warning issue while maintaining clear version constraints.

Strengths

1. Excellent Problem Solution

• Properly uses peerDependenciesMeta to mark all peer dependencies as optional
• Eliminates installation warnings while preserving version constraint enforcement
• Well-researched approach that follows npm/yarn/pnpm best practices

2. Comprehensive Documentation

• Outstanding new documentation in docs/optional-peer-dependencies.md
• Clear examples for different configuration scenarios
• Updated README with helpful dependency matrices
• Good migration guide in docs/v9_upgrade.md

3. Strong Test Coverage

• Added RSpec tests for validating package.json structure
• Shell script for testing installation warnings across package managers
• CI workflow updates to use Yarn consistently

4. TypeScript Handling

• Correct use of type-only imports with ts-ignore comments
• Prevents runtime errors when optional packages are not installed

Critical Issues

1. base.js Regression (BLOCKING)

File: package/environments/base.js

The refactoring introduces a logic bug. The variable reassignment logic is confusing - the new code assigns assetPaths = previousPaths which seems backwards, and always assigns entries[name] = assetPaths even when it should be the original single path.

Recommendation: Revert package/environments/base.js to the original logic. The base.ts file has the correct implementation.

2. Error Message Regression

File: package/environments/base.js lines 52-55

The error message for invalid source_entry_path configuration was downgraded from detailed to vague.

Recommendation: Keep the detailed error message from base.ts for better DX.

3. Inconsistency: base.js vs base.ts

The changes create divergence between base.js and base.ts. The TypeScript version maintains correct logic while the JavaScript version introduces bugs.

Recommendation: Revert all changes to base.js OR compile base.js from base.ts to ensure they stay in sync.

Security and Performance

Security: No issues found
Performance: Neutral to positive (smaller node_modules)

Testing Recommendations

1. Integration test: Build with ONLY rspack (no webpack)
2. Integration test: Build with ONLY webpack (no rspack)
3. Regression test: Verify multi-file entry points work correctly
4. Cross-bundler test: Ensure switching between webpack/rspack works

Action Items

Must Fix (Blocking):

1. Revert package/environments/base.js changes - introduces logic bug
2. Keep detailed error message

Should Fix:
3. Ensure base.js and base.ts stay in sync
4. Add integration tests
5. Test multi-file entry points

Overall Assessment

Concept: Excellent
Documentation: Outstanding
Testing: Good (needs integration tests)
Implementation: Good idea, but base.js regression is concerning

Recommendation: Request changes to fix the base.js regression, then approve. The optional peer dependencies approach is excellent and well-documented, but the unintentional changes to base.js introduce bugs that must be fixed before merging.

Code Review: Optional Peer Dependencies Implementation

I've reviewed PR #615 which restores peer dependencies as optional. This is a well-thought-out solution that addresses issue #565. Here's my comprehensive feedback:

✅ Strengths

1. Excellent Design Decision

• Using peerDependenciesMeta with optional: true is the correct approach for this use case
• Allows users to install only what they need while maintaining version constraints
• Eliminates peer dependency warnings during installation

2. Comprehensive Documentation

• Created detailed docs/optional-peer-dependencies.md with clear examples
• Updated README.md with practical configuration examples for different setups
• Added upgrade guide in docs/v9_upgrade.md
• Updated CONTRIBUTING.md with testing guidance

3. Strong Test Coverage

• Added spec/shakapacker/optional_dependencies_spec.rb with comprehensive checks
• Created test/peer-dependencies.sh for cross-package-manager testing
• Tests verify structure, installation behavior, and TypeScript imports

4. Proper TypeScript Handling

• Uses type-only imports (import type { ... }) for optional webpack dependencies
• Added @ts-ignore comments with clear explanations
• Prevents runtime errors when optional packages aren't installed

⚠️ Issues Found

Critical: Regression in package/environments/base.js

The JavaScript version of base.js was incorrectly modified and introduces bugs:

Line 54-56: Error message was downgraded

// ❌ BAD (in this PR):
"Your shakapacker config specified using a source_entry_path of '/' with 'nested_entries' == " +
  "'true'. Doing this would result in packs for every one of your source files"

// ✅ GOOD (original TypeScript version):
`Invalid Shakapacker configuration detected!\n\n` +
  `You have set source_entry_path to '/' with nested_entries enabled.\n` +
  `This would create webpack entry points for EVERY file in your source directory,\n` +
  `which would severely impact build performance.\n\n` +
  `To fix this issue, either:\n` +
  `1. Set 'nested_entries: false' in your shakapacker.yml\n` +
  `2. Change 'source_entry_path' to a specific subdirectory (e.g., 'packs')\n` +
  `3. Or use both options for better organization of your entry points`

Lines 66-75: Logic bug in entry object handling

// ❌ BAD (in this PR):
let previousPaths = entries[name]
if (previousPaths) {
  previousPaths = Array.isArray(previousPaths) ? previousPaths : [previousPaths]
  previousPaths.push(assetPaths)
  assetPaths = previousPaths
}
entries[name] = assetPaths

// ✅ GOOD (TypeScript version):
const previousPaths = entries[name]
if (previousPaths) {
  const pathArray = Array.isArray(previousPaths)
    ? previousPaths as string[]
    : [previousPaths as string]
  pathArray.push(assetPath)
  entries[name] = pathArray
} else {
  entries[name] = assetPath
}

The modified version has several problems:

1. Uses let and reassigns previousPaths - confusing and unnecessary
2. Variable name assetPaths (plural) is incorrect - should be assetPath (singular)
3. The logic flow is harder to follow

Line 116: Incorrect integrity check

// ❌ BAD:
crossOriginLoading: config.integrity.enabled
  ? config.integrity.cross_origin
  : false

// ✅ GOOD:
crossOriginLoading: config.integrity && config.integrity.enabled
  ? config.integrity.cross_origin
  : false

Missing the config.integrity && check will cause runtime errors if config.integrity is undefined.

🔍 Other Observations

1. webpack-merge Dependency Move

Moving webpack-merge from devDependencies to dependencies is correct since it's used in the package's runtime code (package/index.ts).

2. Dummy App Package Manager Switch

Switching from npm to yarn in spec/dummy/ is fine, but the PR description should explain why (likely for consistency or to test yarn specifically).

3. Version Ranges

The peer dependency version ranges are appropriately broad (e.g., ^8.2.4 || ^9.0.0 || ^10.0.0 for babel-loader), providing good forward compatibility.

4. CI Configuration

Updated .github/workflows/test-bundlers.yml to use yarn consistently - good for test reliability.

📋 Recommendations

Must Fix:

1. Revert package/environments/base.js changes - The TypeScript version compiles to correct JavaScript. The manual edits introduced bugs and should be removed.
2. Verify the build process - Ensure base.js is generated from base.ts via yarn build and not manually edited.

Should Fix:

3. Add a note in the PR description explaining the npm → yarn switch in spec/dummy/
4. Consider adding a CI check that verifies package/environments/base.js matches the compiled output of base.ts

Nice to Have:

5. Add examples in the documentation for CSS-only projects (no transpiler needed)
6. Consider adding a "doctor" command enhancement that suggests which peer dependencies to install based on the shakapacker.yml configuration

🏁 Conclusion

This PR solves a real problem with an elegant solution. The peer dependencies approach is well-executed with excellent documentation and test coverage. However, the manual modifications to package/environments/base.js introduce bugs and must be fixed before merging.

Recommendation: Request changes to fix the base.js regression, then approve.

Files to fix:

• package/environments/base.js - Revert manual changes, regenerate from TypeScript

🤖 Generated with Claude Code

Code Review - PR #615: Restore Optional Peer Dependencies

Summary

This PR implements a well-designed solution to restore peer dependencies while marking them as optional via peerDependenciesMeta. The implementation is solid and addresses the core issue effectively.

✅ Strengths

1. Excellent Architecture Decision

• Using peerDependenciesMeta with optional: true is the correct modern approach
• Allows version constraints to be documented without forcing installations
• Eliminates confusing warnings while maintaining compatibility enforcement

2. TypeScript Type-Only Imports

• Properly uses import type syntax to prevent runtime module resolution
• Type-only imports are correctly erased during compilation
• Includes clear @ts-ignore comments explaining why they're needed
• Example: package/environments/base.ts:8-9, package/index.ts:7-8

3. Comprehensive Documentation

• New docs/optional-peer-dependencies.md provides excellent guidance
• Clear configuration examples for different setups (webpack+babel, webpack+swc, rspack+swc)
• Updated README.md and docs/v9_upgrade.md with migration guidance
• Added contributing guidelines for testing peer dependencies

4. Version Ranges

• Broad but sensible version ranges (e.g., ^5.76.0 for webpack)
• Multiple major version support for evolving packages (e.g., babel-loader ^8.2.4 || ^9.0.0 || ^10.0.0)
• Good forward compatibility planning

5. CI/CD Updates

• Switched from npm to yarn consistently across workflows
• Proper cache configuration with yarn.lock

⚠️ Issues & Concerns

1. Critical: webpack-merge Dependency Change

Issue: webpack-merge moved from devDependencies to dependencies (package.json:42)

Analysis:

• The package uses webpack-merge at runtime in package/index.ts:4 and exports it
• This change is correct - it was incorrectly in devDependencies before
• Users who don't have webpack installed still need webpack-merge to work

Recommendation: ✅ This is the right fix, but should be explicitly called out in the PR description and release notes as it increases the installed package size slightly.

2. Potential Issue: Compiled JavaScript in PR

Observation: The PR includes compiled package/environments/base.js with formatting changes

Concerns:

• The compiled JS output formatting differs from the TypeScript source
• Line 1 shows "use strict"; which is fine, but overall formatting seems mechanically reformatted
• Could indicate inconsistent TypeScript compiler settings

Recommendation:

• Verify that tsconfig.json settings are consistent
• Consider adding yarn build to pre-commit hooks
• Document if compiled files should be committed

3. Missing: @swc/core Peer Dependency

Issue: The documentation extensively mentions SWC (@swc/core), but I don't see it in peerDependencies

Impact: Users following the docs won't get version constraint enforcement for @swc/core

Recommendation: Add to package.json:

"@swc/core": "^1.3.0",

And mark it optional in peerDependenciesMeta.

4. Documentation: Missing webpack-merge in Examples

Issue: The configuration examples in README.md and docs don't mention webpack-merge

Impact: Users might be confused why it's installed as a direct dependency

Recommendation: Add a note explaining that webpack-merge is always included as a core dependency for configuration merging.

5. Test Coverage Gap

Question: Are there tests validating the package works without optional dependencies installed?

Recommendation: Consider adding integration tests that:

• Install shakapacker without webpack (rspack-only scenario)
• Install shakapacker without babel (swc-only scenario)
• Verify no runtime errors when optional packages are missing

🔒 Security Assessment

✅ No security concerns identified

• No credentials or secrets added
• Dependency version ranges are reasonable and not overly permissive
• No execution of arbitrary code
• Type-only imports prevent accidental runtime dependencies

🎯 Performance Considerations

✅ Positive Impact

• Users can now install only what they need (smaller node_modules)
• Rspack users can skip webpack and babel entirely
• SWC users can skip babel entirely
• Estimated savings: 50-100MB+ depending on configuration

📋 Testing Verification

From PR description:

• ✅ npm install tested - no warnings
• ✅ yarn install tested - no warnings
• ✅ TypeScript builds successfully
• ✅ RSpec tests pass
• ✅ RuboCop passes

Additional Testing Recommended:

• Test with pnpm
• Test fresh install in rspack-only mode (no webpack)
• Test fresh install in swc-only mode (no babel)

🎨 Code Quality

✅ Good Practices:

• Clear comments explaining non-obvious patterns
• Consistent naming conventions
• Follows existing codebase patterns
• Type safety maintained with TypeScript

⚠️ Minor Style Issues:

• Compiled JS has different formatting than source (see Issue Bump ini from 1.3.5 to 1.3.8 #2 above)

📝 Final Recommendations

Must Address Before Merge:

1. Add @swc/core to peerDependencies (mentioned in docs but missing)
2. Verify compiled JS formatting is intentional and consistent

Should Address Before Merge:

3. Document webpack-merge as a core dependency in the migration guide
4. Add release notes explicitly calling out that webpack-merge is now a direct dependency

Nice to Have:

5. Add integration tests for scenarios without optional dependencies
6. Test with pnpm in CI
7. Consider adding a shakapacker doctor command to validate peer dependency setup

✨ Overall Assessment

Status: ✅ Approve with Minor Changes

This is a well-executed PR that solves a real pain point for users. The architecture is sound, the documentation is excellent, and the implementation follows best practices. The issues identified are minor and can be addressed quickly.

The optional peer dependencies approach will significantly improve the developer experience and reduce confusion around installation warnings.

Great work on this PR! The design rationale is solid and the execution is thorough. Once the @swc/core peer dependency is added, this will be ready to merge.

🤖 Review generated by Claude Code

Code Review: Optional Peer Dependencies Implementation

Summary

This PR successfully implements optional peer dependencies for Shakapacker, providing a clean solution to reduce installation warnings while maintaining version constraints. The implementation is well-structured with comprehensive documentation and testing.

Code Quality & Best Practices

Strengths

1. Well-documented approach with comprehensive docs/optional-peer-dependencies.md
2. Comprehensive test coverage: Two RSpec test suites plus shell script for cross-package-manager validation
3. TypeScript type safety: Proper use of import type with @ts-ignore comments prevents runtime errors
4. Minimal breaking changes: Existing users can upgrade seamlessly

Minor Concerns

1. webpack-merge moved to dependencies but base.ts:5 still has @ts-ignore (not needed for direct deps)
2. package/environments/base.js (compiled TypeScript) is committed - verify this is intentional

Potential Issues

Critical: None identified

Medium Priority

1. CI Migration from npm to yarn: Ensure all CI environments have yarn. Document why yarn was chosen.
2. Doctor command: Test at doctor_optional_peer_spec.rb:238 has skip - needs follow-up to recognize webpack-merge as direct dependency

Low Priority

1. Version range breadth: esbuild supports 10 major versions. Consider narrowing or adding CI matrix testing for boundary versions.

Performance Considerations

• Positive: Smaller node_modules, faster installation, no runtime changes

Security Concerns

• None identified
• Recommendation: Consider dependabot for monitoring peer dependency versions

Test Coverage

Excellent Coverage

• Package structure validation
• Installation without warnings (npm, yarn, pnpm)
• Loading shakapacker without optional deps
• TypeScript type-only import verification
• Doctor command with various dependency combinations
• Conflict detection

Missing Tests

1. Integration test: Building a project with minimal dependencies
2. Version constraint violations when packages ARE installed
3. Migration path from v8 to v9

Documentation Quality

Excellent documentation including README updates, new optional-peer-dependencies.md, v9_upgrade.md updates, and CONTRIBUTING.md updates.

Suggestion: Add troubleshooting section for package manager cache issues.

Recommendations

1. Address the Doctor command skip (line 238 in doctor_optional_peer_spec.rb)
2. Remove @ts-ignore for webpack-merge since it's now a direct dependency
3. Document the CI yarn migration
4. Add integration test that builds a minimal project
5. Document package manager cache clearing in upgrade guide

Final Verdict

APPROVE with minor suggestions - This is a well-executed PR that solves installation warnings while maintaining backward compatibility. Minor issues are low-priority improvements rather than blockers.

Great work!

Review generated with assistance from Claude Code

Code Review - PR #615: Optional Peer Dependencies

I've completed a comprehensive review of this PR. Overall, this is a well-executed implementation that solves a real pain point. Here's my detailed feedback:

✅ Strengths

1. Excellent Problem-Solution Fit

   • The use of peerDependenciesMeta is the correct approach for optional peer dependencies
   • Solves the warning noise issue while maintaining version constraints
   • Aligns with npm/yarn/pnpm best practices

2. Comprehensive Documentation

   • Added detailed docs in docs/optional-peer-dependencies.md
   • Updated README.md with clear configuration examples
   • Migration guide in docs/v9_upgrade.md is thorough
   • CONTRIBUTING.md includes testing guidance

3. Good Type Safety Approach

   • Using type-only imports (import type) prevents runtime errors
   • @ts-ignore comments are well-justified and documented

4. CI/CD Updates

   • Switched from npm to yarn consistently across workflows
   • Appropriate for testing the peer dependency changes

⚠️ Issues & Concerns

Critical: Inconsistency Between Source Files

Problem: The PR shows a discrepancy between TypeScript source and compiled JavaScript:

In package/environments/base.ts (line 8):

import { Configuration, Entry } from "webpack"

But in package/environments/base.js (the compiled output in the diff):

// @ts-ignore: webpack is an optional peer dependency (using type-only import)
import type { Configuration, Entry } from "webpack"

However, when I checked the current package/environments/base.ts file, it shows:

import { Configuration, Entry } from "webpack"  // Line 8 - NO @ts-ignore, NO type-only

This is problematic because:

• The TypeScript source doesn't have the @ts-ignore comment or type keyword
• The compiled JS appears to have it (based on the diff)
• This suggests either:
  • The source files weren't properly updated before compilation
  • The build script is modifying imports (which would be unusual)
  • The diff is showing changes from a different branch state

Recommendation:

// package/environments/base.ts (line 8)
// @ts-ignore: webpack is an optional peer dependency (using type-only import)
import type { Configuration, Entry } from "webpack"

Same issue exists in package/index.ts (line 7) - needs the same fix.

Medium: Missing webpack-merge from package.json

Problem: The PR moves webpack-merge from devDependencies to dependencies in package.json, which is correct since it's imported in package/index.ts. However, I notice that the current package.json shows:

"dependencies": {
  "js-yaml": "^4.1.0",
  "path-complete-extname": "^1.0.0"
}

webpack-merge is NOT present in dependencies! But the diff shows it being added. This needs verification.

Recommendation:

• Verify webpack-merge is indeed in the dependencies section after the changes
• It should be version ^5.8.0 based on the diff

Medium: Test Coverage Gaps

The PR mentions these test files in the description:

• spec/shakapacker/optional_dependencies_spec.rb
• spec/shakapacker/doctor_optional_peer_spec.rb
• test/peer-dependencies.sh

Problem: These files don't exist in the repository (based on my glob searches).

Recommendations:

1. Add RSpec tests to verify:

   • peerDependencies and peerDependenciesMeta structure
   • All peer deps are marked as optional
   • No missing entries between the two objects

2. Add installation test script (test/peer-dependencies.sh):

   #!/bin/bash
   # Test that installation produces no peer dependency warnings
   
   for pm in npm yarn pnpm; do
     echo "Testing with $pm..."
     tmp_dir=$(mktemp -d)
     cd "$tmp_dir"
     $pm init -y
     $pm add /path/to/shakapacker 2>&1 | grep -i "peer" && exit 1
   done
   echo "✅ All package managers: no peer dependency warnings"

Minor: Documentation Clarity

In docs/optional-peer-dependencies.md:

Line 47 - The comment could be clearer:

// Better phrasing:
// @ts-ignore: webpack is an optional peer dependency - type-only import prevents runtime errors
import type { Configuration } from "webpack"

Minor: Missing .eslintignore Update Rationale

The diff shows adding package/* to .eslintignore. While this is likely correct (to ignore compiled JS from TS), the PR description doesn't mention it.

Recommendation: Verify this is intentional and not masking linting issues in the compiled code.

🔍 Code Quality

• ✅ Follows project conventions (per CLAUDE.md)
• ✅ Files end with trailing newlines
• ✅ Version ranges are reasonable and well-chosen
• ✅ No security concerns identified
• ✅ No performance regressions expected

🧪 Testing Recommendations

Before merging, verify:

1. Build Process:

   yarn clean:ts
   yarn build
   # Verify compiled files have @ts-ignore comments

2. Installation Tests:

   # Test with each package manager
   npm install --dry-run
   yarn install --dry-run
   pnpm install --dry-run

3. RuboCop & Linting:

   bundle exec rubocop
   yarn lint

4. Integration Tests:

   • Test with webpack config
   • Test with rspack config
   • Test with minimal dependencies (only what's needed)

📋 Action Items

1. Fix TypeScript source files - Add @ts-ignore and type keyword to:

   • package/environments/base.ts:8
   • package/index.ts:7

2. Verify webpack-merge - Ensure it's in dependencies after changes

3. Add test coverage:

   • RSpec tests for package.json structure
   • Shell script for installation warnings

4. Rebuild - Run yarn build after TS source fixes

5. Run full test suite - Ensure no regressions

🎯 Overall Assessment

Status: Approve with requested changes

This PR is architecturally sound and solves a real problem in the right way. The main issues are:

• Source file inconsistencies (TypeScript vs compiled JS)
• Missing test coverage for the new functionality

Once the TypeScript source files are fixed and tests are added, this will be ready to merge.

Estimated effort to address: 30-60 minutes

Great work on the comprehensive documentation! The examples and migration guide will be very helpful for users.

🤖 Review by Claude Code