Fix for shared scroll and deferred props #277
Conversation
… in renderer initializer
…to improve consistency and avoid variable collision
| attr_reader( | ||
| :component, | ||
| :configuration, | ||
| :controller, | ||
| :props, | ||
| :view_data, | ||
| :encrypt_history, | ||
| :clear_history | ||
| ) |
There was a problem hiding this comment.
These are public but they aren't referenced anywhere else in the codebase and I can't imagine that someone is calling them in the wild somewhere. Please someone sanity check me on that!
There was a problem hiding this comment.
I hope you're right, but this is a breaking change right? Could they be used in tests?
There was a problem hiding this comment.
This change is unrelated to the fix also, right?
There was a problem hiding this comment.
Yes, these are public but how would you even access this in a regular test spec... you'd have to be creating a InertiaRails::Renderer.new instance in your spec, right?
There was a problem hiding this comment.
The rspec helpers mock it:
inertia-rails/lib/inertia_rails/rspec.rb
Line 85 in 99774c8
There was a problem hiding this comment.
that's mocking the arguments to .new rather than the accessor methods
There was a problem hiding this comment.
Changed it to deprecations! Doesn't really hurt anything and better to not ruin some poor unsuspecting souls day in CI/CD land.
|
|
||
| deep_merge = options.fetch(:deep_merge, @configuration.deep_merge_shared_data) | ||
| passed_props = options.fetch(:props, component.is_a?(Hash) ? component : @controller.__send__(:inertia_view_assigns)) | ||
| @props = merge_props(shared_data, passed_props, deep_merge) |
There was a problem hiding this comment.
This line is the fix... merging props immediately so that downstream methods can reference @props and have the full set of props. This seems like the least surprising behavior.
| @props | ||
| .tap do |merged_props| # Always keep errors in the props |
There was a problem hiding this comment.
other part of the fix (just making sure we aren't unnecessarily calling merge_props twice)
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for scroll props and deferred props in shared data (via inertia_share). The key change is refactoring the Renderer initialization to merge shared data with passed props earlier in the lifecycle, enabling these special prop types to work correctly when defined in shared data blocks.
- Refactors prop merging to occur during initialization instead of later in the rendering process
- Adds test coverage for scroll props and deferred props defined via
inertia_share - Removes redundant
attr_readerdeclarations and standardizes instance variable access with@prefix
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/inertia_rails/renderer.rb | Refactors initialization to merge shared and passed props earlier; removes attr_readers and standardizes instance variable access |
| spec/inertia/rendering_spec.rb | Adds test coverage for shared scroll props and shared deferred props functionality |
| spec/dummy/config/routes.rb | Adds routes for new test controller actions |
| spec/dummy/app/controllers/inertia_render_test_controller.rb | Implements test controller actions using inertia_share for scroll and deferred props |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
….. juuuust in case
Found an issue with our newly released infinite scroll support! Scroll and deferred prop metadata doesn't render to the client if they are shared data. The reason is that they were being computed by iterating over
@props, which doesn't include shared data.I moved the merging of controller props and shared props into the initializer. We don't need them separately anywhere in the renderer and seems like Future Us might assume we can iterate over
@propssomeday.The renderer has become a bit of a maze as it has grown, and one thing that made it tricky to look through is that the attr_readers are used to reference instance variables... but only sometimes. And some methods use those method names as arguments. I think it's cleaner to just consistently reference the instance variables themselves.