Add configurable loading strategy for generated component packs #1712
Conversation
WalkthroughThis pull request adds a new step to several GitHub Actions workflows to set the Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Actions Workflow
participant Env as $GITHUB_ENV
participant Test as RSpec Test Step
Workflow->>Env: Append "CI_PACKER_VERSION=${{ matrix.versions }}"
Env-->>Workflow: Updated environment variable
Workflow->>Test: Execute tests using CI_PACKER_VERSION
sequenceDiagram
participant Helper as ReactOnRails Helper
participant Config as Configuration
participant Renderer as JavaScript Pack Renderer
Helper->>Config: Retrieve generated_component_packs_loading_strategy
alt Strategy is :async
Helper->>Renderer: Append pack tag with async attribute
else Strategy is :defer
Helper->>Renderer: Append pack tag with defer attribute
else Strategy is :sync
Helper->>Renderer: Append pack tag without async/defer attributes
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
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.
| @@ -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.
We are in 15.0 pre-release. Though it may indeed be preferable to deprecate it (and set |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
CHANGELOG.md (1)
21-26: New Configuration Option Added:generated_component_packs_loading_strategy
The “Added” section now clearly documents that the new configuration option supportssync,async, anddeferstrategies. Please double-check that the wording is consistent with its usage throughout the codebase and consider including a brief example or note on why one should prefer the new option over the removed one.docs/release-notes/15.0.0.md (3)
26-32: Detailed Explanation of New Loading Strategies
The bullet list thoroughly explains that the newgenerated_component_packs_loading_strategyreplaces the deprecated option, and it enumerates the three strategies available:
:asyncfor asynchronous loading (default on Shakapacker ≥ 8.2.0),:defer(with a note about its suboptimal behavior with streamed HTML), and:syncfor synchronous loading (default on Shakapacker < 8.2.0).A couple of minor suggestions:
• Consider rephrasing the description of the:deferoption to clarify that its behavior—waiting until the full page load—may delay hydration on streamed pages.
• Verify whether any compound adjectives (such as “Streamed HTML”) require hyphenation for internal consistency, as flagged by static analysis.🧰 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)
40-43: Explanation of Optimized Hydration Behavior
The explanation here elaborates how the new defaults (generated_component_packs_loading_strategy: :asyncandforce_load: true) allow components to hydrate as soon as their code and server-rendered HTML are available. This detailed rationale is very helpful. For improved readability, consider splitting some of the longer sentences into shorter ones and reviewing any flagged compound adjective constructions for proper hyphenation.🧰 Tools
🪛 LanguageTool
[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)
44-46: Migration Options for Preserving Previous Behavior
This section provides guidance for users wishing to replicate the old behavior by settinggenerated_component_packs_loading_strategy: :deferor togglingforce_loadtofalse. A few stylistic tweaks might improve clarity:
• Replace “If we want to keep” with “If you want to retain” for consistency.
• For the Redux store note, consider rephrasing to clarify that Redux stores now adopt the globalforce_loadconfiguration by default, but you can override this on a per-store basis via theredux_storehelper.🧰 Tools
🪛 LanguageTool
[style] ~45-~45: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...zers/react_on_rails.rbfile. - If we want to keep the original behavior offorce_lo...(REP_WANT_TO_VB)
[uncategorized] ~46-~46: The grammatical number of this noun doesn’t look right. Consider replacing it.
Context: ...ce_loadconfiguration. - Redux store supportforce_loadoption now and it usesco...(AI_EN_LECTOR_REPLACEMENT_NOUN_NUMBER)
[uncategorized] ~46-~46: Use a comma before ‘and’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ux store supportforce_loadoption now and it usesconfig.force_loadvalue as th...(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)docs/release-notes/15.0.0.md(2 hunks)lib/react_on_rails/configuration.rb(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/react_on_rails/configuration.rb
🧰 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: 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)
🔇 Additional comments (5)
CHANGELOG.md (1)
27-29: Deprecated Option Removed
The “Removed (Breaking Changes)” section effectively informs users that the deprecateddefer_generated_component_packsoption has been replaced. This is clear and well-documented—but ensure that the migration notes elsewhere (and in the release notes) explicitly guide users on what values to use as replacements.docs/release-notes/15.0.0.md (4)
24-24: New Section Heading: "Enhanced Script Loading Strategies"
The new section heading immediately signals the introduction of the updated configuration option. This serves as a clear marker for users to learn about the new loading strategies.
37-39: Deprecation and Default Value Updates in Hydration Settings
This segment succinctly describes that:
• The olddefer_generated_component_packsconfiguration is deprecated,
• The new option defaults to:asyncfor Shakapacker ≥ 8.2.0 and to:syncfor earlier versions, and
•force_loadnow defaults totrue.These points are clearly stated and help users understand the breaking changes.
48-60: Updated Async Handling forreactOnRailsPageLoaded()
The explanation and code snippet clearly describe thatReactOnRails.reactOnRailsPageLoaded()is now asynchronous and must be awaited. The before-and-after examples will be very useful for developers updating their code.
62-67: Script Loading Strategy Migration Instructions
The migration section succinctly maps the olddefer_generated_component_packsvalues to the newgenerated_component_packs_loading_strategysettings. The instructions are clear; you might consider adding a brief rationale for these mappings to further aid users in understanding the performance implications.
- 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
Co-authored-by: Alexey Romanov <alexey.romanov+ci5@shakacode.com>
AbanoubGhadban
force-pushed
the
abanoubghadban/pro521-add-support-for-async-scripts-in-the-view
branch
from
27c7033 to
0c50b80
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (1)
54-59: The conditional expectation adds complexity for CI version compatibility.This conditional logic accommodates different behavior based on the Shakapacker version in the CI environment. The approach is sound, but consider adding a comment explaining why this conditional is necessary - it would make the test more maintainable for future developers.
if ENV["CI_PACKER_VERSION"] == "oldest" + # Older Shakapacker versions don't support async loading, so we only check for defer: false expect(helper).to have_received(:append_javascript_pack_tag).with("generated/component_name", { defer: false }) else + # Newer Shakapacker versions support async loading, so we check for both options expect(helper).to have_received(:append_javascript_pack_tag) .with("generated/component_name", { defer: false, async: true }) enddocs/release-notes/15.0.0.md (1)
24-32: Good overview of the new loading strategies.The documentation clearly explains the new configuration option and the three loading strategies. This is helpful for users to understand how to configure their applications.
On line 29, consider hyphenating "Streamed-HTML" as it's used as a compound adjective:
- `: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)
📜 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 (13)
.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(5 hunks)lib/react_on_rails/controller.rb(1 hunks)lib/react_on_rails/helper.rb(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)
🚧 Files skipped from review as they are similar to previous changes (10)
- spec/dummy/package.json
- spec/react_on_rails/utils_spec.rb
- .github/workflows/examples.yml
- .github/workflows/rspec-package-specs.yml
- CHANGELOG.md
- Gemfile.development_dependencies
- .github/workflows/main.yml
- lib/react_on_rails/controller.rb
- lib/react_on_rails/helper.rb
- lib/react_on_rails/configuration.rb
🧰 Additional context used
🧬 Code Definitions (1)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (2)
lib/react_on_rails/configuration.rb (1)
configuration(15-55)lib/react_on_rails/helper.rb (1)
load_pack_for_generated_component(417-433)
🪛 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: 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: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: build
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: rspec-package-tests (oldest)
🔇 Additional comments (6)
spec/dummy/spec/helpers/react_on_rails_helper_spec.rb (2)
69-78: Nice technique for temporarily redefining the method.The temporary method redefinition approach is a good solution for testing with older Shakapacker versions that may not support the async option. The comments explain the need clearly, and the implementation with begin/ensure block ensures proper cleanup.
93-105: Well-structured test for defer loading strategy.The defer loading test is straightforward and properly tests that the correct loading attribute is passed to the script tag. The before block to set up the configuration and the expectations are clear and concise.
docs/release-notes/15.0.0.md (2)
37-44: Clear explanation of breaking changes.The explanation of the breaking changes is thorough and provides important context about why the changes were made and how they improve component hydration.
🧰 Tools
🪛 LanguageTool
[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)
65-70: Helpful migration guide.The migration instructions provide clear guidance on how to transition from the deprecated
defer_generated_component_packsto the newgenerated_component_packs_loading_strategy. This will help users adopt the new configuration option more easily.spec/react_on_rails/configuration_spec.rb (2)
268-269: Updated expectation count for calls to the shakapacker_version_requirement_met? method.The change from expecting three calls to two calls to
shakapacker_version_requirement_met?is appropriate. This adjustment aligns with changes in the implementation where one of the checks was likely consolidated or removed.
281-368: Comprehensive test coverage for the new loading strategy configuration.The tests thoroughly cover all aspects of the new
generated_component_packs_loading_strategyconfiguration:
- Default values for different Shakapacker versions
- Validation of valid configuration values
- Error handling for invalid values
- Version-specific behavior (e.g., rejecting
:asyncfor older versions)- Warning logging when appropriate
The test suite is well-structured with clear context separation and covers both the happy path and error cases. This ensures the new configuration option behaves correctly across different environments and configurations.
Judahmeek
deleted the
abanoubghadban/pro521-add-support-for-async-scripts-in-the-view
branch
defer_generated_component_packsand added 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
Chores
Documentation