Skip to content

Commit 34f3410

Browse files
justin808claude
justin808
and
claude
committed
Update using_swc? to return true by default for Shakapacker 9.3.0
Shakapacker 9.3.0 defaults to SWC compiler if javascript_compiler is not explicitly set in shakapacker.yml. Update the using_swc? method to reflect this new default behavior. Changes: - using_swc? now returns true (SWC) by default when config doesn't exist - using_swc? returns true unless javascript_compiler is explicitly set to babel - Updated template shakapacker.yml to comment out javascript_compiler setting - Added clear documentation that Shakapacker 9.3.0+ defaults to SWC This ensures the generator correctly detects and installs SWC dependencies by default, matching Shakapacker 9.3.0's new default compiler. Addresses PR review comment on lib/generators/react_on_rails/base_generator.rb:225 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent aa1523e commit 34f3410

File tree

3 files changed

+190
-5
lines changed

3 files changed

+190
-5
lines changed

‎PR_REVIEW_RESPONSE.md‎

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
# PR #1896 Review Comments Response
2+
3+
## Review Comments Addressed
4+
5+
### 1. ✅ shakapacker-precompile-hook template file
6+
7+
**Status:** Fixed
8+
9+
- Restored `lib/generators/react_on_rails/templates/base/base/bin/shakapacker-precompile-hook`
10+
- The file now exists in both template and spec/dummy
11+
12+
### 2. ✅ shakapacker.yml template changes
13+
14+
**Status:** Fixed
15+
16+
- Reverted changes to `lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml`
17+
- Precompile hook configuration remains in template
18+
19+
### 3. ❓ base_generator.rb:225 - Should `using_swc?` return 'true' for Shakapacker 9.3.0 default?
20+
21+
**Status:** Needs clarification
22+
23+
**Current Code (line 225):**
24+
25+
```ruby
26+
def using_swc?
27+
shakapacker_config_path = File.join(destination_root, "config", "shakapacker.yml")
28+
return false unless File.exist?(shakapacker_config_path) # Line 225
29+
30+
config_content = File.read(shakapacker_config_path)
31+
config_content.include?("javascript_compiler: swc")
32+
end
33+
```
34+
35+
**Current Behavior:** Returns `false` (Babel) when config doesn't exist
36+
37+
**Question:** Should this return `true` (SWC) instead?
38+
39+
**Analysis:**
40+
41+
- Our template (`lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml:57`) explicitly sets:
42+
```yaml
43+
# Compiler to use for JavaScript: 'babel' or 'swc'
44+
# SWC is faster but Babel has more plugins and wider ecosystem support
45+
# Default: babel
46+
javascript_compiler: babel
47+
```
48+
49+
**Conclusion:** The current implementation appears **correct** - it returns `false` (Babel) by default, matching our template's default.
50+
51+
**However, please clarify:**
52+
53+
1. Did Shakapacker 9.3.0 change the built-in default from Babel to SWC?
54+
2. Should React on Rails default to SWC instead of Babel for new projects?
55+
3. Or is there a different scenario where this method behaves incorrectly?
56+
57+
### 4. lib/react_on_rails/dev/pack_generator.rb - Relation to Shakapacker upgrade
58+
59+
**Explanation:**
60+
61+
This change is **indirectly related** to Shakapacker upgrade:
62+
63+
**Context:** During Shakapacker 9.3.0 upgrade testing, discovered that pack generation fails when `bin/dev` is run from Bundler context.
64+
65+
**The Problem:**
66+
67+
```ruby
68+
# Old code - breaks when run from bundler
69+
system("bundle exec rails react_on_rails:generate_packs")
70+
```
71+
72+
When you run `bundle exec bin/dev`, which internally runs pack generation, this creates nested `bundle exec` calls that fail.
73+
74+
**The Solution:**
75+
76+
```ruby
77+
# New code - detects Rails availability and runs appropriately
78+
if defined?(Rails) && Rails.application
79+
Rake::Task["react_on_rails:generate_packs"].invoke
80+
else
81+
system("bundle exec rails react_on_rails:generate_packs")
82+
end
83+
```
84+
85+
**Recommendation:** This could be split into a separate PR since it's a bug fix discovered during upgrade testing, not a requirement of Shakapacker 9.3.0 itself.
86+
87+
### 5. lib/react_on_rails/engine.rb - Why related to Shakapacker update
88+
89+
**Explanation:**
90+
91+
This file has **both** Shakapacker-related changes AND code review fixes:
92+
93+
**Shakapacker-related changes (commits: 54c2e1e4, 7eb87732, 110d753f):**
94+
95+
```ruby
96+
# Skip validation if package.json doesn't exist yet (during initial setup)
97+
next unless File.exist?(package_json)
98+
99+
# Skip validation when react-on-rails package not installed
100+
next unless PackerUtils.react_on_rails_pro_installed? || PackerUtils.react_on_rails_package_installed?
101+
```
102+
103+
These are needed because:
104+
105+
- Shakapacker 9.3.0 upgrade process involves reinstalling packages
106+
- During testing, validation was failing when packages not yet installed
107+
- Prevents Rails from crashing during Shakapacker setup
108+
109+
**Code review fix (commit: 5619ea0b - YESTERDAY):**
110+
111+
```ruby
112+
# Skip validation when generators explicitly set this flag (packages may not be installed yet)
113+
next if ENV["REACT_ON_RAILS_SKIP_VALIDATION"] == "true"
114+
```
115+
116+
This change was from addressing separate code review feedback about fragile ARGV-based generator detection.
117+
118+
**Recommendation:** The latest commit (5619ea0b) with generator robustness fixes could potentially be split to a separate PR since it addresses code review feedback unrelated to Shakapacker upgrade.
119+
120+
### 6. spec/dummy/babel.config.js - Why related to Shakapacker upgrade
121+
122+
**Explanation:**
123+
124+
This is **directly required** by Shakapacker 9.3.0:
125+
126+
**The Change:**
127+
128+
```javascript
129+
// Old (Shakapacker 8.x)
130+
const defaultConfigFunc = require('shakapacker/package/babel/preset');
131+
132+
// New (Shakapacker 9.3.0)
133+
// eslint-disable-next-line import/extensions
134+
const defaultConfigFunc = require('shakapacker/package/babel/preset.js');
135+
```
136+
137+
**Why it's required:**
138+
139+
- Shakapacker 9.3.0 changed its module resolution behavior
140+
- The `.js` extension is now required for CommonJS requires
141+
- Without this change, the require fails with "Cannot find module" error
142+
- This is a **breaking change** in Shakapacker 9.3.0
143+
144+
**Evidence:**
145+
146+
- Commit: caeb0796 "Fix babel preset require path for Shakapacker 9.3.0 with explicit .js extension"
147+
- This was discovered during Shakapacker 9.3.0 testing
148+
149+
## Summary & Recommendation
150+
151+
### Changes that SHOULD stay in Shakapacker 9.3.0 PR:
152+
153+
1. ✅ babel.config.js - Required by Shakapacker 9.3.0
154+
2. ✅ engine.rb validation guards (commits 54c2e1e4, 7eb87732, 110d753f) - Needed for upgrade process
155+
3. ✅ All template file restorations
156+
157+
### Changes that COULD be split to separate PRs:
158+
159+
1. 🔄 pack_generator.rb fix (commit 3b9ad525) - Bug discovered during testing but not Shakapacker requirement
160+
2. 🔄 Generator robustness fixes (commit 5619ea0b) - Addresses separate code review feedback
161+
162+
### Proposed Action Plan:
163+
164+
**Option A (Keep everything together):**
165+
166+
- Argument: All these changes were discovered/needed during Shakapacker 9.3.0 upgrade and testing
167+
- Makes the PR self-contained with all related fixes
168+
- Easier to review the complete upgrade impact
169+
170+
**Option B (Split into separate PRs):**
171+
172+
- Create PR #1: Shakapacker 9.3.0 upgrade (core changes only)
173+
- Create PR #2: Pack generator bundler context fix
174+
- Create PR #3: Generator robustness improvements (the code review fixes)
175+
176+
**Recommendation:** Option A (keep together) because:
177+
178+
1. All changes were discovered during Shakapacker upgrade testing
179+
2. They're all related to making the upgrade smooth
180+
3. Splitting now would require rebasing and retesting
181+
4. The latest commit (generator fixes) improves code that was already being modified in this PR
182+
183+
What's your preference?

