diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c6d318fd4..22bfdc1660 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,8 +37,14 @@ Changes since the last non-beta release. #### Changed -- **Shakapacker 9.0.0 Upgrade**: Upgraded Shakapacker from 8.2.0 to 9.0.0 with Babel transpiler configuration for compatibility. Key changes include: +- **Pro-Only Feature Validation**: Added validation for Pro-only features with clear error messages: + - `immediate_hydration = true` requires React on Rails Pro. This setting should be configured in `config/initializers/react_on_rails_pro.rb` for Pro users. + - `generated_component_packs_loading_strategy = :async` requires React on Rails Pro. Non-Pro users can use `:defer` or `:sync` loading strategies. + - Non-Pro users attempting to use Pro-only features will receive a clear error message in development/test environments directing them to either remove the settings or purchase a Pro license. + - In production, errors are logged without crashing the application for graceful degradation. + [PR 1983](https://github.com/shakacode/react_on_rails/pull/1983) by [justin808](https://github.com/justin808). +- **Shakapacker 9.0.0 Upgrade**: Upgraded Shakapacker from 8.2.0 to 9.0.0 with Babel transpiler configuration for compatibility. Key changes include: - Configured `javascript_transpiler: babel` in shakapacker.yml (Shakapacker 9.0 defaults to SWC which has PropTypes handling issues) - Added precompile hook support via `bin/shakapacker-precompile-hook` for ReScript builds and pack generation - Configured CSS Modules to use default exports (`namedExport: false`) for backward compatibility with existing `import styles from` syntax @@ -100,7 +106,6 @@ To migrate to React on Rails Pro: **Note:** If you're not using any of the Pro-only methods listed above, no changes are required. - **Pro-Specific Configurations Moved to Pro Gem**: The following React Server Components (RSC) configurations have been moved from `ReactOnRails.configure` to `ReactOnRailsPro.configure`: - - `rsc_bundle_js_file` - Path to the RSC bundle file - `react_server_client_manifest_file` - Path to the React server client manifest - `react_client_manifest_file` - Path to the React client manifest @@ -126,7 +131,6 @@ To migrate to React on Rails Pro: See the [React on Rails Pro Configuration docs](https://github.com/shakacode/react_on_rails/blob/master/react_on_rails_pro/docs/configuration.md) for more details. - **Streaming View Helpers Moved to Pro Gem**: The following view helpers have been removed from the open-source gem and are now only available in React on Rails Pro: - - `stream_react_component` - Progressive SSR using React 18+ streaming - `rsc_payload_react_component` - RSC payload rendering @@ -151,12 +155,10 @@ To migrate to React on Rails Pro: #### New Features - **Server Bundle Security**: Added new configuration options for enhanced server bundle security and organization: - - `server_bundle_output_path`: Configurable directory (relative to the Rails root) for server bundle output (default: "ssr-generated"). If set to `nil`, the server bundle will be loaded from the same public directory as client bundles. [PR 1798](https://github.com/shakacode/react_on_rails/pull/1798) by [justin808](https://github.com/justin808) - `enforce_private_server_bundles`: When enabled, ensures server bundles are only loaded from private directories outside the public folder (default: false for backward compatibility) [PR 1798](https://github.com/shakacode/react_on_rails/pull/1798) by [justin808](https://github.com/justin808) - **Improved Bundle Path Resolution**: Bundle path resolution for server bundles now works as follows: - - If `server_bundle_output_path` is set, the server bundle is loaded from that directory. - If `server_bundle_output_path` is not set, the server bundle falls back to the client bundle directory (typically the public output path). - If `enforce_private_server_bundles` is enabled: @@ -268,7 +270,6 @@ See [Release Notes](docs/release-notes/16.0.0.md) for complete migration guide. - **`defer_generated_component_packs` deprecated** → use `generated_component_packs_loading_strategy` - Migration: - - `defer_generated_component_packs: true` → `generated_component_packs_loading_strategy: :defer` - `defer_generated_component_packs: false` → `generated_component_packs_loading_strategy: :sync` - Recommended: `generated_component_packs_loading_strategy: :async` for best performance @@ -677,7 +678,6 @@ for details. - Removal of config.symlink_non_digested_assets_regex as it's no longer needed with rails/webpacker. If any business needs this, we can move the code to a separate gem. - Added configuration option `same_bundle_for_client_and_server` with default `false` because - 1. Production applications would typically have a server bundle that differs from the client bundle 2. This change only affects trying to use HMR with react_on_rails with rails/webpacker. @@ -1395,13 +1395,11 @@ No changes. - Added automatic compilation of assets at precompile is now done by ReactOnRails. Thus, you don't need to provide your own `assets.rake` file that does the precompilation. [#398](https://github.com/shakacode/react_on_rails/pull/398) by [robwise](https://github.com/robwise), [jbhatab](https://github.com/jbhatab), and [justin808](https://github.com/justin808). - **Migration to v6** - - Do not run the generator again if you've already run it. - See [shakacode/react-webpack-rails-tutorial/pull/287](https://github.com/shakacode/react-webpack-rails-tutorial/pull/287) for an example of upgrading from v5. - To configure the asset compilation you can either - 1. Specify a `config/react_on_rails` setting for `build_production_command` to be nil to turn this feature off. 2. Specify the script command you want to run to build your production assets, and remove your `assets.rake` file. diff --git a/PR_1972_INVESTIGATION.md b/PR_1972_INVESTIGATION.md new file mode 100644 index 0000000000..cd01b8afaa --- /dev/null +++ b/PR_1972_INVESTIGATION.md @@ -0,0 +1,324 @@ +# PR #1972 Investigation: Component Registration Race Condition + +## Executive Summary + +PR #1972 attempted to fix intermittent CI test failures by changing the default `generated_component_packs_loading_strategy` from `:async` to `:defer`. This document provides an in-depth analysis of the race condition, the proposed solution, and recommendations for a better approach. + +## The Problem + +### Test Failures + +The following tests were failing intermittently in CI: + +- `spec/system/integration_spec.rb[1:1:6:1:2]` - "2 react components, 1 store, client only, defer" +- `spec/system/integration_spec.rb[1:1:6:2:2]` - "2 react components, 1 store, server side, defer" + +These tests check Redux shared store functionality where two components share the same store and typing in one component should update the other. + +### Root Cause Analysis + +#### The Race Condition + +With `async` script loading (default on Shakapacker >= 8.2.0): + +1. **Browser behavior**: When scripts have the `async` attribute, they: + + - Download in parallel (good for performance) + - Execute immediately when download completes (unpredictable order) + - Do not block HTML parsing + +2. **The problem with generated component packs**: + + ```html + + + + + + ``` + +3. **Race condition scenario**: + - If `client-bundle.js` finishes downloading first, it executes immediately + - React hydration starts before component registrations from generated packs + - Error: "Could not find component registered with name ComponentName" + +#### Why It's Intermittent + +The race condition depends on: + +- Network conditions +- File sizes (smaller files download faster) +- Browser caching +- Server response times + +This makes it particularly difficult to reproduce locally but common in CI environments with varying network conditions. + +## PR #1972 Solution Analysis + +### What Changed + +1. **Configuration default** (`lib/react_on_rails/configuration.rb`): + + ```ruby + # OLD: Defaulted to :async when Shakapacker >= 8.2.0, else :sync + # NEW: Always defaults to :defer + self.generated_component_packs_loading_strategy = :defer + ``` + +2. **Layout file** (`spec/dummy/app/views/layouts/application.html.erb`): + + ```erb + + + <%= javascript_pack_tag('client-bundle', defer: true) %> + ``` + +3. **Test expectations updated** to expect `:defer` as default + +### How Defer "Fixes" It + +With `defer`: + +- Scripts still download in parallel (fast) +- Scripts execute in DOM order after HTML parsing completes +- Generated component packs execute before main bundle (predictable) +- Component registrations complete before React hydration + +```html + + + + + +``` + +## The Real Issue + +### Why This Solution Is Problematic + +1. **Performance Impact**: + + - `async` provides better performance by executing scripts as soon as they're ready + - `defer` forces sequential execution, which can be slower + - Modern web apps benefit from async loading + +2. **Masks Architectural Problem**: + + - The real issue is that React hydration shouldn't depend on script execution order + - Components should be registered before hydration attempts to use them + - This is a timing/coordination problem, not a loading strategy problem + +3. **Doesn't Address Root Cause**: + - The race condition still exists with generated component packs + - We're just forcing a particular execution order to avoid it + - Better solution: ensure component registry is ready before hydration + +### The `uses_redux_shared_store?` Helper + +Before PR #1972, there was conditional logic: + +```ruby +# application_controller.rb +def uses_redux_shared_store? + action_name.in?(%w[ + index + server_side_redux_app + # ... other actions with shared stores + ]) +end +``` + +This recognized that **only certain pages need defer**. PR #1972 removed this nuance by forcing defer everywhere. + +## Recommended Approach + +### Option 1: Component Registry Timeout (Already Implemented!) + +React on Rails already has `component_registry_timeout` (default 5000ms): + +```ruby +# configuration.rb +component_registry_timeout: DEFAULT_COMPONENT_REGISTRY_TIMEOUT # 5000ms +``` + +This means the client-side code should **wait** for components to register before hydrating. The race condition might indicate: + +- The timeout isn't working correctly +- There's a bug in the component registration check +- The timeout is too short for CI environments + +**Investigation needed**: + +- Review `packages/react-on-rails/src/` for component registry logic +- Check if hydration properly waits for registrations +- Verify timeout is honored in all code paths + +### Option 2: Explicit Component Dependencies + +Make the main bundle explicitly wait for generated pack scripts: + +```javascript +// In generated component packs: +window.ReactOnRailsComponentsReady = window.ReactOnRailsComponentsReady || []; +window.ReactOnRailsComponentsReady.push('ComponentName'); + +// In client-bundle before hydration: +function waitForComponents(required, timeout = 5000) { + return new Promise((resolve, reject) => { + const check = () => { + if (required.every((name) => window.ReactOnRailsComponentsReady.includes(name))) { + resolve(); + } + }; + // Poll until ready or timeout + }); +} +``` + +### Option 3: Module Dependencies + +Use ES modules with dynamic imports: + +```javascript +// Instead of script tags, use: +const component = await import(`./generated/${componentName}`); +``` + +This gives explicit control over load order without sacrificing async benefits. + +### Option 4: Smart Loading Strategy + +Keep async as default but fall back to defer only when needed: + +```ruby +# Configuration that detects when defer is necessary +def required_loading_strategy + if @rendered_components.any? { |c| needs_guaranteed_order?(c) } + :defer + else + :async + end +end +``` + +## Test Analysis + +### The Failing Tests + +Looking at `spec/dummy/spec/system/integration_spec.rb:360-382`: + +```ruby +describe "2 react components, 1 store, client only, defer", :js do + include_examples "React Component Shared Store", "/client_side_hello_world_shared_store_defer" +end + +describe "2 react components, 1 store, server side, defer", :js do + include_examples "React Component Shared Store", "/server_side_hello_world_shared_store_defer" +end +``` + +These tests **specifically test defer functionality**. The fact that they fail with async is expected behavior! The routes ending in `_defer` are explicitly testing defer mode. + +**Key insight**: The failures might not be a bug but tests failing because: + +1. Default was changed from async to defer +2. Tests expected defer behavior +3. When default was async, these defer-specific tests used async instead + +## Recommendations + +### Immediate Actions + +1. **Revert PR #1972** ✅ (Already done) + +2. **Investigate component registry timeout**: + + - Review `packages/react-on-rails/src/ComponentRegistry.ts` + - Check `component_registry_timeout` implementation + - Add detailed logging to see when/why registrations fail + +3. **Reproduce the race condition locally**: + + ```bash + # Throttle network to simulate CI conditions + # Use browser DevTools Network tab -> Throttling + # Run tests multiple times to catch intermittent failures + ``` + +4. **Add instrumentation**: + ```javascript + console.log('[RoR] Component registered:', componentName, Date.now()); + console.log('[RoR] Attempting hydration:', componentName, Date.now()); + console.log('[RoR] Registry contents:', Object.keys(componentRegistry)); + ``` + +### Long-term Solutions + +1. **Fix the timing issue properly**: + + - Ensure `component_registry_timeout` works correctly + - Make hydration explicitly wait for required components + - Add warnings when components aren't registered in time + +2. **Make loading strategy configurable per-component**: + + ```ruby + react_component('ComponentName', props, loading_strategy: :defer) + ``` + +3. **Document when defer is needed**: + + - Update docs to explain async vs defer trade-offs + - Provide guidance on when to use each + - Explain the performance implications + +4. **Improve test reliability**: + - Add retries for tests with network dependencies + - Use `retry: 3` in RSpec for these specific tests + - Consider mocking/stubbing script loading in tests + +## Questions to Answer + +1. **Why does `component_registry_timeout` not prevent the race condition?** + + - Is it being used correctly? + - Is there a code path that bypasses it? + - Are generated component packs registering correctly? + +2. **Why do defer-specific tests fail with async default?** + + - Are the routes configured correctly? + - Should these tests explicitly set the loading strategy? + - Is there a bug in the configuration precedence? + +3. **Can we detect when defer is truly necessary?** + - Shared Redux stores? + - Inline component registration? + - Server-side rendering? + +## Conclusion + +PR #1972's solution works but treats the symptom rather than the disease. The real fix requires: + +1. Understanding why the component registry timeout doesn't prevent the race +2. Fixing the underlying timing/coordination issue +3. Keeping async as the default for performance +4. Using defer only when truly necessary (documented cases) + +The intermittent nature of the failures suggests a real race condition that needs proper synchronization, not just forced execution order. + +## Next Steps + +1. ✅ Revert PR #1972 +2. ⏳ Deep dive into component registry timeout implementation +3. ⏳ Reproduce failures locally with network throttling +4. ⏳ Add instrumentation to understand timing +5. ⏳ Implement proper synchronization fix +6. ⏳ Update documentation with clear guidance +7. ⏳ Create new PR with proper solution + +--- + +**Author**: Claude Code +**Date**: November 11, 2025 +**Status**: Investigation Complete, Awaiting Implementation diff --git a/docs/api-reference/configuration.md b/docs/api-reference/configuration.md index 96391204da..a558520e04 100644 --- a/docs/api-reference/configuration.md +++ b/docs/api-reference/configuration.md @@ -267,11 +267,17 @@ ReactOnRails.configure do |config| config.make_generated_server_bundle_the_entrypoint = false # Configuration for how generated component packs are loaded. - # Options: :sync, :async, :defer - # - :sync (default for Shakapacker < 8.2.0): Loads scripts synchronously - # - :async (default for Shakapacker ≥ 8.2.0): Loads scripts asynchronously for better performance - # - :defer: Defers script execution until after page load - config.generated_component_packs_loading_strategy = :async + # Options: :defer (default), :sync, :async (Pro only) + # + # - :defer (default for non-Pro, recommended): Scripts load in parallel but execute in order + # after HTML parsing. Prevents race conditions where components render before registration. + # - :sync: Scripts block HTML parsing while loading (not recommended, slowest option and can still cause race conditions in the non-pro version if the component is hydrated before its SSR-generated HTML appears in the DOM). + # - :async (Pro only): Scripts load and execute as soon as available. Requires Pro's + # immediate_hydration feature to prevent race conditions. + # + # Default: :defer (non-Pro) or :async (Pro with Shakapacker >= 8.2.0) + # Note: Shakapacker >= 8.2.0 required for :async support + config.generated_component_packs_loading_strategy = :defer # DEPRECATED: Use `generated_component_packs_loading_strategy` instead. # Migration: `defer_generated_component_packs: true` → `generated_component_packs_loading_strategy: :defer` @@ -279,11 +285,32 @@ ReactOnRails.configure do |config| # See [16.0.0 Release Notes](docs/release-notes/16.0.0.md) for more details. # config.defer_generated_component_packs = false - # Default is false - # React on Rails Pro (licensed) feature: When true, components hydrate immediately as soon as - # their server-rendered HTML reaches the client, without waiting for the full page load. - # This improves time-to-interactive performance. - config.immediate_hydration = false + ################################################################################ + # REACT ON RAILS PRO FEATURES + ################################################################################ + # The following features require React on Rails Pro and should be configured + # in config/initializers/react_on_rails_pro.rb using ReactOnRailsPro.configure: + # + # - immediate_hydration: When true, components hydrate immediately as soon as + # their server-rendered HTML reaches the client, without waiting for the full page load. + # This improves time-to-interactive performance. + # + # - generated_component_packs_loading_strategy = :async: Pro-only loading strategy that + # works with immediate_hydration to load component scripts asynchronously for maximum + # performance. Without Pro, :async can cause race conditions where components attempt + # to hydrate before their JavaScript is loaded. + # + # Example (in config/initializers/react_on_rails_pro.rb): + # ReactOnRailsPro.configure do |config| + # config.immediate_hydration = true + # end + # + # # In config/initializers/react_on_rails.rb: + # ReactOnRails.configure do |config| + # config.generated_component_packs_loading_strategy = :async + # end + # + # For more information, visit: https://www.shakacode.com/react-on-rails-pro ################################################################################ # I18N OPTIONS diff --git a/docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.md b/docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.md index ce9022e4d2..05194f7ae9 100644 --- a/docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.md +++ b/docs/core-concepts/auto-bundling-file-system-based-automated-bundle-generation.md @@ -47,6 +47,36 @@ config.auto_load_bundle = true > Example (dummy app): `auto_load_bundle` is set to `true` in the same initializer. > [Dummy initializer](https://github.com/shakacode/react_on_rails/blob/master/spec/dummy/config/initializers/react_on_rails.rb) +### Configure `generated_component_packs_loading_strategy` + +The `generated_component_packs_loading_strategy` option controls how generated component packs are loaded in the browser. This affects the `async` and `defer` attributes on the `