-
Notifications
You must be signed in to change notification settings - Fork 69
Support dot notation for :only keys in partial reloads #163
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
Conversation
lib/inertia_rails/renderer.rb
Outdated
|
||
drop_partial_except_keys(_props) if rendering_partial_component? | ||
deep_transform_props _props do |prop, path| | ||
next [:dont_keep] unless keep_prop?(prop, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why return this array from the block now? Well, the block has two jobs 1) to filter out props we don't need and 2) transform the keepers into their final format. We want track those separately so that the transformation can return things that are nil/false-y. (i think the typical pattern here would be next nil unless some_condition?
)
(This block will also be a nice place to rewrite the logic from #154 )
lib/inertia_rails/renderer.rb
Outdated
|
||
hash.transform_values {|value| deep_transform_values(value, &block)} | ||
end | ||
def deep_transform_props(props, parent_path = '', &block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar logic as the previous method, but we're now also dealing with the results of the filtering within the hash we're building up and conditionally setting keys.
@@ -138,5 +134,35 @@ def resolve_component(component) | |||
|
|||
configuration.component_path_resolver(path: controller.controller_path, action: controller.action_name) | |||
end | |||
|
|||
def keep_prop?(prop, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the prop filtering logic now lives in one concise place. i debated pulling this out into a class, but I don't think the cleanliness is worth the extra indirection.
return true if prop.is_a?(AlwaysProp) | ||
|
||
if rendering_partial_component? | ||
path_with_prefixes = path_prefixes(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth nothing that if we're in a partial reload, we'll end up calculating this array for every single prop key. That should still be way faster than fully evaluating all of those props, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this, looks really clean!
lib/inertia_rails/renderer.rb
Outdated
end | ||
|
||
def excluded_by_except_partial_keys?(path_with_prefixes) | ||
partial_except_keys.present? && (path_with_prefixes & partial_except_keys.map(&:to_s)).any? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can replace .filter_map(&:to_sym)
in the partial_except_keys
and then stop calling .map(&:to_s)
here
And same for partial_keys
partial_except_keys.present? && (path_with_prefixes & partial_except_keys.map(&:to_s)).any? | |
partial_except_keys.present? && (path_with_prefixes & partial_except_keys).any? |
lib/inertia_rails/renderer.rb
Outdated
parts = key.to_s.split('.').map(&:to_sym) | ||
*initial_keys, last_key = parts | ||
current = initial_keys.any? ? hash.dig(*initial_keys) : hash | ||
if prop.is_a?(Hash) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that 'what_about_empty_hash' => {},
example will fail, here's a fix:
if prop.is_a?(Hash) | |
if prop.is_a?(Hash) && prop.any? |
lib/inertia_rails/renderer.rb
Outdated
end | ||
def deep_transform_props(props, parent_path = '', &block) | ||
props.reduce({}) do |transformed_props, (key, prop)| | ||
current_path = [parent_path, key].reject(&:empty?).join('.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can simplify this by sticking with arrays:
current_path = [parent_path, key].reject(&:empty?).join('.') | |
current_path = path + [key] |
Deploying inertia-rails with
|
Latest commit: |
c4606ef
|
Status: | ✅ Deploy successful! |
Preview URL: | https://8d1fa703.inertia-rails.pages.dev |
Branch Preview URL: | https://dot-notation-for-only-props.inertia-rails.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! only one small comment
lib/inertia_rails/renderer.rb
Outdated
|
||
drop_partial_except_keys(_props) if rendering_partial_component? | ||
deep_transform_props _props do |prop, path| | ||
next [:dont_keep] unless keep_prop?(prop, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: could we move these symbol values to constants since they're acting as an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure! as an aside, :dont_keep
doesn't do anything at all... it's just there to reveal intention
@bknoles I spent an absurd amount of time troubleshooting why Is it possible to add this caveat to the rails documentation? |
Hey @chasegiunta can you describe your situation with some more detail? I need more information to figure out if you've stumbled into a bug or just a misunderstanding we could fix in the docs. |
@bknoles sure. I mean, easiest example would be I have a
Writing a profile form, updating just the user's first & last name (no need to reload all the orgs, preferences, etc again). So we
Well, that's not ideal. Now if I have email, orgs, etc displayed elsewhere on the page (not in forms, just displayed in the UI), all that data is lost. Why it doesn't work makes sense: it's not a Inertia core feature. The Inertia frontend library would have to be aware of which properties being returned in the response are meant to be merged into frontend props. It doesn't currently do that. |
Oh I see. Yea, partial data reloads overwrite props rather than merging them. The use case for dot notation :only keys in the Laravel adapter's test suite is instructive: the :only data really just serves to filter out a Have you looked at the merging props section of the docs? I think it might be what you're after? Something like: def create
render 'Some/Page', props: {
# :only keys should filter this down to a tiny response; performance benefits depends on how you render the JSON
current_user: InertiaRails.merge(current_user)
}
end (caveat: i haven't built merge props into one of our apps so there may be a nuance my cursory review isn't capturing) |
@bknoles I tried playing with it, yeah, but couldn't quite get it. I think the syntax would actually be |
Ah yea, merging won't work either now that I think it through. We actually call this out in the docs: The purpose of a partial reload is to prevent unnecessary computations by filtering out props before any callable props are evaluated. InertiaRails.merge() is a callable. From the perspective of the filtering algorithm, there are zero users with a "name" property in this code: render inertia: 'SomePage', props: {
users: -> { User.where(some_conditions) }
} because the filtering happens before Maybe you could use a query parameter to render the user differently within a merge prop (I haven't tested this): render inertia: 'SomePage', props: {
current_user: InertiaRails.deep_merge{
params[:give_me_the_slim_user_please] ? current_user.as_json(only: [:first_name, :last_name]) : current_user
}
} then in the JS router.get(currentPagePath, { give_me_the_slim_user_please: 1 }) Basically, grabbing a subset of an evaluated prop and then merging it back into the Page isn't a workflow that partial reloading is capable of doing right now... so some creativity is required. I will say that we're talking about this and would like to support complex fetching/merging more cleanly. But nothing ready as of today. |
Resolves #157
I also refactored the rendering logic a bit, to:
Note: the renderer diff isn't very helpful; better to view the entire file to see how it looks now.
TODO: