ContextQMD
Back to React On Rails

Analysis: Why rake_tasks Block Was Added in PR #1770 and Caused Duplicate Execution

analysis/rake-task-duplicate-analysis.md

16.6.06.8 KB
Original Source

Analysis: Why rake_tasks Block Was Added in PR #1770 and Caused Duplicate Execution

Summary

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.

The Problem: Double Loading of Rake Tasks

What Was Added in PR #1770 (8f3d178)

ruby
# lib/react_on_rails/engine.rb
module ReactOnRails
  class Engine < ::Rails::Engine
    # ... existing code ...

    rake_tasks do
      load File.expand_path("../tasks/generate_packs.rake", __dir__)
      load File.expand_path("../tasks/assets.rake", __dir__)
      load File.expand_path("../tasks/locale.rake", __dir__)
    end
  end
end

Why This Caused Duplicate Execution

Rails Engines have two different mechanisms for loading rake tasks, and this code inadvertently activated both:

  1. Automatic Loading (Engine Layer): Rails::Engine automatically loads all .rake files from lib/tasks/ directory
  2. Manual Loading (Railtie Layer): The rake_tasks block explicitly loads specific files

Because the task files existed in lib/tasks/:

  • lib/tasks/assets.rake
  • lib/tasks/generate_packs.rake
  • lib/tasks/locale.rake

They were being loaded twice:

  • Once automatically by Rails::Engine from the lib/tasks/ directory
  • Once explicitly by the rake_tasks block

Why Was This Added in PR #1770?

PR #1770 was a major overhaul with 97 files changed. Looking at the context:

The Generator Overhaul Introduced:

  1. File-system auto-registration: New feature where components auto-register under app/javascript/src/.../ror_components
  2. New react_on_rails:generate_packs rake task: Critical new task for the auto-registration system
  3. Enhanced dev tooling: New ReactOnRails::Dev namespace with ServerManager, ProcessManager, PackGenerator
  4. Shakapacker as required dependency: Made Shakapacker mandatory (removed Webpacker support)

Why the Explicit Loading Was Added:

Based on the PR context and commit message, the most likely reasons:

  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.

  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."

  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.

  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.

  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.

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.

The Impact

Tasks affected by duplicate execution:

  • react_on_rails:assets:webpack - Webpack builds ran twice
  • react_on_rails:generate_packs - Pack generation ran twice
  • react_on_rails:locale - Locale file generation ran twice

This meant:

  • 2x build times during asset precompilation
  • Slower CI builds
  • Confusing console output showing duplicate webpack compilation messages
  • Wasted resources running the same expensive operations twice

The Fix (PR #2052)

The fix was simple—remove the redundant rake_tasks block and rely solely on Rails' automatic loading:

ruby
# lib/react_on_rails/engine.rb
module ReactOnRails
  class Engine < ::Rails::Engine
    # ... existing code ...

    # Rake tasks are automatically loaded from lib/tasks/*.rake by Rails::Engine
    # No need to explicitly load them here to avoid duplicate loading
  end
end

Key Lesson

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:

  • Tasks are in a non-standard location
  • You need to programmatically generate tasks
  • You need to pass context to the tasks

Timeline

  • Sep 16, 2025 (PR #1770, commit 8f3d178): Ihab adds rake_tasks block as part of massive Generator Overhaul (97 files changed)
  • Nov 18, 2025 (PR #2052, commit 3f6df6be9): Justin discovers and fixes duplicate execution issue by removing the block (~2 months later)

What We Learned

For Code Reviews

This incident highlights the challenge of reviewing massive PRs:

  • 97 files changed made it nearly impossible to catch subtle issues
  • The rake_tasks addition was 6 lines in a file that wasn't the focus of the PR
  • The duplicate loading bug only manifested during asset precompilation, not during normal development
  • Smaller, focused PRs would have made this easier to catch

For Testing

The duplicate execution bug was subtle:

  • Didn't cause failures—just slower builds (2x time)
  • Hard to notice locally—developers might not realize builds were taking twice as long
  • Only obvious in CI—where build times are closely monitored
  • Needed production-like scenarios—requires running rake assets:precompile to trigger

For Documentation

Better documentation of Rails::Engine automatic loading would help:

  • Many Rails guides show rake_tasks blocks without mentioning automatic loading
  • The Rails Engine guide doesn't clearly state when NOT to use rake_tasks
  • This leads to cargo-culting of the pattern

References