‎lib/generators/react_on_rails/base_generator.rb‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -232,12 +232,13 @@ def add_dev_dependencies
232232

233233
def using_swc?
234234
# Check shakapacker.yml for javascript_compiler setting
235-
# Default to babel if not specified
235+
# Shakapacker 9.3.0 defaults to SWC if not specified
236236
shakapacker_config_path = File.join(destination_root, "config", "shakapacker.yml")
237-
return false unless File.exist?(shakapacker_config_path)
237+
return true unless File.exist?(shakapacker_config_path)
238238

239239
config_content = File.read(shakapacker_config_path)
240-
config_content.include?("javascript_compiler: swc")
240+
# Return true (SWC) unless explicitly set to babel
241+
!config_content.include?("javascript_compiler: babel")
241242
end
242243

243244
def install_js_dependencies

‎lib/generators/react_on_rails/templates/base/base/config/shakapacker.yml‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,9 @@ default: &default
5858

5959
# Compiler to use for JavaScript: 'babel' or 'swc'
6060
# SWC is faster but Babel has more plugins and wider ecosystem support
61-
# Default: babel
62-
javascript_compiler: babel
61+
# Shakapacker 9.3.0+ defaults to 'swc' if not specified
62+
# Uncomment to use Babel instead:
63+
# javascript_compiler: babel
6364

6465
# Setting the asset host here will override Rails.application.config.asset_host.
6566
# Here, you can set different asset_host per environment. Note that

0 commit comments

Comments
 (0)