Fix ReScript deprecation warnings #669
Conversation
WalkthroughUpdates build configuration by switching module type in bsconfig.json to esmodule. Minor UI code tweak in CommentForm: renames an onClick handler parameter to a discarded variable without altering dispatch logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (2)
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 |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
|
/deploy-review-app |
Pull Request Review: Migrate from Babel to SWC transpilerSummaryThis PR successfully migrates the project from Babel to SWC, modernizing the build toolchain and aligning with Shakapacker 9.0 defaults. The migration is well-executed with comprehensive configuration updates and dependency management. Strengths1. Comprehensive Migration
2. Configuration Quality
3. Documentation
Issues and RecommendationsCritical: Configuration InconsistencyIssue: .swcrc and config/swc.config.js have conflicting React refresh settings:
Impact: This creates ambiguity about which config is used when. If both are present, SWC may prioritize one over the other unpredictably. Recommendation:
Performance: Missing jsc.looseLocation: .swcrc Issue: config/swc.config.js:17 has loose: true but .swcrc doesn't. Impact: Loose mode generates more performant code. The inconsistency may cause different transpilation behavior. Recommendation: Add loose: true to .swcrc if it's being used, or remove .swcrc entirely. Code Quality: ReScript Generated FileLocation: client/app/bundles/comments/rescript/RescriptShow/ror_components/RescriptShow.jsx Issue: This appears to be a ReScript-generated file (line 1: Generated by ReScript, PLEASE EDIT WITH CARE). The 201-line change shows reformatting (quote style changes) but no logical changes. Questions:
Minor: Code Style InconsistenciesObservation: The PR removes trailing newlines from several webpack config files and changes quote styles in server-bundle.js. These are unrelated to the Babel to SWC migration and appear to be incidental formatting changes. Security ConsiderationsNo security concerns identified:
Test CoveragePositive:
Concerns:
Performance ConsiderationsPositive:
Questions:
Recommendation: Add before/after metrics to PR description if available. Best Practices Assessment
RecommendationStatus: Approve with Changes Requested This is a solid migration that modernizes the build toolchain. The main blocker is the configuration inconsistency between .swcrc and config/swc.config.js. Once resolved, this should be safe to merge. Required Changes:
Suggested Improvements:
Great work on this migration! SWC is a significant improvement over Babel for build performance. |
justin808
force-pushed
the
justin808/migrate-to-swc
branch
from
64b0ff4 to
91acf43
Compare
|
✅ Deployment complete for PR #669, commit 91acf43 |
PR Review: Migrate from Babel to SWC transpilerSummaryThis PR successfully migrates the project from Babel to SWC for JavaScript transpilation. The migration is well-executed with comprehensive configuration updates and proper dependency management. ✅ Strengths1. Configuration Quality
2. Dependency Management
3. Testing Setup
4. ESLint Migration
|
|
✅ Deployment complete for PR #669, commit 91acf43 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
1-155: Remove remaining Babel-related package
@babel/runtimeis still listed in dependencies. Remove it (or provide a justification for its inclusion) to complete the cleanup outlined in issue #666.
🧹 Nitpick comments (1)
package.json (1)
34-34: No direct imports of @babel/runtime detected
All matches are confined toyarn.lock. If your SWC configuration provides the necessary runtime helpers, you can remove@babel/runtimefromdependencies. Verify that builds and helper functions remain correct without it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.eslintrc(1 hunks).swcrc(1 hunks)babel.config.js(0 hunks)config/shakapacker.yml(1 hunks)config/swc.config.js(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (1)
- babel.config.js
🔇 Additional comments (6)
config/swc.config.js (1)
1-32: Good environment-aware configuration.The use of environment variables for
development(line 13) andrefresh(line 14) settings is the correct approach, as it allows the configuration to adapt to different build contexts. This is superior to the hardcoded values in.swcrc.Additional features like
loose: true(line 17) andsourceMaps: true(line 22) are appropriate for the build pipeline.config/shakapacker.yml (1)
11-11: LGTM!The transpiler configuration correctly switches from Babel to SWC, aligning with the PR objectives.
.eslintrc (1)
10-16: LGTM!Switching to
espreeparser with ES2022 support is the correct approach after removing@babel/eslint-parser. The parser options correctly enable JSX and module syntax.package.json (2)
102-105: LGTM!The SWC dependencies (
@swc/core,@swc/jest,swc-loader) are added with appropriate versions for the migration.
151-151: LGTM!Jest transform correctly updated to use
@swc/jestinstead ofbabel-jest..swcrc (1)
1-33: Confirm SWC config usageNeither .swcrc nor config/swc.config.js are referenced by swc-loader in webpack configs or package.json – verify which file is actually loaded during your build to avoid divergent settings.
justin808
force-pushed
the
justin808/migrate-to-swc
branch
from
91acf43 to
e62c1ea
Compare
|
✅ Deployment complete for PR #669, commit e62c1ea |
PR Review: Migrate from Babel to SWC transpilerSummaryThis PR successfully migrates the project from Babel to SWC for JavaScript transpilation, aligning with Shakapacker 9.0 defaults. The changes are well-structured and the PR description is comprehensive. Overall, this is a solid migration with minimal issues found. ✅ Strengths
🔍 Code Quality Findingsconfig/swc.config.jsIssue 1: Hardcoded String Comparison (Minor) development: process.env.NODE_ENV === 'development',
refresh: process.env.WEBPACK_SERVE === 'true',Recommendation: The string comparison for Issue 2: Browser Targets Duplication (Low Priority) targets: {
browsers: ['>1%', 'last 5 versions', 'safari >= 7', 'Firefox ESR', 'not IE 11'],
}Observation: These targets are duplicated in Issue 3: CoreJS Mode Configuration (Review Needed) env: {
coreJs: 3,
mode: 'entry',
}Concern: Using
.eslintrcIssue 4: Parser Configuration (Positive Change) parser: espree
parserOptions:
ecmaVersion: 2022Observation: Switching from Issue 5: Missing Root Setting Explanation root: trueObservation: Adding 🐛 Potential Bugs1. React Fast Refresh ConfigurationIn refresh: process.env.WEBPACK_SERVE === 'true',Potential Issue: This enables React Fast Refresh only when
2. Loose Mode ImplicationsIn loose: true,Security/Correctness Concern: Loose mode trades spec compliance for performance/size. This can lead to subtle bugs where code behaves differently from standard JavaScript. Unless you're migrating from Babel with loose mode enabled (which should be verified), consider:
🚀 Performance ConsiderationsPositive: The migration to SWC should provide significant performance improvements (10-20x faster builds as noted). Suggestion: Consider adding build time metrics to your CI/CD to track and validate the performance improvements over time. 🔒 Security ConcernsNo Critical Issues Found Minor observations:
📊 Test CoveragePositive: The PR includes a comprehensive test plan covering:
Recommendation: Consider adding:
📝 Additional Recommendations1. Documentation UpdatesConsider updating the README or adding migration notes for other developers:
2. Missing .swcrc FileThe PR description mentions "Created
3. Dependency Version Specifications"@swc/core": "^1.3.100",
"@swc/jest": "^0.2.29",
"swc-loader": "^0.2.3",These versions are relatively old. Consider:
✅ Approval RecommendationRecommendation: Approve with Minor Changes This is a well-executed migration. The issues identified are minor and mostly around optimization and documentation. The core functionality appears sound. Before merging, please address:
Post-merge follow-ups:
Questions for Author
Great work on this migration! 🎉 Review generated with assistance from Claude Code |
justin808
force-pushed
the
justin808/migrate-to-swc
branch
from
e62c1ea to
683b1e9
Compare
|
✅ Deployment complete for PR #669, commit 683b1e9 |
Pull Request Review: Migrate from Babel to SWC transpilerOverall AssessmentThis is a well-executed migration that modernizes the build toolchain. The changes are thorough, well-documented, and the test plan demonstrates due diligence. ✅ Code Quality & Best Practices✅ Strengths
|
| Category | Rating | Notes |
|---|---|---|
| Code Quality | ⭐⭐⭐⭐ | Well-structured migration |
| Potential Bugs | i18n files concern needs clarification | |
| Performance | ⭐⭐⭐⭐⭐ | Excellent performance gains expected |
| Security | ✅ | No new security concerns |
| Test Coverage | Need to verify automated tests | |
| Documentation | ⭐⭐⭐ | Good PR docs, could improve project docs |
Overall: Approve with minor concerns addressed. The migration is solid, but please clarify the i18n files situation and verify the ESLint parser works correctly with all your modern JavaScript syntax.
Review generated with 🤖 attention to detail and developer empathy
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/swc.config.js (1)
1-32: SWC configuration suggestions
- react.runtime: 'automatic' — correct for React 19.
- loose: true — improves speed but may deviate from spec; test for subtle differences.
- decorators: false — no decorators detected in the codebase.
- env.mode: 'entry' — pulls in all core-js polyfills; consider
mode: 'usage'to trim bundle size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
.eslintignore(0 hunks).eslintrc(1 hunks).gitignore(1 hunks)babel.config.js(0 hunks)client/app/libs/i18n/default.js(1 hunks)client/app/libs/i18n/translations.js(1 hunks)config/shakapacker.yml(1 hunks)config/swc.config.js(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (2)
- .eslintignore
- babel.config.js
🚧 Files skipped from review as they are similar to previous changes (1)
- .eslintrc
🧰 Additional context used
🪛 GitHub Actions: JS CI
client/app/libs/i18n/translations.js
[error] 1-1: ESLint: 'import/prefer-default-export' - Prefer default export on a file with a single export.
🪛 GitHub Actions: Lint CI
client/app/libs/i18n/translations.js
[error] 1-1: import/prefer-default-export: Prefer default export on a file with single export
🪛 GitHub Check: test (22.x, 3.3.4)
client/app/libs/i18n/translations.js
[failure] 1-1:
Prefer default export on a file with single export
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: deploy
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (6)
.gitignore (1)
44-46: LGTM! Clear documentation of the change.The comment effectively explains why these i18n files are now committed to the repository instead of being ignored, aligning with the new i18n files added in this PR.
client/app/libs/i18n/default.js (1)
1-26: LGTM! Well-structured i18n defaults.The implementation follows react-intl best practices:
- Consistent message ID naming convention (
app.*)- Clear separation of default locale and messages
- Proper use of
defineMessagesfor message descriptorsconfig/shakapacker.yml (1)
11-11: LGTM! Core migration change.This change switches the JavaScript transpiler from Babel to SWC, which is the primary objective of this PR. The configuration aligns with the new SWC config file at
config/swc.config.js.package.json (3)
34-34: LGTM! Correct retention of @babel/runtime.The
@babel/runtimepackage is correctly retained despite migrating to SWC. This package provides runtime helpers and polyfills that are still needed regardless of the transpiler used.
102-103: LGTM! SWC dependencies added.The SWC packages are correctly added to
devDependencies:
@swc/core- the core SWC compiler@swc/jest- Jest transformer using SWCswc-loader- webpack loader for SWCAlso applies to: 105-105
150-151: LGTM – Jest transform updated to @swc/jest; Babel deps removed
All Babel dependencies from issue #666 are removed and @babel/runtime remains.
justin808
force-pushed
the
justin808/migrate-to-swc
branch
from
683b1e9 to
4f3af19
Compare
|
✅ Deployment complete for PR #669, commit 4f3af19 |
PR Review: Migrate from Babel to SWC transpilerOverall Assessment✅ Excellent migration! This PR successfully modernizes the build toolchain with significant performance benefits and no breaking changes. The implementation is clean, well-documented, and thorough. Code Quality & Best Practices✅ Strengths
|
|
✅ Deployment complete for PR #669, commit 24481d3 |
Pull Request Review: Migrate from Babel to SWC transpilerOverall AssessmentThis is a well-executed migration that modernizes the build toolchain. The PR demonstrates thorough testing and clear documentation. However, there are a few issues that should be addressed. 🔴 Critical Issues1. Missing
|
justin808
force-pushed
the
justin808/migrate-to-swc
branch
from
24481d3 to
3676023
Compare
Pull Request Review: Migrate from Babel to SWC transpilerOverall Assessment ✅This is a well-executed migration that follows Shakapacker's official migration process. The changes are clean, comprehensive, and appropriately scoped. The PR demonstrates good engineering practices with thorough testing and clear documentation. ✅ Strengths
|
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #669, commit 0be82af 🎮 Control Plane Console |
Pull Request Review: Migrate from Babel to SWC transpilerSummaryThis PR migrates the project from Babel to SWC for JavaScript transpilation. Overall, this is a well-executed migration with good documentation. However, there are some significant concerns that should be addressed before merging. 🔴 Critical Issues1. Hybrid Babel/SWC Configuration Creates InconsistencyLocation: The PR claims to migrate to SWC, but actually implements a hybrid approach where:
Issues:
Recommendation:
2. ESLint Still Depends on BabelLocation: parserOptions:
requireConfigFile: false
babelOptions:
presets:
- "@babel/preset-react"Issues:
Recommendation:
3. Missing core-js DependencyLocation: The Babel config specifies: useBuiltIns: 'entry',
corejs: 3,But I don't see Recommendation: Add
|
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #669, commit 21e9a2c 🎮 Control Plane Console |
Pull Request Review: Migrate from Babel to SWC TranspilerSummaryThis PR migrates the project from Babel to SWC for JavaScript transpilation. The migration is well-documented and follows Shakapacker's official migration process. Overall, this is a solid implementation with good attention to compatibility concerns. ✅ Strengths
🔍 Code Quality & Best Practicesconfig/webpack/commonWebpackConfig.js (Lines 73-116)Issue: Hybrid SWC/Babel Approach The implementation adds Babel back for JavaScript files while using SWC only for TypeScript. This is contrary to the PR's stated goal of migrating to SWC. // Lines 76-86: SWC restricted to only TypeScript
swcRule.test = /\.(ts|tsx)(\.erb)?$/;
// Lines 89-112: Babel re-introduced for all JavaScript files
config.module.rules.push({
test: /\.(js|jsx|mjs)(\.erb)?$/,
// ... full Babel configuration
});Concerns:
Recommendations:
.eslintrc (Lines 8-12)Issue: ESLint Still Configured for Babel parserOptions:
requireConfigFile: false
babelOptions:
presets:
- "@babel/preset-react"Concerns:
Recommendations:
package.json (Lines 100-110, 133)Dependencies Review: ✅ Good:
🐛 Potential Bugs1. Missing corejs DependencyLocation: config/webpack/commonWebpackConfig.js:99 useBuiltIns: 'entry',
corejs: 3,Issue: Recommendation: Add 2. Babel Loader Configuration HardcodingLocation: config/webpack/commonWebpackConfig.js:96-109 The Babel configuration is hardcoded in webpack config, duplicating what was previously in babel.config.js. This:
Recommendation: Extract to a shared babel.config.js or use environment variables 🔒 Security ConcernsNo major security issues identified. The dependency changes appear safe:
Recommendation: Run ⚡ Performance ConsiderationsPositive:
Concerns:
Recommendations:
🧪 Test CoveragePositive:
Concerns:
Recommendations:
📋 Additional Recommendations1. DocumentationMissing documentation about:
Add to README.md or docs: ## Build Tooling
- TypeScript files: Transpiled with SWC
- JavaScript files: Transpiled with Babel (due to Stimulus compatibility)
- Tests: Use @swc/jest2. Configuration File Location
3. Clean Up Old Babel ConfigThe deleted grep -r "babel.config" . --exclude-dir=node_modules🎯 Summary & VerdictOverall Assessment: This PR makes good progress toward modernizing the build tooling, but the hybrid approach contradicts the stated migration goal and may reduce the performance benefits. Required Changes:
Recommended Changes:
Great work on tackling this migration! The attention to compatibility and the use of official migration tools shows good engineering judgment. With the recommended changes, this will be a solid improvement to the codebase. 🤖 Review generated with Claude Code |
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #669, commit 0444f60 🎮 Control Plane Console |
Pull Request Review: Migrate from Babel to SWC transpilerOverall AssessmentThis PR migrates JavaScript transpilation from Babel to SWC, which is a significant tooling change. While the migration approach using Shakapacker's official task is sound, there are critical architectural concerns that need to be addressed. 🚨 Critical Issues1. Contradictory Configuration - Mixed Babel/SWC SetupLocation: The PR claims to migrate to SWC, but the webpack configuration adds Babel back for JavaScript files: config.module.rules.splice(swcRuleIndex, 0, {
test: /\.(js|jsx|mjs)(\.erb)?$/,
include: swcRule.include,
exclude: swcRule.exclude,
use: {
loader: 'babel-loader',
options: {
presets: [
['@babel/preset-env', {
useBuiltIns: 'entry',
corejs: 3,
// ...
}],
['@babel/preset-react', {
runtime: 'automatic',
// ...
}]
]
}
}
});Issues:
Why was this done? The code comment says "This allows Babel to handle all JavaScript while SWC continues to match TypeScript" - but this contradicts the PR's stated goal. Recommendation:
2. Dependency Management IssuesMissing
Recommendation: yarn add core-js@3Babel dependencies kept as devDependencies:
This is actually correct given the hybrid setup, but contradicts the PR description claiming Babel was removed. 3. Test Configuration MismatchLocation: Jest is configured to use "transform": {
"^.+\\.jsx?$": "@swc/jest"
}Issue: Tests will use SWC for transformation, but the actual application builds use Babel for JS files. This creates a testing/production parity problem:
Recommendation:
|
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #669, commit 543f1a6 🎮 Control Plane Console |
Code Review: Migrate from Babel to SWC transpilerOverviewThis PR migrates the project from Babel to SWC for JavaScript transpilation, which is a significant tooling change. The migration appears well-executed and follows Shakapacker's official migration path. Overall, this is a solid migration with some notable concerns that should be addressed. ✅ Strengths
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/webpack/commonWebpackConfig.js (1)
6-6: Unused import:pathmodule.The
pathmodule is imported but not used in this file. Consider removing it unless it's planned for future use.Apply this diff to remove the unused import:
-const path = require('path');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
config/swc.config.js(1 hunks)config/webpack/commonWebpackConfig.js(2 hunks)package.json(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/webpack/commonWebpackConfig.js (2)
config/webpack/serverWebpackConfig.js (2)
require(5-5)commonWebpackConfig(6-6)config/webpack/clientWebpackConfig.js (1)
commonWebpackConfig(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (22.x, 3.4.6)
- GitHub Check: deploy
- GitHub Check: claude-review
🔇 Additional comments (4)
config/swc.config.js (1)
1-11: LGTM! Minimal SWC configuration is appropriate.The configuration correctly sets React's automatic runtime and preserves class names for Stimulus compatibility. This minimal config is suitable since JavaScript transpilation is now handled by Babel (per commonWebpackConfig.js).
Note: The past review comment about
env.mode: 'entry'is no longer relevant as that configuration has been removed from this file.package.json (2)
101-110: Dependency structure supports hybrid Babel+SWC approach.The Babel packages (@babel/core, presets, babel-loader) moved to devDependencies alongside SWC packages (@swc/core, swc-loader). This aligns with the webpack configuration that uses Babel for JavaScript and SWC for TypeScript.
However, note that the PR description indicates a migration "from Babel to SWC," but the implementation maintains Babel for JavaScript transpilation. Consider clarifying in the PR description that this is a hybrid approach rather than a complete Babel removal.
156-156: LGTM! Jest correctly configured to use SWC.The Jest transformer updated to use
@swc/jestfor JavaScript/JSX files, which aligns with the migration goals for test transpilation.config/webpack/commonWebpackConfig.js (1)
84-85: Clarify the hybrid Babel+SWC architecture.The comment states "This allows Babel to handle all JavaScript while SWC continues to match TypeScript," but this hybrid approach wasn't clearly described in the PR objectives, which indicate a migration "from Babel to SWC."
Consider updating the PR description to clarify that:
- This is a hybrid approach, not a complete Babel removal
- Babel handles JavaScript (.js, .jsx, .mjs)
- SWC handles TypeScript (.ts, .tsx)
- The rationale for maintaining Babel for JavaScript
| const commonWebpackConfig = () => { | ||
| const config = merge({}, baseClientWebpackConfig, commonOptions, ignoreWarningsConfig); | ||
|
|
||
| // Find the SWC rule and get its include/exclude | ||
| const swcRuleIndex = config.module.rules.findIndex(rule => | ||
| rule.test && /\.(ts|tsx|js|jsx|mjs|coffee)/.test(rule.test.toString()) | ||
| ); | ||
|
|
||
| if (swcRuleIndex !== -1) { | ||
| const swcRule = config.module.rules[swcRuleIndex]; | ||
|
|
||
| // Insert Babel rule BEFORE SWC rule so it matches .js files first | ||
| // This allows Babel to handle all JavaScript while SWC continues to match TypeScript | ||
| config.module.rules.splice(swcRuleIndex, 0, { | ||
| test: /\.(js|jsx|mjs)(\.erb)?$/, | ||
| include: swcRule.include, | ||
| exclude: swcRule.exclude, | ||
| use: { | ||
| loader: 'babel-loader', | ||
| options: { | ||
| presets: [ | ||
| ['@babel/preset-env', { | ||
| useBuiltIns: 'entry', | ||
| corejs: 3, | ||
| modules: 'auto', | ||
| bugfixes: true, | ||
| exclude: ['transform-typeof-symbol'] | ||
| }], | ||
| ['@babel/preset-react', { | ||
| runtime: 'automatic', | ||
| development: process.env.NODE_ENV !== 'production', | ||
| useBuiltIns: true | ||
| }] | ||
| ] | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return config; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Babel's useBuiltIns: 'entry' requires explicit core-js imports.
The Babel loader is configured with useBuiltIns: 'entry' (line 95), which requires explicit import 'core-js/stable' and import 'regenerator-runtime/runtime' in each entry file. Without these imports, polyfills won't be included, potentially causing runtime errors in older browsers.
Additionally, this implementation maintains Babel for JavaScript transpilation, creating a hybrid Babel+SWC setup (Babel for .js/.jsx, SWC for .ts/.tsx) rather than a complete migration to SWC as suggested by the PR description.
Option 1 (Recommended): Change to useBuiltIns: 'usage' to let Babel automatically inject only the required polyfills:
['@babel/preset-env', {
- useBuiltIns: 'entry',
+ useBuiltIns: 'usage',
corejs: 3,Option 2: Keep useBuiltIns: 'entry' and add core-js imports to all entry files (e.g., files in client/app/packs/).
Run the following script to verify if core-js imports exist in entry files:
🏁 Script executed:
#!/bin/bash
# Description: Check for core-js imports in entry files
echo "=== Checking for core-js imports in entry files ==="
fd -e js -e jsx . client/app/packs/ --exec rg -l "import.*['\"]core-js" {} \; || echo "No core-js imports found"
echo ""
echo "=== Checking for regenerator-runtime imports ==="
fd -e js -e jsx . client/app/packs/ --exec rg -l "import.*['\"]regenerator-runtime" {} \; || echo "No regenerator-runtime imports found"
echo ""
echo "=== Entry files in packs directory ==="
fd -e js -e jsx . client/app/packs/Length of output: 313
Switch Babel preset-env to useBuiltIns: 'usage' or add core-js imports
No core-js or regenerator-runtime imports were found in client/app/packs/*; with useBuiltIns: 'entry', polyfills won’t be included. Prefer:
['@babel/preset-env', {
- useBuiltIns: 'entry',
+ useBuiltIns: 'usage',
corejs: 3,
}]Alternatively, keep entry and add import 'core-js/stable' and import 'regenerator-runtime/runtime' to each entry file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const commonWebpackConfig = () => { | |
| const config = merge({}, baseClientWebpackConfig, commonOptions, ignoreWarningsConfig); | |
| // Find the SWC rule and get its include/exclude | |
| const swcRuleIndex = config.module.rules.findIndex(rule => | |
| rule.test && /\.(ts|tsx|js|jsx|mjs|coffee)/.test(rule.test.toString()) | |
| ); | |
| if (swcRuleIndex !== -1) { | |
| const swcRule = config.module.rules[swcRuleIndex]; | |
| // Insert Babel rule BEFORE SWC rule so it matches .js files first | |
| // This allows Babel to handle all JavaScript while SWC continues to match TypeScript | |
| config.module.rules.splice(swcRuleIndex, 0, { | |
| test: /\.(js|jsx|mjs)(\.erb)?$/, | |
| include: swcRule.include, | |
| exclude: swcRule.exclude, | |
| use: { | |
| loader: 'babel-loader', | |
| options: { | |
| presets: [ | |
| ['@babel/preset-env', { | |
| useBuiltIns: 'entry', | |
| corejs: 3, | |
| modules: 'auto', | |
| bugfixes: true, | |
| exclude: ['transform-typeof-symbol'] | |
| }], | |
| ['@babel/preset-react', { | |
| runtime: 'automatic', | |
| development: process.env.NODE_ENV !== 'production', | |
| useBuiltIns: true | |
| }] | |
| ] | |
| } | |
| } | |
| }); | |
| } | |
| return config; | |
| }; | |
| const commonWebpackConfig = () => { | |
| const config = merge({}, baseClientWebpackConfig, commonOptions, ignoreWarningsConfig); | |
| // Find the SWC rule and get its include/exclude | |
| const swcRuleIndex = config.module.rules.findIndex(rule => | |
| rule.test && /\.(ts|tsx|js|jsx|mjs|coffee)/.test(rule.test.toString()) | |
| ); | |
| if (swcRuleIndex !== -1) { | |
| const swcRule = config.module.rules[swcRuleIndex]; | |
| // Insert Babel rule BEFORE SWC rule so it matches .js files first | |
| // This allows Babel to handle all JavaScript while SWC continues to match TypeScript | |
| config.module.rules.splice(swcRuleIndex, 0, { | |
| test: /\.(js|jsx|mjs)(\.erb)?$/, | |
| include: swcRule.include, | |
| exclude: swcRule.exclude, | |
| use: { | |
| loader: 'babel-loader', | |
| options: { | |
| presets: [ | |
| ['@babel/preset-env', { | |
| useBuiltIns: 'usage', | |
| corejs: 3, | |
| modules: 'auto', | |
| bugfixes: true, | |
| exclude: ['transform-typeof-symbol'] | |
| }], | |
| ['@babel/preset-react', { | |
| runtime: 'automatic', | |
| development: process.env.NODE_ENV !== 'production', | |
| useBuiltIns: true | |
| }] | |
| ] | |
| } | |
| } | |
| }); | |
| } | |
| return config; | |
| }; |
🤖 Prompt for AI Agents
In config/webpack/commonWebpackConfig.js around lines 73 to 113: the Babel
preset-env is configured with useBuiltIns: 'entry' but there are no
core-js/regenerator imports in client entry files, so polyfills are not being
injected; either change the preset-env option to useBuiltIns: 'usage' so Babel
injects polyfills where needed, or keep 'entry' and add import 'core-js/stable'
and import 'regenerator-runtime/runtime' to each client entry file (or a shared
bootstrap entry) so the required polyfills are bundled.
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #669, commit 74d89c1 🎮 Control Plane Console |
PR Review: Migrate from Babel to SWC transpilerThank you for working on this migration! I've conducted a thorough review and identified several critical issues that need to be addressed before merging. 🔴 Critical Issues1. Contradictory Architecture - Defeating the Purpose of MigrationThe most significant issue is in // Insert Babel rule BEFORE SWC rule so it matches .js files first
// This allows Babel to handle all JavaScript while SWC continues to match TypeScript
config.module.rules.splice(swcRuleIndex, 0, {
test: /\.(js|jsx|mjs)(\.erb)?$/,
include: swcRule.include,
exclude: swcRule.exclude,
use: {
loader: 'babel-loader',
// ... babel configuration
}
});Problems:
Impact: The claimed performance benefits are likely not being realized since Babel is still transpiling the majority of files. 2. Missing SWC Configuration FileThe PR adds // In webpack config, you'd typically see:
{
loader: 'swc-loader',
options: require('../swc.config.js')
}Without this reference, your SWC configuration (including the important 3. Dependency InconsistencyIn
Issue: If someone runs Recommendation: Either move these to 4. ESLint Still Requires BabelIn parserOptions:
requireConfigFile: false
babelOptions:
presets:
- "@babel/preset-react"This configuration requires Consideration: Consider using
|
justin808
force-pushed
the
justin808/migrate-to-swc
branch
from
74d89c1 to
4332a54
Compare
justin808
changed the title
🎉 ✨ Deploy Complete! 🚀🌐 ➡️ Open Review AppDeployment successful for PR #669, commit 4332a54 🎮 Control Plane Console |
Pull Request ReviewSummaryThis PR addresses ReScript deprecation warnings by updating configuration and removing an unused event parameter. The changes are minimal, focused, and appropriate. ✅ Code Quality & Best PracticesExcellent:
✅ CorrectnessBoth changes are technically sound:
✅ Security & PerformanceNo concerns:
📝 ObservationsGood:
Minor note:
Recommendation✅ Approve - This is a clean, straightforward fix for deprecation warnings with no negative side effects. Review generated with assistance from Claude Code |
|
✅ Review app for PR #669 was successfully deleted |
Fixes ReScript deprecation warnings:
These changes resolve deprecation warnings shown during ReScript compilation.
This change is
Summary by CodeRabbit