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
Mention Strict Locals in more documentation #50864
Mention Strict Locals in more documentation #50864
Conversation
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.
TIL about strict locals! 👍 for better docs!
189548b
to
c8fb226
Compare
guides/source/7_1_release_notes.md
Outdated
All templating engines that support comments are supported: | ||
|
||
```ruby | ||
# locals: (json:, message:) | ||
json.message message | ||
``` |
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.
While this is true in the sense that Action View will parse out a # locals: (...)
magic comment, engines like jbuilder
merge additional values into the locals:
Hash under keys like :json
, :collection
, or :cached
).
This behavior is exhibited in the test failures included in rails/jbuilder#557.
Instances of ActionView::Template
can hint to the renderer a set of implicit keys that the templating engine is merging into the :locals
Hash (for example, collection counters and the current iteration index) with its :implicit_locals
keyword argument.
Unfortunately, templating engines like jbuilder
are not integrating with Action View rendering at that level.
I see two paths forward for engines like jbuilder
:
- change their implementation to instead rely on template-scoped helper methods to provide access to baked-in references like the
json
object
1a. alternatively, change their implementation to directly access an instance ofActionView::Template
(maybe through the@current_template
instance variable set in theActionView::Base
instance), then render withActionView::Template#render
instead ofActionView::Base#render
) - change Action View's implementation to support passing in
:implicit_locals
from higher layers
I'm not sure of the labor involved in either approach, but it's probably worth considering, since Strict Locals has utility outside of ERB.
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.
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.
@casperisfine do you have any insight here?
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.
No particular insights and I didn't have much time to dig to have a strong opinion. From high level I think 2 is probably the better solution (at least the lest disruptive one).
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.
@casperisfine for now, I've softened the language from All templating engines that support comments are supported
to Action View will process the locals: magic comment in any templating engine that supports
#-prefixed comments
.
For now, that represents the truth, and we can investigate a public interface for templating engines separate from improving the documentation.
c8fb226
to
9fd2122
Compare
Does it make sense to explain what happens if the arguments are not passed? It looks like it raises a useful error message at runtime but that's probably worth stating. |
9fd2122
to
4d13341
Compare
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.
Documentation isn't my forte, but this generally LGTM. I made a small suggestion, so feel free to apply or disagree, then let me know and I'll merge.
87bf3e1
to
db37c1e
Compare
829451c
to
97ab470
Compare
Motivation / Background --- Strict Locals support was introduced in [rails#45727][] and announced as part of the [7.1 Release][]. There are several mentions across the Guides, but support is rarely mentioned in the API documentation. Detail ---- Mention the template short identifier (the pathname, in most cases) as part of the `ArgumentError` message. This commit adds two test cases to ensure support for splatting additional arguments, and for forbidding block and positional arguments. It also makes mention of strict locals in more places, and links to the guides. [rails#45727]: rails#45727 [7.1 Release]: https://edgeguides.rubyonrails.org/7_1_release_notes.html#allow-templates-to-set-strict-locals
97ab470
to
6e1c2f7
Compare
I've also opened #50865 to scaffold out new view partials with built-in |
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 to me!
Motivation / Background
Strict Locals support was introduced in #45727 and announced as part of the 7.1 Release. There are several mentions across the Guides, but support is rarely mentioned in the API documentation.
Detail
This commit adds two test cases to ensure support for splatting additional arguments, and for forbidding block and positional arguments.
It also makes mention of strict locals in more places, and links to the guides.