Fix SWC migration to use config/swc.config.js instead of .swcrc (#634)

[justin808]

Summary

Fixes #634 - The rake shakapacker:migrate_to_swc task now creates config/swc.config.js instead of .swcrc, fixing three critical issues that made the migration task broken for most users.

Changes Made

1. Configuration File Change

• Changed: Migration now creates config/swc.config.js instead of .swcrc
• Why: .swcrc completely overrides Shakapacker's default SWC settings, bypassing webpack-merge
• Benefit: config/swc.config.js properly merges with Shakapacker's defaults, preserving important settings like loose: true, env.coreJs: 3, source maps, and dynamic parser configuration

2. ESLint Dependencies Preserved

• Split: BABEL_PACKAGES into safe-to-remove packages and ESLint-related packages (ESLINT_BABEL_PACKAGES)
• Preserved: @babel/core and @babel/eslint-parser when ESLint uses them
• Why: Many projects (like those using eslint-config-shakacode) require these packages for ESLint to work
• Improved: Warning messages now explain how to migrate ESLint parser if desired

3. Doctor Check Updates

• Added: Warning when .swcrc file is detected (anti-pattern)
• Removed: Obsolete jsc.target/env conflict checks (no longer generated)
• Added: Info message when using recommended config/swc.config.js

4. Documentation Updates

• Updated: transpiler-migration.md to show config/swc.config.js instead of .swcrc
• Added: Important note explaining .swcrc vs config/swc.config.js difference
• Fixed: Decorator example to use recommended config file format

Testing

• ✅ All RSpec tests updated and passing
• ✅ RuboCop clean
• ✅ JavaScript linting clean
• ✅ Both SwcMigrator and Doctor specs updated

Breaking Changes

None - this is a bug fix that makes the migration task actually work correctly.

Migration Path

Existing users with .swcrc files will:

1. Get a warning from shakapacker:doctor to migrate to config/swc.config.js
2. Can manually migrate by moving their custom settings to config/swc.config.js
3. New migrations will automatically use the correct file

🤖 Generated with Claude Code

[coderabbitai]

Warning

Rate limit exceeded

@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 46 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 4fe4c30 and f477fde.

📒 Files selected for processing (5)

• docs/transpiler-migration.md (6 hunks)
• lib/shakapacker/doctor.rb (1 hunks)
• lib/shakapacker/swc_migrator.rb (10 hunks)
• spec/shakapacker/doctor_spec.rb (3 hunks)
• spec/shakapacker/swc_migrator_spec.rb (10 hunks)

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch justin808/swc-config-fix

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

Code Review - PR #635: Fix SWC migration to use config/swc.config.js

Summary

This is an excellent bug fix that addresses a critical issue with the SWC migration task. The PR correctly identifies and fixes three major problems with the migration flow. The implementation is solid and well-tested.

✅ Strengths

1. Well-Designed Solution

• Moving from .swcrc to config/swc.config.js is the right approach. The .swcrc file indeed completely overrides Shakapacker defaults, which breaks the webpack-merge functionality
• The new JavaScript config format allows proper merging with Shakapacker's base configuration, preserving important defaults like loose: true, env.coreJs: 3, and source maps

2. Smart ESLint Handling

• The separation of BABEL_PACKAGES and ESLINT_BABEL_PACKAGES is well-thought-out (lines 13-33 in swc_migrator.rb)
• Preserving @babel/core and @babel/eslint-parser when ESLint uses them prevents breaking user configurations
• The warning messages provide clear, actionable guidance for users who want to migrate their ESLint parser

3. Comprehensive Testing

• All RSpec tests updated to reflect the new behavior
• Test coverage includes edge cases (ESLint preservation, existing config files, etc.)
• Both SwcMigrator and Doctor specs properly updated

4. Excellent Documentation

• transpiler-migration.md updated with JavaScript config example instead of JSON
• Important callout explaining the difference between .swcrc and config/swc.config.js
• Examples use proper module.exports syntax

5. Good Code Quality

• Follows Ruby conventions and existing patterns in the codebase
• Clear comments explaining the reasoning (e.g., line 11-12)
• Proper error handling throughout
• Clean separation of concerns

🔍 Observations & Minor Suggestions

1. Code Style ✅

The whitespace-only changes (lines with trailing spaces removed) follow the project's linting requirements - good attention to detail!

2. Doctor Check Logic (lib/shakapacker/doctor.rb:443-456)

The new logic is simpler and more effective than the old jsc.target/env conflict checks. The old checks were trying to parse .swcrc and detect conflicts, but since the migration no longer generates those problematic fields, removing that logic makes sense.

Suggestion: The warning message at line 448-450 is quite long. Consider if it could be split into multiple messages for better readability, though the current approach is acceptable.

3. Migration Path for Existing Users

The PR description mentions existing users will get warnings from shakapacker:doctor, which is good. However, it might be helpful to add a note in the CHANGELOG.md about this breaking change pattern (users with .swcrc should migrate to config/swc.config.js).

Question: Should there be a task like shakapacker:migrate_swcrc_to_config to help existing users automatically migrate their .swcrc settings to config/swc.config.js? This could be a follow-up PR if needed.

4. DEFAULT_SWC_CONFIG Format (lib/shakapacker/swc_migrator.rb:49-63)

The heredoc format for the JavaScript config is clean. The comment at the top is helpful.

Nitpick: The comment references the SWC docs at line 52. Consider adding a note about what settings Shakapacker provides by default so users know what they can safely override vs. what's already configured.

5. Test Assertions (spec/shakapacker/swc_migrator_spec.rb:77-79)

The test reads the generated file and checks for string content. This is pragmatic and works well.

config_content = File.read(root_path.join("config/swc.config.js"))
expect(config_content).to include("module.exports")
expect(config_content).to include('runtime: "automatic"')

This is fine, though theoretically the test could be more robust by actually require-ing the config and checking its structure. However, for this use case, string matching is perfectly adequate.

🐛 Potential Issues

1. Minor: Directory Creation (lib/shakapacker/swc_migrator.rb:274)

FileUtils.mkdir_p(config_dir) unless config_dir.exist?

This is fine, but mkdir_p already handles the case where the directory exists (it's a no-op), so the conditional check is redundant. You could simplify to:

FileUtils.mkdir_p(config_dir)

2. ESLint Detection Heuristic (lib/shakapacker/swc_migrator.rb:165-199)

The comment at lines 167-169 correctly notes this is a heuristic that may have false positives. The false positive bias is the right choice here (better to preserve packages unnecessarily than break ESLint).

However, the string matching at line 176 could potentially match these strings in comments:

return true if content.match?(/@babel\/eslint-parser|babel-eslint/)

Suggestion: While false positives are acceptable, you could reduce them by:

• Checking for "parser" key proximity in config files
• Looking for actual require/import statements

That said, this is probably premature optimization - the current approach is pragmatic and safe.

🔒 Security Considerations

No security concerns identified. The code:

• ✅ Properly validates file paths using Pathname
• ✅ Handles JSON parsing errors gracefully
• ✅ Uses safe file operations
• ✅ Doesn't execute user-provided code

⚡ Performance Considerations

No performance concerns. All operations are file I/O bound and occur during migration (not runtime). The migration is already an infrequent operation.

📊 Test Coverage Assessment

Excellent test coverage:

• ✅ Migration creates correct config file
• ✅ Doesn't overwrite existing config
• ✅ ESLint detection and package preservation
• ✅ Package removal logic
• ✅ Doctor check warnings for .swcrc
• ✅ Doctor check info for config/swc.config.js
• ✅ Error handling scenarios

The test suite properly validates all the new behavior.

📝 Documentation Quality

Very good:

• ✅ Updated transpiler-migration.md with correct examples
• ✅ Clear explanations of .swcrc vs config/swc.config.js
• ✅ Code comments explain the "why" behind decisions
• ✅ Warning messages provide actionable guidance

Final Verdict

✅ Approve with minor suggestions

This is a well-crafted bug fix that solves real problems users are experiencing. The code quality is high, testing is comprehensive, and the documentation is clear. The minor suggestions above are truly minor and don't block merging.

The separation of ESLint-related Babel packages is particularly thoughtful - it shows understanding of real-world usage patterns where projects use Babel for ESLint but SWC for transpilation.

Recommendation: Merge after addressing the directory creation redundancy (if desired) and considering whether to add CHANGELOG notes about the migration path for existing .swcrc users.

Great work! 🎉

---

Review conducted following Shakapacker's CLAUDE.md guidelines

[claude]

Code Review for PR #635: Fix SWC Migration Configuration

✅ Overall Assessment

This is a well-executed bug fix that addresses critical issues in the SWC migration task. The changes are thoughtful, well-tested, and properly documented.

---

🎯 Strengths

1. Problem Identification & Solution

• Excellent root cause analysis: Correctly identified that .swcrc completely overrides Shakapacker's defaults (bypassing webpack-merge)
• Smart solution: Using config/swc.config.js instead allows proper merging with defaults
• Preserves critical settings: loose: true, env.coreJs: 3, source maps, and dynamic parser config

2. ESLint Dependencies Handling

• Smart preservation logic: Keeps @babel/core and @babel/eslint-parser when ESLint uses them
• Clear user guidance: Warning messages explain migration options without breaking existing setups
• No breaking changes: Users with eslint-config-shakacode won't have their linting broken

3. Code Quality

• Clean separation of concerns: BABEL_PACKAGES vs ESLINT_BABEL_PACKAGES makes intent clear
• Proper return values: All methods return consistent, meaningful hashes with appropriate keys
• Error handling: Graceful degradation with helpful error messages
• Follows project conventions: Uses bundle exec, trailing newlines, existing patterns

4. Test Coverage ✅

• All RSpec tests updated to reflect new behavior
• Tests cover edge cases (existing config, ESLint detection, package preservation)
• Both positive and negative test cases included
• Doctor checks updated appropriately

5. Documentation

• transpiler-migration.md updated with correct config file
• Important callout box explaining .swcrc vs config/swc.config.js difference
• Decorator example uses recommended config format

---

🔍 Potential Issues & Suggestions

1. Minor: Inconsistent Constant Definition (Low Priority)

Location: lib/shakapacker/swc_migrator.rb:11-26

The diff shows @babel/core being removed from BABEL_PACKAGES, but I see it's still defined on line 12 in the current code. The constant definition looks correct in the PR diff though, so this appears to be resolved in the actual changes.

2. Doctor Check Could Be More Actionable

Location: lib/shakapacker/doctor.rb:443-455

The new warning for .swcrc is great, but consider adding a code example or command:

@warnings << "SWC configuration: .swcrc file detected. This file completely overrides Shakapacker's default SWC settings and may cause build failures. " \
            "Please migrate to config/swc.config.js which properly merges with Shakapacker defaults. " \
            "Run 'rake shakapacker:migrate_to_swc' to update your config. " \
            "See: https://github.com/shakacode/shakapacker/blob/main/docs/using_swc_loader.md?target=https://github.com"

3. Config File Format Consideration

Location: lib/shakapacker/swc_migrator.rb:49-62

The default config uses ES6 module syntax (module.exports), but could also support ESM format for future compatibility:

// Alternative ESM format (if supported by the loader)
export default {
  jsc: {
    transform: {
      react: { runtime: "automatic" }
    }
  }
}

However, module.exports is the safer choice for now, so this is fine.

4. ESLint Detection Heuristic

Location: lib/shakapacker/swc_migrator.rb:157-191

The ESLint detection using regex is reasonable, but has a known limitation (noted in comments). Consider adding a check for parser field in ESLint config:

# Check for parser field in ESLint config
if package_json["eslintConfig"] && package_json["eslintConfig"]["parser"]
  parser = package_json["eslintConfig"]["parser"]
  return true if parser.match?(/@babel\/eslint-parser|babel-eslint/)
end

---

🔒 Security Considerations

✅ No security concerns identified

• No user input is directly executed
• File operations use safe paths (Pathname)
• JSON parsing has proper error handling
• No SQL/command injection risks

---

⚡ Performance Considerations

✅ Performance is appropriate

• File operations are minimal and necessary
• JSON parsing is efficient
• No unnecessary loops or redundant operations
• Migration is a one-time operation

---

📊 Test Coverage

✅ Excellent test coverage

• All modified methods have corresponding tests
• Edge cases covered (existing files, ESLint configs, missing files)
• Both SwcMigrator and Doctor specs updated
• Tests verify behavior, not just implementation

---

🎯 Recommendations

Must-Have (Before Merge):

✅ All items already addressed in the PR

Nice-to-Have (Future Improvements):

1. Add migration guide for existing .swcrc users in a separate doc
2. Consider a --force flag for clean_babel_packages to override ESLint preservation
3. Add a --dry-run option to preview changes before applying

---

📝 Final Verdict

LGTM 🚀

This PR successfully fixes a critical bug in the migration task while being backwards compatible and user-friendly. The code is clean, well-tested, and properly documented. The approach of preserving ESLint dependencies is smart and prevents breaking user setups.

Recommendation: ✅ Approve and merge

---

Great work on this fix, @justin808! The attention to detail in handling edge cases (especially ESLint) shows excellent engineering judgment.

[claude]

Pull Request Review: Fix SWC Migration (#635)

Summary

This PR addresses a critical issue where the SWC migration task was creating .swcrc files that completely override Shakapacker's defaults, breaking the webpack-merge functionality. The fix changes the migration to use config/swc.config.js instead, which properly merges with defaults.

---

Code Quality & Best Practices

Excellent Practices

1. Proper separation of concerns: The new ESLINT_BABEL_PACKAGES constant cleanly separates ESLint-related packages from general Babel packages
2. Comprehensive testing: All specs have been updated to reflect the new behavior with appropriate test coverage
3. Backward compatibility: The doctor check appropriately warns users with existing .swcrc files rather than breaking their setup
4. Clear user communication: Warning messages now provide actionable migration steps

Suggested Improvements

1. Missing trailing newline verification

Per CLAUDE.md requirements, all files must end with a trailing newline. Please verify all modified files comply.

2. Potential issue in clean_babel_packages return value

In lib/shakapacker/swc_migrator.rb:111, the early return hash doesn't match the new signature at line 134. Consider adding preserved_packages key for consistency.

3. Inconsistent ESLint detection behavior

The eslint_uses_babel? method has good heuristics, but the warning message could be more specific about where the detection occurred.

---

Potential Bugs

1. find_babel_packages shows packages that won't be removed

At lib/shakapacker/swc_migrator.rb:153, the method now includes ESLint packages in the results. This means cleanup recommendations will suggest removing @babel/core and @babel/eslint-parser even though they'll be preserved for ESLint. Consider filtering the display to only show packages that will actually be removed.

2. Test expectations may need clarification

The test at line 277 has a comment saying it removes babel packages but not @babel/core or @babel/eslint-parser, but the test context doesn't set up ESLint, so those packages would actually be removed. The comment and test expectations should align.

---

Test Coverage

Excellent Coverage

• All existing tests updated for new config/swc.config.js behavior
• Doctor spec updated to detect .swcrc anti-pattern
• ESLint preservation logic tested
• Error handling maintained

Missing Coverage

1. No explicit test for the preserved_packages return value - Tests verify preservation behavior but don't assert on the returned preserved_packages array
2. Edge case: Config directory creation is handled but not explicitly tested
3. Integration test: Consider verifying that the generated config/swc.config.js actually merges correctly

---

Action Items

Before Merge

• Fix return value consistency in clean_babel_packages early return (line 111)
• Verify trailing newlines on all modified files
• Run bundle exec rubocop to ensure Ruby linting passes
• Run yarn lint to ensure JavaScript linting passes
• Consider improving find_babel_packages to not suggest removing preserved packages

Nice to Have

• Add explicit test for preserved_packages in return value
• Add more specific context to ESLint detection warning messages

---

Conclusion

This is a high-quality PR that fixes a critical bug in the SWC migration task. The approach is sound, the code is well-structured, and the test coverage is comprehensive. The issues identified above are minor and mostly relate to edge cases and consistency.

Recommendation: Approve with minor revisions

The PR successfully addresses the core issue and adds valuable functionality. The documentation updates are clear and helpful.

Great work!

[claude]

Code Review - PR #635: Fix SWC migration to use config/swc.config.js

✅ Overall Assessment

This is an excellent bug fix that addresses a critical issue with the SWC migration task. The changes are well-structured, thoroughly tested, and properly documented.

---

🎯 Code Quality & Best Practices

Strengths:

• Clean separation of concerns: The split between BABEL_PACKAGES and ESLINT_BABEL_PACKAGES is well-designed and makes the intent clear
• Proper error handling: All file operations have appropriate error handling with informative logging
• Consistent code style: Follows Ruby and JavaScript conventions throughout
• Good use of constants: DEFAULT_SWC_CONFIG as a heredoc is more readable than nested JSON
• Backward compatibility: Properly handles existing configurations without breaking changes

Minor observations:

• The eslint_uses_babel? method in lib/shakapacker/swc_migrator.rb:174 has good defensive coding with the parser field check before the broader JSON check - this is excellent
• File operations consistently use FileUtils.mkdir_p before writing, which is good practice

---

🐛 Potential Bugs/Issues

No critical bugs found, but some considerations:

1. ESLint detection logic (lib/shakapacker/swc_migrator.rb:157-191):

   • The heuristic-based detection is sensible and the comment explains the tradeoff well
   • Consider: What if a project has @babel/eslint-parser installed but does not use it? The preserved packages would be unnecessary but harmless
   • ✅ This is acceptable given the conservative approach

2. File creation race condition (lib/shakapacker/swc_migrator.rb:270-287):

   • The create_swc_config method checks if the file exists, but there is a small window between check and write
   • ✅ Not a real concern for this use case (single-threaded rake task)

3. JSON parsing (lib/shakapacker/doctor.rb:448):

   • The old implementation had JSON parsing with error handling that is now removed since .swcrc is discouraged
   • ✅ This is correct - no need to parse a deprecated file format

---

⚡ Performance Considerations

• Doctor checks: The new check_swc_config_conflicts method is more efficient - it only checks file existence instead of parsing JSON
• File operations: All file I/O is appropriately minimized
• ✅ No performance concerns

---

🔒 Security Considerations

• File path operations: All use Pathname#join which prevents path traversal
• JSON parsing: Removed potentially unsafe JSON parsing of .swcrc
• File writes: Use File.write with proper escaping via heredoc
• ✅ No security concerns identified

---

🧪 Test Coverage

Excellent test coverage:

1. SwcMigrator specs (spec/shakapacker/swc_migrator_spec.rb):

   • ✅ Tests config file creation with proper content validation
   • ✅ Tests ESLint package preservation with multiple scenarios
   • ✅ Tests edge cases (missing files, existing configs)
   • ✅ Updated all assertions to match new behavior

2. Doctor specs (spec/shakapacker/doctor_spec.rb):

   • ✅ Tests .swcrc warning
   • ✅ Tests config/swc.config.js info message
   • ✅ Removed obsolete jsc.target conflict tests

3. Coverage completeness:

   • Package preservation logic: ✅ Covered
   • File creation: ✅ Covered
   • ESLint detection: ✅ Covered
   • Error paths: ✅ Covered

Suggestions:

• Consider adding a test for the case where ESLint parser is in package.json eslintConfig.parser field specifically (though the broader check already catches this)

---

📋 Additional Observations

Documentation:

• ✅ docs/transpiler-migration.md properly updated with clear warnings
• ✅ Code comments explain the "why" behind decisions
• ✅ Helpful user-facing messages guide migration

Breaking changes:

• ✅ None - this is truly a bug fix
• ✅ Existing .swcrc users get a helpful warning, not an error

Adherence to CLAUDE.md:

• ✅ All files end with trailing newlines
• ✅ Follows existing code conventions
• ✅ Tests updated appropriately
• ✅ Changes are focused and minimal

---

🎉 Recommendation

APPROVE - This PR is ready to merge. It:

• Fixes a critical bug that made the migration task unusable
• Includes comprehensive test coverage
• Has excellent documentation
• Maintains backward compatibility
• Follows all project conventions

The split between safe-to-remove and ESLint-required Babel packages is particularly elegant and shows good understanding of real-world usage patterns.

Great work! 🚀

[claude]

Code Review for PR #635

Summary

This PR fixes a critical bug in the SWC migration task. The code quality is excellent with comprehensive test coverage.

✅ Strengths

• Well-designed solution: Using config/swc.config.js allows proper merging with defaults
• Smart ESLint handling: Separating BABEL_PACKAGES and ESLINT_BABEL_PACKAGES prevents breaking ESLint
• Excellent test coverage: All edge cases covered including ESLint detection and package preservation
• Good UX: Clear warnings guide users to migrate from .swcrc
• Follows conventions: Trailing newlines, proper error handling, backward compatible

🔍 Minor Suggestions

1. ESLint Detection (lib/shakapacker/swc_migrator.rb:190)

Consider adding a comment explaining why .to_json.match? is used (to catch nested configs).

2. Doctor Warning (lib/shakapacker/doctor.rb:448-451)

The 4-line concatenated warning might not wrap well on some terminals. Consider using newlines for better readability.

3. Test Clarity (spec/shakapacker/swc_migrator_spec.rb:277-286)

Test description could clarify that @babel/core is now in ESLINT_BABEL_PACKAGES constant.

🔒 Security: ✅ No concerns

⚡ Performance: ✅ No concerns

🐛 Bugs: ✅ None identified

📚 Documentation: ✅ Excellent

Final Verdict

✅ APPROVED - High-quality bug fix. Suggestions above are minor and don't block merging.

Great work! 🎉