Add configurable loading strategy for generated component packs #1712
Conversation
WalkthroughThis pull request introduces changes across CI workflows, documentation, library configuration, and tests. It adds steps to set the Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Step as "Set packer version environment variable" Step
participant Env as $GITHUB_ENV
participant Next as Subsequent CI Steps
CI->>Step: Trigger workflow step
Step->>Env: Append "CI_PACKER_VERSION=${{ matrix.versions }}"
Next->>Env: Read CI_PACKER_VERSION for further actions
sequenceDiagram
participant App as Application
participant Config as ReactOnRails::Configuration
participant Helper as ReactOnRails::Helper
participant Browser as Client Browser
App->>Config: Initialize configuration with generated_component_packs_loading_strategy
Config-->>Helper: Provide loading strategy details
Helper->>Browser: Append JS pack tag with async/defer options based on strategy
Possibly related PRs
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
AbanoubGhadban
requested review from
Judahmeek,
alexeyr-ci and
justin808
and removed request for
Judahmeek,
alexeyr-ci and
justin808
- Replace `defer_generated_component_packs` with more flexible `generated_component_packs_loading_strategy` - Add support for `:async`, `:defer`, and `:sync` loading strategies - Validate loading strategy based on Shakapacker version - Update helper to support new loading strategy configuration - Add comprehensive specs for the new configuration option
AbanoubGhadban
force-pushed
the
abanoubghadban/pro521-add-support-for-async-scripts-in-the-view
branch
from
c3eae47 to
781c2c1
Compare
AbanoubGhadban
changed the title
![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
🧹 Nitpick comments (3)
script/convert (1)
24-24: Consider using a variable for version number.The version "6.6.0" appears in multiple places in the script. Consider extracting this to a variable at the top of the file to make future version updates easier to maintain.
-gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, '"shakapacker": "6.6.0",') +# At the top of the file +SHAKAPACKER_VERSION = "6.6.0" + +# Then use it here +gsub_file_content("../spec/dummy/package.json", /"shakapacker": "[^"]*",/, "\"shakapacker\": \"#{SHAKAPACKER_VERSION}\",")CHANGELOG.md (1)
21-29: Well-documented breaking change.The CHANGELOG clearly describes the new
generated_component_packs_loading_strategyconfiguration option and marks the removal ofdefer_generated_component_packsas a breaking change. This provides users with clear migration instructions.One suggestion: Consider adding a brief example of how to migrate from the old configuration to the new one to make the transition easier for users.
- Removed `defer_generated_component_packs` configuration option. You can use `generated_component_packs_loading_strategy` instead. [PR 1712](https://github.com/shakacode/react_on_rails/pull/1712) by [AbanoubGhadban](https://github.com/AbanoubGhadban). + +#### Migration Example + +```ruby +# Before +config.defer_generated_component_packs = true + +# After +config.generated_component_packs_loading_strategy = :defer # Or :async or :sync +```docs/release-notes/15.0.0.md (1)
43-43: Fix verb agreement in the configuration explanation.There's a minor grammatical error in the sentence.
- - The `force_load` configuration make `react-on-rails` hydrate components immediately as soon as their server-rendered HTML reaches the client, without waiting for the full page load. + - The `force_load` configuration makes `react-on-rails` hydrate components immediately as soon as their server-rendered HTML reaches the client, without waiting for the full-page load.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ded. - Theforce_loadconfiguration makereact-on-railshydrate components imm...(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~43-~43: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...hes the client, without waiting for the full page load. - If you want to keep the previ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Gemfile.lockis excluded by!**/*.lockspec/dummy/Gemfile.lockis excluded by!**/*.lockspec/dummy/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
.github/workflows/examples.yml(1 hunks).github/workflows/main.yml(1 hunks).github/workflows/rspec-package-specs.yml(1 hunks)CHANGELOG.md(1 hunks)Gemfile.development_dependencies(1 hunks)docs/release-notes/15.0.0.md(2 hunks)lib/react_on_rails/configuration.rb(6 hunks)lib/react_on_rails/controller.rb(1 hunks)lib/react_on_rails/helper.rb(1 hunks)script/convert(1 hunks)spec/dummy/package.json(1 hunks)spec/dummy/spec/helpers/react_on_rails_helper_spec.rb(1 hunks)spec/react_on_rails/configuration_spec.rb(2 hunks)spec/react_on_rails/utils_spec.rb(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/release-notes/15.0.0.md
[uncategorized] ~29-~29: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...h Streamed HTML as it will wait for the full page load before hydrating the components) ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~43-~43: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ded. - The force_load configuration make react-on-rails hydrate components imm...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~43-~43: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...hes the client, without waiting for the full page load. - If you want to keep the previ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
🔇 Additional comments (23)
Gemfile.development_dependencies (1)
3-3: Shakapacker version upgrade looks goodThis update to shakapacker 8.2.0 aligns with the introduction of the new
generated_component_packs_loading_strategyconfiguration option mentioned in the PR description.spec/dummy/package.json (1)
53-53: Shakapacker version upgrade is consistentThe shakapacker NPM package update to version 8.2.0 correctly matches the version update in the Gemfile, ensuring consistency between Ruby and JavaScript dependencies.
.github/workflows/examples.yml (1)
91-93: Environment variable step simplification suggestionThe step for setting the CI_PACKER_VERSION environment variable looks good and ensures the appropriate packer version is available in the CI environment.
Consider simplifying to just:
- echo "CI_PACKER_VERSION=${{ matrix.versions }}" >> $GITHUB_ENV + CI_PACKER_VERSION=${{ matrix.versions }} >> $GITHUB_ENV.github/workflows/rspec-package-specs.yml (1)
52-52: Improved environment variable settingUsing the matrix version directly as the CI_PACKER_VERSION is a good simplification from the previous conditional approach.
.github/workflows/main.yml (1)
193-195: Adding environment variable for Shakapacker version testing - good approach.The addition of this step sets the CI_PACKER_VERSION environment variable based on the matrix version, which will be used to validate loading strategies against different Shakapacker versions. This is a clean implementation that properly integrates with the GitHub Actions workflow.
script/convert (1)
16-16: Improved version matching flexibility.The updated regex pattern matches any version string rather than a hardcoded version, making the script more maintainable when the Shakapacker version changes in the future.
lib/react_on_rails/controller.rb (1)
15-19: Good enhancement to redux_store method.Adding the
force_loadparameter with proper default handling improves the flexibility of the method. This change complements the new loading strategy configuration and maintains backward compatibility by defaulting to the global configuration when not specified.spec/react_on_rails/utils_spec.rb (1)
13-15: Looks consistent with naming changes.Renaming the environment condition to
"oldest"and"newest"aligns with the updated workflows. The logic remains clear, and no issues stand out.spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (2)
53-59: Ensure test scenario coverage for other loading strategies.This test checks "oldest" vs. everything else, implicitly treating all non-"oldest" versions as async. If you also support
:syncor:deferin other contexts, consider adding a separate test branch here to solidify coverage of those scenarios.
63-105: Good approach to testing async and defer modes.Redefining
append_javascript_pack_tagto acceptasync:ensures backward compatibility checks with older Shakapacker versions. Restoring the original method within an ensure block prevents negative side effects on subsequent tests. No concerns here.lib/react_on_rails/helper.rb (1)
425-430: Well-structured conditional assignment for script loading strategy.Assigning
deferandasyncbased ongenerated_component_packs_loading_strategyis clear and maintains backward compatibility. The inline remark about older Shakapacker versions is helpful. Nothing to fix.docs/release-notes/15.0.0.md (4)
24-32: Well-structured documentation for the new loading strategies feature.The documentation clearly explains the three available loading strategies and their use cases. This will help users understand when to use each strategy based on their Shakapacker version.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...h Streamed HTML as it will wait for the full page load before hydrating the components) ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
29-29: Consider updating the description of the:deferstrategy.There is a grammar issue with the description of the streamed HTML support.
- - `:defer` - Defers script execution until after page load (Doesn't work well with Streamed HTML as it will wait for the full page load before hydrating the components) + - `:defer` - Defers script execution until after page load (Doesn't work well with Streamed HTML as it will wait for the full-page load before hydrating the components)🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...h Streamed HTML as it will wait for the full page load before hydrating the components) ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
37-40: Clear explanation of the breaking changes.The documentation effectively communicates the removal of the deprecated option and the introduction of the new configuration option with its default values.
62-67: Clear migration instructions.The migration section provides straightforward guidance for users transitioning from the old configuration to the new one. The recommendation to use
:asyncfor optimal performance with newer Shakapacker versions is helpful.lib/react_on_rails/configuration.rb (6)
51-52: Good configuration structure for the new option.Adding the new configuration option with a trailing comma for the previous option maintains consistent formatting.
67-68: Proper attribute accessibility for the new configuration option.The attribute has been correctly added to the accessor list, maintaining alphabetical ordering.
79-80: Correctly replaced deprecated parameter in the initializer.The
defer_generated_component_packsparameter has been properly replaced withgenerated_component_packs_loading_strategy.
126-127: Added initialization for the new configuration option.The new option is correctly initialized from the constructor parameter.
142-143: Added validation call for the new option.The call to validate the new option has been properly added to the
setup_config_valuesmethod.
155-179: Well-implemented validation for the loading strategy.The validation logic is thorough, handling different Shakapacker versions appropriately:
- Sets
:asyncas default for newer versions- Sets
:syncas default for older versions with a helpful warning- Prevents using
:asyncwith older versions- Validates that the strategy is one of the supported values
The error messages are clear and actionable, making it easy for users to fix configuration issues.
spec/react_on_rails/configuration_spec.rb (2)
268-269: Updated expectation count for method calls.The expectation for
shakapacker_version_requirement_met?has been updated fromthricetotwice, which likely reflects changes in how this method is called due to the new configuration option.
281-368: Comprehensive test coverage for the new configuration option.The tests thoroughly cover all aspects of the new loading strategy option:
- Defaults based on Shakapacker version
- Validation of accepted values
- Error handling for invalid values
- Warning messages for older Shakapacker versions
This ensures the configuration logic works correctly across different scenarios and version combinations.
Co-authored-by: Alexey Romanov <[email protected]>
| @@ -12,9 +12,11 @@ module Controller | |||
| # | |||
| # Be sure to include view helper `redux_store_hydration_data` at the end of your layout or view | |||
| # or else there will be no client side hydration of your stores. | |||
| def redux_store(store_name, props: {}) | |||
| def redux_store(store_name, props: {}, force_load: nil) | |||
| force_load = ReactOnRails.configuration.force_load if force_load.nil? | |||
There was a problem hiding this comment.
This change should be part of #1644 PR, but seems we missed it
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/release-notes/15.0.0.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/release-notes/15.0.0.md
[uncategorized] ~29-~29: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...h Streamed HTML as it will wait for the full page load before hydrating the components) ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~43-~43: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ded. - The force_load configuration make react-on-rails hydrate components imm...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~43-~43: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...hes the client, without waiting for the full page load. - If you want to keep the previ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: dummy-app-integration-tests (newest)
- GitHub Check: dummy-app-integration-tests (oldest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: rspec-package-tests (newest)
🔇 Additional comments (2)
docs/release-notes/15.0.0.md (2)
24-32: Enhanced Script Loading Strategies Section: Clarity and Consistency
This new section clearly introduces thegenerated_component_packs_loading_strategyconfiguration and lists the three supported loading strategies. The bullet-point descriptions are informative.
- Nitpick: Consider reviewing the phrase “Streamed HTML” in the description for
:defer(line 29). Using a hyphen (e.g. “streamed-HTML”) might improve clarity when functioning as a compound adjective.🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...h Streamed HTML as it will wait for the full page load before hydrating the components) ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
62-67: Script Loading Strategy Migration Section: Clear Migration Guidelines
The migration instructions for users transitioning fromdefer_generated_component_packstogenerated_component_packs_loading_strategyare concise and unambiguous. The mapping of previous boolean values to the new symbolic strategies is clearly outlined.
| - The `defer_generated_component_packs` configuration has been removed. Use `generated_component_packs_loading_strategy` instead. | ||
| - The `generated_component_packs_loading_strategy` defaults to `:async` for Shakapacker ≥ 8.2.0 and `:sync` for Shakapacker < 8.2.0. | ||
| - The `force_load` configuration now defaults to `true`. | ||
| - The new default values of `generated_component_packs_loading_strategy: :async` and `force_load: true` work together to optimize component hydration. Components now hydrate as soon as their code and server-rendered HTML are available, without waiting for the full page to load. This parallel processing significantly improves time-to-interactive by eliminating the traditional waterfall of waiting for page load before beginning hydration (It's critical for streamed HTML). | ||
|
|
||
| - The previous need for deferring scripts to prevent race conditions has been eliminated due to improved hydration handling. Making scripts not defer is critical to execute the hydration scripts early before the page is fully loaded. | ||
| - The `force_load` configuration make `react-on-rails` hydrate components immediately as soon as their server-rendered HTML reaches the client, without waiting for the full page load. | ||
| - If you want to keep the previous behavior, you can set `defer_generated_component_packs: true` or `force_load: false` in your `config/initializers/react_on_rails.rb` file. | ||
| - If you want to keep the previous behavior, you can set `generated_component_packs_loading_strategy: :defer` or `force_load: false` in your `config/initializers/react_on_rails.rb` file. |
There was a problem hiding this comment.
Component Hydration Changes: Verb Agreement and Clarity
The explanatory bullet points are generally clear; however, note the grammatical issue on line 43 where the phrase:
“The force_load configuration make react-on-rails hydrate components immediately…”
should use the singular verb “makes” to agree with “configuration.”
- Suggested Fix:
- The `force_load` configuration make `react-on-rails` hydrate components immediately...
+ The `force_load` configuration makes `react-on-rails` hydrate components immediately...📝 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.
| - The `defer_generated_component_packs` configuration has been removed. Use `generated_component_packs_loading_strategy` instead. | |
| - The `generated_component_packs_loading_strategy` defaults to `:async` for Shakapacker ≥ 8.2.0 and `:sync` for Shakapacker < 8.2.0. | |
| - The `force_load` configuration now defaults to `true`. | |
| - The new default values of `generated_component_packs_loading_strategy: :async` and `force_load: true` work together to optimize component hydration. Components now hydrate as soon as their code and server-rendered HTML are available, without waiting for the full page to load. This parallel processing significantly improves time-to-interactive by eliminating the traditional waterfall of waiting for page load before beginning hydration (It's critical for streamed HTML). | |
| - The previous need for deferring scripts to prevent race conditions has been eliminated due to improved hydration handling. Making scripts not defer is critical to execute the hydration scripts early before the page is fully loaded. | |
| - The `force_load` configuration make `react-on-rails` hydrate components immediately as soon as their server-rendered HTML reaches the client, without waiting for the full page load. | |
| - If you want to keep the previous behavior, you can set `defer_generated_component_packs: true` or `force_load: false` in your `config/initializers/react_on_rails.rb` file. | |
| - If you want to keep the previous behavior, you can set `generated_component_packs_loading_strategy: :defer` or `force_load: false` in your `config/initializers/react_on_rails.rb` file. | |
| - The `defer_generated_component_packs` configuration has been removed. Use `generated_component_packs_loading_strategy` instead. | |
| - The `generated_component_packs_loading_strategy` defaults to `:async` for Shakapacker ≥ 8.2.0 and `:sync` for Shakapacker < 8.2.0. | |
| - The `force_load` configuration now defaults to `true`. | |
| - The new default values of `generated_component_packs_loading_strategy: :async` and `force_load: true` work together to optimize component hydration. Components now hydrate as soon as their code and server-rendered HTML are available, without waiting for the full page to load. This parallel processing significantly improves time-to-interactive by eliminating the traditional waterfall of waiting for page load before beginning hydration (It's critical for streamed HTML). | |
| - The previous need for deferring scripts to prevent race conditions has been eliminated due to improved hydration handling. Making scripts not defer is critical to execute the hydration scripts early before the page is fully loaded. | |
| - The `force_load` configuration **makes** `react-on-rails` hydrate components immediately as soon as their server-rendered HTML reaches the client, without waiting for the full page load. | |
| - If you want to keep the previous behavior, you can set `generated_component_packs_loading_strategy: :defer` or `force_load: false` in your `config/initializers/react_on_rails.rb` file. |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: This verb does not appear to agree with the subject. Consider using a different form.
Context: ...ded. - The force_load configuration make react-on-rails hydrate components imm...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~43-~43: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...hes the client, without waiting for the full page load. - If you want to keep the previ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
defer_generated_component_packswith more flexiblegenerated_component_packs_loading_strategy:async,:defer, and:syncloading strategiesSummary
Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.
Pull Request checklist
Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by
~.Add the CHANGELOG entry at the top of the file.
Other Information
Remove this paragraph and mention any other important and relevant information such as benchmarks.
This change is
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
shakapackergem, to ensure better compatibility and performance.