Skip to content

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

Merged
merged 9 commits into from
Nov 21, 2024
68 changes: 47 additions & 21 deletions lib/inertia_rails/renderer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,25 +74,21 @@ def merge_props(shared_data, props)
end

def computed_props
_props = merge_props(shared_data, props).select do |key, prop|
if rendering_partial_component?
partial_keys.none? || key.in?(partial_keys) || prop.is_a?(AlwaysProp)
else
!prop.is_a?(LazyProp)
end
end
_props = merge_props(shared_data, props)

drop_partial_except_keys(_props) if rendering_partial_component?
deep_transform_props _props do |prop, path|
next [:dont_keep] unless keep_prop?(prop, path)
Copy link
Collaborator Author

@bknoles bknoles Nov 14, 2024

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 )

Copy link
Collaborator

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?

Copy link
Collaborator Author

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


deep_transform_values _props do |prop|
case prop
transformed_prop = case prop
when BaseProp
prop.call(controller)
when Proc
controller.instance_exec(&prop)
else
prop
end

[:keep, transformed_prop]
end
end

Expand All @@ -105,19 +101,19 @@ def page
}
end

def deep_transform_values(hash, &block)
return block.call(hash) unless hash.is_a? Hash

hash.transform_values {|value| deep_transform_values(value, &block)}
end
def deep_transform_props(props, parent_path = '', &block)
Copy link
Collaborator Author

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.

props.reduce({}) do |transformed_props, (key, prop)|
current_path = [parent_path, key].reject(&:empty?).join('.')
Copy link
Contributor

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:

Suggested change
current_path = [parent_path, key].reject(&:empty?).join('.')
current_path = path + [key]


def drop_partial_except_keys(hash)
partial_except_keys.each do |key|
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)
Copy link
Contributor

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:

Suggested change
if prop.is_a?(Hash)
if prop.is_a?(Hash) && prop.any?

nested = deep_transform_props(prop, current_path, &block)
transformed_props.merge!(key => nested) unless nested.empty?
else
action, transformed_prop = block.call(prop, current_path)
transformed_props.merge!(key => transformed_prop) if action == :keep
end

current.delete(last_key) if current.is_a?(Hash) && !current[last_key].is_a?(AlwaysProp)
transformed_props
end
end

Expand All @@ -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)
Copy link
Collaborator Author

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)
Copy link
Collaborator Author

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.

return false if excluded_by_only_partial_keys?(path_with_prefixes)
return false if excluded_by_except_partial_keys?(path_with_prefixes)
end

# Precedence: Evaluate LazyProp only after partial keys have been checked
return false if prop.is_a?(LazyProp) && !rendering_partial_component?

true
end

def path_prefixes(path)
parts = path.split('.')
(0...parts.length).map do |i|
parts[0..i].join('.')
end
end

def excluded_by_only_partial_keys?(path_with_prefixes)
partial_keys.present? && (path_with_prefixes & partial_keys.map(&:to_s)).empty?
end

def excluded_by_except_partial_keys?(path_with_prefixes)
partial_except_keys.present? && (path_with_prefixes & partial_except_keys.map(&:to_s)).any?
Copy link
Contributor

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

Suggested change
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?

end
end
end
24 changes: 24 additions & 0 deletions spec/dummy/app/controllers/inertia_render_test_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,30 @@ def except_props
}
end

def deeply_nested_props
render inertia: 'TestComponent', props: {
flat: 'flat param',
lazy: InertiaRails.lazy('lazy param'),
nested_lazy: InertiaRails.lazy do
{
first: 'first nested lazy param',
}
end,
nested: {
first: 'first nested param',
second: 'second nested param',
deeply_nested: {
first: 'first deeply nested param',
second: false,
what_about_nil: nil,
deeply_nested_always: InertiaRails.always { 'deeply nested always prop' },
deeply_nested_lazy: InertiaRails.lazy { 'deeply nested lazy prop' }
}
},
always: InertiaRails.always { 'always prop' }
}
end

def view_data
render inertia: 'TestComponent', view_data: {
name: 'Brian',
Expand Down
1 change: 1 addition & 0 deletions spec/dummy/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
get 'always_props' => 'inertia_render_test#always_props'
get 'except_props' => 'inertia_render_test#except_props'
get 'non_inertiafied' => 'inertia_test#non_inertiafied'
get 'deeply_nested_props' => 'inertia_render_test#deeply_nested_props'

get 'instance_props_test' => 'inertia_rails_mimic#instance_props_test'
get 'default_render_test' => 'inertia_rails_mimic#default_render_test'
Expand Down
26 changes: 26 additions & 0 deletions spec/inertia/rendering_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,32 @@
is_expected.to include('Brandon')
end
end

context 'with dot notation' do
let(:headers) do
{
'X-Inertia' => true,
'X-Inertia-Partial-Data' => 'nested.first,nested.deeply_nested.second,nested.deeply_nested.what_about_nil',
'X-Inertia-Partial-Component' => 'TestComponent',
}
end

before { get deeply_nested_props_path, headers: headers }

it 'only renders the dot notated props' do
expect(response.parsed_body['props']).to eq(
'always' => 'always prop',
'nested' => {
'first' => 'first nested param',
'deeply_nested' => {
'second' => false,
'what_about_nil' => nil,
'deeply_nested_always' => 'deeply nested always prop',
},
},
)
end
end
end

context 'partial except rendering' do
Expand Down