Unoptimized partial reload warnings #165
Conversation
| @@ -12,5 +12,20 @@ class Engine < ::Rails::Engine | |||
| include ::InertiaRails::Controller | |||
| end | |||
| end | |||
|
|
|||
| initializer "inertia_rails.subscribe_to_notifications" do | |||
| if Rails.env.development? || Rails.env.test? | |||
There was a problem hiding this comment.
I want the default experience to add warnings to the logs in development / test only.
| set_vary_header | ||
|
|
||
| validate_partial_reload_optimization if rendering_partial_component? | ||
|
|
||
| return render_inertia_response if @request.headers['X-Inertia'] | ||
| return render_ssr if configuration.ssr_enabled rescue nil | ||
|
|
||
| render_full_page |
There was a problem hiding this comment.
Moved these into methods purely for readability. IMO this method should be super readable since it spells out the core functionality
| if @deep_merge | ||
| shared_data.deep_symbolize_keys.deep_merge!(props.deep_symbolize_keys) | ||
| def merged_props | ||
| @merged_props ||= if @deep_merge |
There was a problem hiding this comment.
memoizing just to future proof. it's only called once, but can't hurt
| @@ -166,5 +146,72 @@ def excluded_by_only_partial_keys?(path_with_prefixes) | |||
| def excluded_by_except_partial_keys?(path_with_prefixes) | |||
| partial_except_keys.present? && (path_with_prefixes & partial_except_keys).any? | |||
| end | |||
|
|
|||
| def validate_partial_reload_optimization | |||
There was a problem hiding this comment.
don't love the long method name... i'm open to suggestions!
| paths: prop_transformer.unoptimized_prop_paths, | ||
| controller: controller, | ||
| action: controller.action_name, |
There was a problem hiding this comment.
@skryukov you mentioned wanting this feature to accept a callable... with this anyone can register a subscriber with that sort of functionality. any other options you'd like to see passed to the subscriber context?
| end | ||
| end | ||
|
|
||
| class PropTransformer |
There was a problem hiding this comment.
I moved the transformation logic into a nested subclass so 1) all the behavior is encapsulated in a single place and 2) I had a place to store unoptimized_prop_paths while traversing the props hash. The transformation code is the same, and it was super easy to drop in the track_unoptimized_prop call.
The caller (above) uses a .select_transformed method with a block, so the caller only has to worry about filtering now (instead of both filtering and transforming). We also get to remove the admittedly-clunky keep/dont keep enum!
I considered pulling this out of the renderer class, but I don't really see a need for it. It's tested by our request specs and never going to be used outside that context.
| @@ -153,11 +153,60 @@ | |||
| end | |||
| end | |||
| end | |||
|
|
|||
| describe '.action_on_unoptimized_partial_reload' do | |||
There was a problem hiding this comment.
Much cleaner specs than in #154 :)
I revisited this after:
The refactored renderer was much easier to add this feature to, and I liked Rails' use of ActiveSupport::Notifications for logging/raising errors. So here we are!
TODO