-
-
Notifications
You must be signed in to change notification settings - Fork 638
move immediate hydration to pro #1996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move immediate hydration to pro #1996
Conversation
Move immediate_hydration from ReactOnRails to ReactOnRailsPro configuration since it's a Pro-only feature. Changes: - Add DEFAULT_IMMEDIATE_HYDRATION constant (default: false) - Add immediate_hydration to attr_accessor list - Add immediate_hydration parameter to initialize method - Add immediate_hydration assignment in initialize - Add immediate_hydration to configuration instantiation This prepares for removing immediate_hydration from the core ReactOnRails configuration in the next commit.
Add a helper method to access the immediate_hydration configuration from ReactOnRailsPro when Pro is available. Changes: - Add immediate_hydration_config class method that returns false when Pro is not available, or ReactOnRailsPro.configuration. immediate_hydration when Pro is available - Update disable_pro_render_options_if_not_licensed to use the new helper instead of accessing ReactOnRails.configuration This prepares for removing immediate_hydration from ReactOnRails configuration.
Update all code that accesses immediate_hydration configuration to use the new ProUtils.immediate_hydration_config helper. Changes: - helper.rb: Use ProUtils.immediate_hydration_config in redux_store - controller.rb: Use ProUtils.immediate_hydration_config in redux_store - render_options.rb: Update immediate_hydration method to use ProUtils.immediate_hydration_config instead of retrieve_configuration_value_for This prepares for removing immediate_hydration from the ReactOnRails configuration class.
This removes immediate_hydration config from the core ReactOnRails configuration as it is now managed by ReactOnRailsPro::Configuration. All code now uses ReactOnRails::ProUtils.immediate_hydration_config to access the Pro configuration value.
Moves immediate_hydration configuration check from ReactOnRails config analysis to ReactOnRailsPro config analysis. The doctor now reads the react_on_rails_pro.rb initializer file to display immediate_hydration settings when the Pro package is configured.
Changes test setup to stub ReactOnRails::ProUtils.immediate_hydration_config instead of setting config.immediate_hydration on ReactOnRails configuration. This aligns with the new architecture where immediate_hydration is a Pro-only configuration option. Also removes config.immediate_hydration from the dummy app initializer as it is no longer a valid ReactOnRails configuration option.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Code Review: Move immediate_hydration to ProSummaryThis PR successfully migrates the ✅ Strengths1. Clean Architecture
2. Comprehensive Migration
3. Test Quality Improvements
4. Documentation Updates
🔍 Potential Issues1. Logic Error in option_enabled = if raw_options[option].nil?
# Use the Pro config helper to get the global config value
immediate_hydration_configProblem: This hardcodes Current behavior: Works only because Fix: option_enabled = if raw_options[option].nil?
# Dynamically call the Pro config method for this specific option
method_name = "\#{option}_config"
respond_to?(method_name, true) ? send(method_name) : false
else
raw_options[option]
endOr simpler for now: option_enabled = if raw_options[option].nil?
send("\#{option}_config")
else
raw_options[option]
end2. Missing CHANGELOG Entry 📋
This is a breaking change for users who:
Suggested entry: #### [PR 1996](https://github.com/shakacode/react_on_rails/pull/1996) by [AbanoubGhadban](https://github.com/AbanoubGhadban)
**Breaking Change:** The `immediate_hydration` configuration option is now a React on Rails Pro exclusive feature. If you were using this option without a Pro license, you'll need to either:
1. Remove `config.immediate_hydration = true` from your `config/initializers/react_on_rails.rb`
2. Upgrade to React on Rails Pro and move the setting to `config/initializers/react_on_rails_pro.rb`
The option will be silently ignored if Pro is not installed.3. PR Description is Template Text 📝
🎯 Security & PerformanceSecurity: ✅ No concerns. The feature gracefully degrades when Pro is not licensed. Performance: ✅ Negligible impact. The 📊 Test CoverageCurrent coverage: ✅ Good
Recommendation: Consider adding a test case for: context "when Pro is not available" do
it "returns false for immediate_hydration_config" do
allow(ReactOnRails::ProUtils).to receive(:support_pro_features?).and_return(false)
expect(ReactOnRails::ProUtils.immediate_hydration_config).to eq(false)
end
end📋 Pre-Merge ChecklistPer
🎉 Overall AssessmentQuality: High - clean refactoring with good test practices The architecture is solid and sets a good pattern for future Pro features. The test improvements (using stubs instead of global config mutation) are a welcome bonus. Great work! 🚀 |
|
See #1997. |
Summary
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