Skip to content
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

Merged
merged 1 commit into from Jan 25, 2024

Conversation

seanpdoyle
Copy link
Contributor

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.

Copy link

@JoelQ JoelQ left a 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!

@seanpdoyle seanpdoyle force-pushed the strict-loading-documentation branch 2 times, most recently from 189548b to c8fb226 Compare January 24, 2024 15:36
Comment on lines 334 to 339
All templating engines that support comments are supported:

```ruby
# locals: (json:, message:)
json.message message
```
Copy link
Contributor Author

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:

  1. 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 of ActionView::Template (maybe through the @current_template instance variable set in the ActionView::Base instance), then render with ActionView::Template#render instead of ActionView::Base#render)
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pinging @byroot since you're the author of #49782, which resolved issues very similar to this one.

Copy link
Contributor Author

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?

Copy link
Contributor

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).

Copy link
Contributor Author

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.

@mike-burns
Copy link

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.

Copy link
Contributor

@casperisfine casperisfine left a 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.

actionview/lib/action_view/base.rb Outdated Show resolved Hide resolved
actionview/lib/action_view/template.rb Outdated Show resolved Hide resolved
guides/source/action_view_overview.md Outdated Show resolved Hide resolved
guides/source/action_view_overview.md Outdated Show resolved Hide resolved
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
@seanpdoyle
Copy link
Contributor Author

I've also opened #50865 to scaffold out new view partials with built-in # locals: integration.

Copy link
Member

@p8 p8 left a 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!

@rafaelfranca rafaelfranca merged commit c6bd547 into rails:main Jan 25, 2024
4 checks passed
@seanpdoyle seanpdoyle deleted the strict-loading-documentation branch January 25, 2024 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants