fix: Preserve locals when rendering inline partial for object#28
fix: Preserve locals when rendering inline partial for object#28moberegger merged 3 commits intoproductionfrom
Conversation
|
Ugh. I had the test use strict locals, which aren't supported by older versions of Rails. I'll figure out another way to do it... |
|
Filed upstream: rails#613 |
There was a problem hiding this comment.
Pull request overview
Fixes an inconsistency in Jbuilder’s inline partial rendering so that additional locals passed when rendering a single object under a key are preserved (matching the existing collection behavior), and adds documentation/tests for these supported render forms.
Changes:
- Adjust inline-partial rendering internals to defer locals construction to
_render_partial_with_options, preserving passed locals for single-object inline partials. - Expand test coverage for inline partial rendering under a key (single object + locals, collection + locals).
- Update README examples for inline partial rendering and correct minor documentation typos.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
lib/jbuilder/jbuilder_template.rb |
Changes inline partial object path to avoid pre-setting options[:locals], preserving locals passed via keyword args. |
test/jbuilder_template_test.rb |
Adds regression tests for inline partial rendering under a key (single + locals, collection + locals). |
test/test_helper.rb |
Updates the Post test struct to include :title used by new partial logic/tests. |
README.md |
Documents inline partial rendering for objects/collections under a key and passing additional locals; fixes small doc wording. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else | ||
| _scope do | ||
| options[:locals] = { options[:as] => object } | ||
| options[options[:as]] = object |
There was a problem hiding this comment.
Inline object partial rendering now sets the object on the top-level options hash (options[options[:as]] = object) so _render_partial_with_options can derive options[:locals]. However, if the caller explicitly provides locals: (e.g., json.post @post, partial: 'posts/post', as: :post, locals: { include_title: true }), _render_partial_with_options will keep the provided options[:locals] and will not include the :post local, so the partial will not receive the object local and can fail with an undefined post.
Consider handling this case by merging the object into options[:locals] when options[:locals] is already present (instead of only setting the top-level key), and add a regression test for inline object partial + locals:.
| options[options[:as]] = object | |
| options[options[:as]] = object | |
| options[:locals] = (options[:locals] || {}).merge(options[:as] => object) |
There was a problem hiding this comment.
Explicitly providing locals to inline renders is not documented, so I don' think this needs to be supported.
Prior to this PR, it wasn't possible to "mix" these key args, either. So something like
# Note - the use of `locals:` here for an inline render is not documented. It is only documented for `json.partial!`
json.foos, @foos, partial: 'foos/foo', as: :foo, my_local: :foo, locals: { my_other_local: :bar }wouldn't work. It has to be one or the other.
Ran into an odd bug.
Given a template like
Rendering to a collection inline like
Would correctly render with the value of
my_localwith the value of'custom'{ "foos": [ "id": 1, "name": "Test", "myLocal": "custom" ] }But rendering a single object wasn't picking up the value for the local
{ "foo": { "id": 1, "name": "Test", "myLocal": "default" } }The reason is because this render path in
_set_inline_partialwould set a key forlocalsbefore the render is done in_render_partial_with_options._render_partial_with_optionsattempts to pluck key args forlocals, but since the key is already there, they are effectively ignored.The fix was to not have
_set_inline_partialsetlocals, and instead defer that logic to_render_partial_with_options.I added some extra tests cases to cover this.
I also updated the docs to include an example for rendering an object to an inline partial. This is supported by the library, but was seemingly undocumented. Also documented rendering to partials inline with other locals, which was also supported by the library but seemingly undocumented.