Skip to content

Commit 88529af

Browse files
justin808claude
justin808
and
claude
committed
Document Rails Engine development nuances and rake task loading
Add comprehensive documentation to CLAUDE.md about Rails Engine specifics: - Automatic rake task loading from lib/tasks/ (avoid duplicate execution) - Engine initializers and hooks (config.to_prepare, initializer blocks) - Engine vs application code contexts - Testing strategies (dummy app vs unit tests) - Common pitfalls and best practices Create RAKE_TASK_DUPLICATE_ANALYSIS.md analyzing PR #1770: - Why explicit rake_tasks block was added during Generator Overhaul - How it caused 2x slower builds for 2 months - Lessons learned about massive PRs and subtle performance bugs - Best practices for Rails::Engine rake task loading This documentation prevents future bugs like duplicate task execution and helps developers understand Rails Engine development nuances. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent beb70f0 commit 88529af

File tree

2 files changed

+218
-0
lines changed

2 files changed

+218
-0
lines changed

‎CLAUDE.md‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -517,6 +517,83 @@ Playwright E2E tests run automatically in CI via GitHub Actions (`.github/workfl
517517
- Uploads HTML reports as artifacts (available for 30 days)
518518
- Auto-starts Rails server before running tests
519519

520+
## Rails Engine Development Nuances
521+
522+
React on Rails is a **Rails Engine**, which has important implications for development:
523+
524+
### Automatic Rake Task Loading
525+
526+
**CRITICAL**: Rails::Engine automatically loads all `.rake` files from `lib/tasks/` directory. **DO NOT** use a `rake_tasks` block to explicitly load them, as this causes duplicate task execution.
527+
528+
```ruby
529+
# ❌ WRONG - Causes duplicate execution
530+
module ReactOnRails
531+
class Engine < ::Rails::Engine
532+
rake_tasks do
533+
load File.expand_path("../tasks/generate_packs.rake", __dir__)
534+
load File.expand_path("../tasks/assets.rake", __dir__)
535+
load File.expand_path("../tasks/locale.rake", __dir__)
536+
end
537+
end
538+
end
539+
540+
# ✅ CORRECT - Rails::Engine loads lib/tasks/*.rake automatically
541+
module ReactOnRails
542+
class Engine < ::Rails::Engine
543+
# Rake tasks are automatically loaded from lib/tasks/*.rake by Rails::Engine
544+
# No explicit loading needed
545+
end
546+
end
547+
```
548+
549+
**When to use `rake_tasks` block:**
550+
- Tasks are in a **non-standard location** (not `lib/tasks/`)
551+
- You need to **programmatically generate** tasks
552+
- You need to **pass context** to the tasks
553+
554+
**Historical Context**: PR #1770 added explicit rake task loading, causing webpack builds and pack generation to run twice during `rake assets:precompile`. This was fixed in PR #2052. See `RAKE_TASK_DUPLICATE_ANALYSIS.md` for full details.
555+
556+
### Engine Initializers and Hooks
557+
558+
Engines have specific initialization hooks that run at different times:
559+
560+
```ruby
561+
module ReactOnRails
562+
class Engine < ::Rails::Engine
563+
# Runs after Rails initializes but before routes are loaded
564+
config.to_prepare do
565+
ReactOnRails::ServerRenderingPool.reset_pool
566+
end
567+
568+
# Runs during Rails initialization, use for validations
569+
initializer "react_on_rails.validate_version" do
570+
config.after_initialize do
571+
# Validation logic here
572+
end
573+
end
574+
end
575+
end
576+
```
577+
578+
### Engine vs Application Code
579+
580+
- **Engine code** (`lib/react_on_rails/`): Runs in the gem context, has limited access to host application
581+
- **Host application code**: The Rails app that includes the gem
582+
- **Generators** (`lib/generators/react_on_rails/`): Run in host app context during setup
583+
584+
### Testing Engines
585+
586+
- **Dummy app** (`spec/dummy/`): Full Rails app for integration testing
587+
- **Unit tests** (`spec/react_on_rails/`): Test gem code in isolation
588+
- Always test both contexts: gem code alone and gem + host app integration
589+
590+
### Common Pitfalls
591+
592+
1. **Assuming host app structure**: Don't assume `app/javascript/` exists—it might not in older apps
593+
2. **Path resolution**: Use `Rails.root` for host app paths, not relative paths
594+
3. **Autoloading**: Engine code follows Rails autoloading rules but with a different load path
595+
4. **Configuration**: Engine config is separate from host app config—use `ReactOnRails.configure`
596+
520597
## IDE Configuration
521598

522599
Exclude these directories to prevent IDE slowdowns:

‎RAKE_TASK_DUPLICATE_ANALYSIS.md‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
# Analysis: Why rake_tasks Block Was Added in PR #1770 and Caused Duplicate Execution
2+
3+
## Summary
4+
5+
In PR #1770 (commit `8f3d178` - "Generator Overhaul & Developer Experience Enhancement"), Ihab added a `rake_tasks` block to `lib/react_on_rails/engine.rb` that explicitly loaded three rake task files. This was part of a **massive generator overhaul** that introduced new rake tasks for the file-system auto-registration feature. However, this caused those tasks to execute **twice** during operations like `rake assets:precompile`, which was fixed in PR #2052.
6+
7+
## The Problem: Double Loading of Rake Tasks
8+
9+
### What Was Added in PR #1770 (8f3d178)
10+
11+
```ruby
12+
# lib/react_on_rails/engine.rb
13+
module ReactOnRails
14+
class Engine < ::Rails::Engine
15+
# ... existing code ...
16+
17+
rake_tasks do
18+
load File.expand_path("../tasks/generate_packs.rake", __dir__)
19+
load File.expand_path("../tasks/assets.rake", __dir__)
20+
load File.expand_path("../tasks/locale.rake", __dir__)
21+
end
22+
end
23+
end
24+
```
25+
26+
### Why This Caused Duplicate Execution
27+
28+
Rails Engines have **two different mechanisms** for loading rake tasks, and this code inadvertently activated both:
29+
30+
1. **Automatic Loading (Engine Layer)**: Rails::Engine automatically loads all `.rake` files from `lib/tasks/` directory
31+
2. **Manual Loading (Railtie Layer)**: The `rake_tasks` block explicitly loads specific files
32+
33+
Because the task files existed in `lib/tasks/`:
34+
- `lib/tasks/assets.rake`
35+
- `lib/tasks/generate_packs.rake`
36+
- `lib/tasks/locale.rake`
37+
38+
They were being loaded **twice**:
39+
- Once automatically by Rails::Engine from the `lib/tasks/` directory
40+
- Once explicitly by the `rake_tasks` block
41+
42+
## Why Was This Added in PR #1770?
43+
44+
PR #1770 was a **major overhaul** with 97 files changed. Looking at the context:
45+
46+
### The Generator Overhaul Introduced:
47+
48+
1. **File-system auto-registration**: New feature where components auto-register under `app/javascript/src/.../ror_components`
49+
2. **New `react_on_rails:generate_packs` rake task**: Critical new task for the auto-registration system
50+
3. **Enhanced dev tooling**: New `ReactOnRails::Dev` namespace with ServerManager, ProcessManager, PackGenerator
51+
4. **Shakapacker as required dependency**: Made Shakapacker mandatory (removed Webpacker support)
52+
53+
### Why the Explicit Loading Was Added:
54+
55+
Based on the PR context and commit message, the most likely reasons:
56+
57+
1. **Ensuring Critical Task Availability**: The `react_on_rails:generate_packs` task was brand new and absolutely critical to the file-system auto-registration feature. Ihab may have wanted to guarantee it would be loaded in all contexts.
58+
59+
2. **Following Common Rails Engine Patterns**: The `rake_tasks` block is a well-documented pattern in Rails engines. Many gems use it explicitly, even when files are in `lib/tasks/`. Ihab likely followed this pattern as "best practice."
60+
61+
3. **Massive PR Complexity**: With 97 files changed, this was a huge refactor. The `rake_tasks` block addition was a tiny part of the overall changes, and the duplicate loading issue was subtle enough that it wasn't caught during review.
62+
63+
4. **Lack of Awareness About Automatic Loading**: Rails::Engine's automatic loading of `lib/tasks/*.rake` files is not as well-known as it should be. Many developers (even experienced ones) don't realize this happens automatically.
64+
65+
5. **"Belt and Suspenders" Approach**: Given the criticality of the new auto-registration feature, Ihab may have intentionally added explicit loading as a safety measure, not realizing it would cause duplication.
66+
67+
**The commit message doesn't mention the rake_tasks addition at all**—it focuses on generator improvements, dev experience, and component architecture. This suggests the `rake_tasks` block was added as a routine implementation detail, not something Ihab thought needed explanation.
68+
69+
## The Impact
70+
71+
Tasks affected by duplicate execution:
72+
- `react_on_rails:assets:webpack` - Webpack builds ran twice
73+
- `react_on_rails:generate_packs` - Pack generation ran twice
74+
- `react_on_rails:locale` - Locale file generation ran twice
75+
76+
This meant:
77+
- **2x build times** during asset precompilation
78+
- **Slower CI** builds
79+
- **Confusing console output** showing duplicate webpack compilation messages
80+
- **Wasted resources** running the same expensive operations twice
81+
82+
## The Fix (PR #2052)
83+
84+
The fix was simple—remove the redundant `rake_tasks` block and rely solely on Rails' automatic loading:
85+
86+
```ruby
87+
# lib/react_on_rails/engine.rb
88+
module ReactOnRails
89+
class Engine < ::Rails::Engine
90+
# ... existing code ...
91+
92+
# Rake tasks are automatically loaded from lib/tasks/*.rake by Rails::Engine
93+
# No need to explicitly load them here to avoid duplicate loading
94+
end
95+
end
96+
```
97+
98+
## Key Lesson
99+
100+
**Rails::Engine Best Practice**: If your rake task files are in `lib/tasks/`, you don't need a `rake_tasks` block. Rails will load them automatically. Only use `rake_tasks do` if:
101+
- Tasks are in a non-standard location
102+
- You need to programmatically generate tasks
103+
- You need to pass context to the tasks
104+
105+
## Timeline
106+
107+
- **Sep 16, 2025** (PR #1770, commit 8f3d178): Ihab adds `rake_tasks` block as part of massive Generator Overhaul (97 files changed)
108+
- **Nov 18, 2025** (PR #2052, commit 3f6df6be9): Justin discovers and fixes duplicate execution issue by removing the block (~2 months later)
109+
110+
## What We Learned
111+
112+
### For Code Reviews
113+
114+
This incident highlights the challenge of reviewing massive PRs:
115+
- **97 files changed** made it nearly impossible to catch subtle issues
116+
- The `rake_tasks` addition was 6 lines in a file that wasn't the focus of the PR
117+
- The duplicate loading bug only manifested during asset precompilation, not during normal development
118+
- Smaller, focused PRs would have made this easier to catch
119+
120+
### For Testing
121+
122+
The duplicate execution bug was subtle:
123+
- **Didn't cause failures**—just slower builds (2x time)
124+
- **Hard to notice locally**—developers might not realize builds were taking twice as long
125+
- **Only obvious in CI**—where build times are closely monitored
126+
- **Needed production-like scenarios**—requires running `rake assets:precompile` to trigger
127+
128+
### For Documentation
129+
130+
Better documentation of Rails::Engine automatic loading would help:
131+
- Many Rails guides show `rake_tasks` blocks without mentioning automatic loading
132+
- The Rails Engine guide doesn't clearly state when NOT to use `rake_tasks`
133+
- This leads to cargo-culting of the pattern
134+
135+
## References
136+
137+
- **Original PR**: [#1770 - "React on Rails Generator Overhaul & Developer Experience Enhancement"](https://github.com/shakacode/react_on_rails/pull/1770)
138+
- **Original commit**: `8f3d178` - 97 files changed, massive refactor
139+
- **Fix PR**: [#2052 - "Fix duplicate rake task execution by removing explicit task loading"](https://github.com/shakacode/react_on_rails/pull/2052)
140+
- **Fix commit**: `3f6df6be9` - Simple 6-line removal
141+
- **Rails Engine documentation**: https://guides.rubyonrails.org/engines.html#rake-tasks

0 commit comments

Comments
 (0)