Allow templates to define which locals they accept #45602

joelhawksley · 2022-07-14T20:23:34Z

Summary

This PR introduces an option that enables templates to have required arguments with default values.

This change will also unlock the ability to pre-compile templates during initialization in the future.

Problem statement

Rails templates have an implicit API, accepting any locals passed to them. This can make template dependencies difficult to reason about. In addition, the underlying ActionView code that enables flexible locals means that templates have to be compiled for each unique combination of locals passed in at runtime.

Proposed solution

In chatting with @tenderlove, @jhawthorn, and @byroot at RailsConf, we came up with the idea of allowing for optional template signatures using a magic comment. We would then compile these arguments as the method signature for the template.

Doing so would allow us to pre-compile templates at application boot time, instead of at runtime in the future. For more information on this issue, see:

https://www.johnhawthorn.com/2019/09/precompiling-rails-templates/
Aaron Patterson's RailsConf 2019 Keynote
John Hawthorn's "Parsing and Rewriting Ruby Templates" (slides)

Example

Before:

<%# issues/_card.html.erb %>
<% title = local_assigns[:title] || "Default title" %>
<% comment_count = local_assigns[:comment_count] || 0 %>
<h2><%= title %></h2>
<span class="comment-count"><%= comment_count %></span>

After:

<%# issues/_card.html.erb %>
<%# locals: (title: "Default title", comment_count: 0) %>
<h2><%= title %></h2>
<span class="comment-count"><%= comment_count %></span>

Alternatives considered

Compiling templates to one object per template, using object initializer for arguments

This is more or less what we've built with ViewComponent at GitHub. It's worked quite well for us, but wrapping a template in an object has its downsides when it comes to object allocations in heavy usage and we've had issues with form helpers.

Implicit signature compilation

We could, in theory, generate the signature for a template by traversing its AST or another form of static analysis. This has proven difficult in our experience and still does not allow one to set defaults for arguments or to require arguments.

jhawthorn · 2022-07-14T20:57:03Z

I like this idea, though I think we should shelve the precompilation discussion as I think that is just a nice (but significant!) side-effect. I think the reason we should do this is the improved developer experience from validating template arguments and having a convenient way to specify defaults.

I do really like this idea. The one reservation I can forsee is surrounding use of "significant", "magic" comments, but I think we already have a precedent for that for specifying encoding (<%# encoding: utf-8 -%>) and specifying possible child templates for cache digesting.

jhawthorn · 2022-07-14T20:59:51Z

actionview/CHANGELOG.md

+    This allows for defining what locals can be passed to a template. For example:
+
+    ```erb
+    # strict: (message:)


This reads a bit odd to me. Should we use <%# strict: (message:) -%> to match existing magic comments? (we can still search for just # strict: .* to match other template formats, but don't have to sub! it from the template)

joelhawksley · 2022-07-15T17:16:37Z

@jhawthorn thanks for having a look! I've scaled this back to not mention precompilation- we can handle that later.

In the meantime, I'd be curious to get your thoughts on the way strict: (true) behaves differently between Ruby versions. Is this something we should be concerned about? I could see us adding a check for this and perhaps a specific error class so that behavior is consistent across Ruby versions. What do you think?

jonathanhefner

Cool! 😃

jonathanhefner · 2022-07-15T18:48:15Z

actionview/lib/action_view/template.rb

+      return unless STRICT_REGEX.match?(source)
+
+      # Look for # strict: (*). If we find one, we'll mark
+      # the template as strict and set the arguments it
+      # should accept.
+      if source.sub!(STRICT_REGEX, "")
+        @arguments = $1
+
+        true
+      else
+        false
+      end


I think return unless and the else branch are redundant... no?

jonathanhefner · 2022-07-15T18:51:39Z

actionview/CHANGELOG.md

+    This allows for defining what locals can be passed to a template. For example:
+
+    ```erb
+    <%# strict: (message: "Hello, world!") -%>
+    <%= message %>
+    ```
+
+    Default values can also be provided:
+
+    ```erb
+    <%# strict: (message: "Hello, world!") -%>
+    <%= message %>
+    ```


Should the first example here omit "Hello, world!"?

jonathanhefner · 2022-07-15T19:00:12Z

guides/source/action_view_overview.md

+By default, templates will accept any `locals`. To define what locals a template should accept, use the `strict` magic comment:
+
+```erb
+<%# strict: (message:) -%>
+<%= message %>
+```
+
+Default values can also be provided:
+
+```erb
+<%# strict: (message: "Hello, world!") -%>
+<%= message %>
+```
+
+To disallow locals entirely, set `# strict: (true)`.


Floating an idea: what if the magic "comment" was 100% Ruby? e.g.:

<% template_signature { |foo:, bar: 2, baz: (foo + bar)| } %>

That might make the valid syntax of the "comment" more obvious, including when disallowing locals:

<% template_signature { || } %>

And it could leverage syntax highlighting in editors.

Huh that's pretty cool! I love the idea of having it not be a comment. Are you thinking we would we AST this line to extract the method signature for the template?

byroot · 2022-07-17T09:18:29Z

So I like the feature too, I'm not sure I'd call it "strict" though. What I had in mind when we discussed this was something like <%# def(post:, title: "Hello World") %>

As for supporting no arguments <%# strict: (true) %> is a bit weird to me. With def I guess it should look like <%# def() %>.

There is also the question of wether these arguments should be passed as locals: or another, more convenient way, but I guess this would help the transition as you could modify a template without having to modify the callers, so it probably make sense to keep it that way.

joelhawksley · 2022-07-19T20:49:07Z

There is also the question of wether these arguments should be passed as locals: or another, more convenient way, but I guess this would help the transition as you could modify a template without having to modify the callers, so it probably make sense to keep it that way.

Right, I really want to avoid modifying callers, at least at this point.

joelhawksley · 2022-07-19T22:16:05Z

@jhawthorn @byroot @jonathanhefner thanks for the thoughtful feedback! I really like the idea of writing this in valid Ruby vs. as a comment. I suggested an accepts_locals(message:) API to further align this feature conceptually with the existing locals mental model.

Let me know what you think ❤️

byroot · 2022-07-20T09:47:31Z

I must say I'm on the fence.

The pro I see if that since it's not a magic comment, editors will highlight it.

The con is that we're kinda lying to the user, as this line of code won't be executed, but striped during compilation.

I'm not strongly opposed though.

joelhawksley · 2022-07-20T14:53:54Z

@byroot I'm pretty indifferent as well. @jonathanhefner @jhawthorn how do you lean here?

jonathanhefner · 2022-07-20T16:45:21Z

actionview/CHANGELOG.md

+    For example:
+
+    ```erb
+    <% accepts_locals(message:) -%>


This would only be valid syntax in Ruby >= 3.1, right? (And such a method call would imply that message is already defined.)

My thinking was: by using a block, we link the declared locals syntax to valid params syntax. Linking declared locals syntax to valid argument syntax seems to be mostly equivalent (for Ruby >= 3.1), but that could change in the future.

I think if we don't want to use something with params syntax (because e.g. it doesn't look as clean), then a comment is probably better.

@jonathanhefner regardless of the syntax, this line is not executed in the proposed implementation. It's nice to have syntax highlighting, but based on your response I think a comment is a clearer API due to the lack of execution.

+1 for the comment. I think the Ruby method is confusing because though it's tecnically now valid syntax we aren't actually doing a method call (we are "defining" a method, though even that is a little abstracted to the user). Having this look like regular Ruby is likely to confuse users into thinking they could call the method dynamically.

joelhawksley · 2022-07-20T22:36:17Z

Alright, I switched this back to a magic comment ❤️

jonathanhefner · 2022-07-21T15:59:07Z

actionview/CHANGELOG.md

+    <%= message %>
+    ```
+
+    To disallow locals entirely, set `# accepts_locals()`.


In #45602 (comment), I was thinking a label like template_signature would make it clear that empty brackets denote no locals. But, to me, accepts_locals() reads as "this template accepts (some unspecified) locals".

One possibility would be to have different comments for each case. For example:

<%# no_locals %> <%# def_locals (message:) %> <%# def_locals message: %> perhaps without parens?

Another possibility is to rephrase by adding a colon:

<%# locals: false %> <%# locals: (message:) %>

That would mirror the syntax of other magic comments.

I guess what weirds me out a bit, is that most proposals so far use some variation of locals (which makes sense since it's the parameter you use to pass them), but to me this is more akin to declaring a method signature, hence why I'm more attracted by something like:

<%# def(message:, author: "George") %>

or

<%# sig(message:, author: "George") %>

But that's just my opinion, I don't feel that strongly about it.

jonathanhefner · 2022-07-21T15:59:33Z

actionview/lib/action_view/template.rb

+      return unless EXPLICIT_LOCALS_REGEX.match?(source)
+
+      source.sub!(EXPLICIT_LOCALS_REGEX, "")
+
+      @arguments = $1
+
+      true


It probably doesn't matter, but this evaluates the regexp twice. I think it could be written as:

Suggested change

return unless EXPLICIT_LOCALS_REGEX.match?(source)

source.sub!(EXPLICIT_LOCALS_REGEX, "")

@arguments = $1

true

if source.sub!(EXPLICIT_LOCALS_REGEX, "")

@arguments = $1

true

end

Note that if we change the regexp to something like /no_locals|def_locals (.+)/ or /locals: (?:false|$(.+)$)/ then $1 could be nil. But, based on the current code, I think that's okay.

jonathanhefner · 2022-07-21T16:04:33Z

guides/source/action_view_overview.md

@@ -318,6 +318,24 @@ You can also specify a second partial to be rendered between instances of the ma

 Rails will render the `_product_ruler` partial (with no data passed to it) between each pair of `_product` partials.

+#### Explicit locals


Suggested change

#### Explicit locals

#### Explicit Locals

actionview/lib/action_view/template.rb

byroot · 2022-08-01T21:30:21Z

Ok, all good to me for real this time. Just need you to squash into a single commit, and I'll merge on green.

Thank you for bearing with me on this one, I was a bit picky.

byroot · 2022-08-01T21:35:37Z

actionview/lib/action_view/template.rb

@@ -8,6 +8,8 @@ module ActionView
  class Template
    extend ActiveSupport::Autoload

+    EXPLICIT_LOCALS_REGEX = /\# locals: \((.*)\)/


I hate myself for only noticing it now, be we may need to rework that regexp a bit:

We should ensure it's the only thing on the line, so add ^ and $

We can probably allow extra whitespaces in a bunch of places, e.g \s+ instead of just .

I tested /^\s*\#\s+locals:\s+$(.*)$$/ on https://rubular.com/, and it seem to do the job

🤔 the existing magic comment regex does not have the exact line match: https://github.com/rails/rails/blob/main/actionview/lib/action_view.rb#L33. I'm guessing this is to allow it to work with non-ERB templating languages? For example, in haml they look like:

-# locals: (foo:)

(https://makandracards.com/makandra/689-know-your-haml-comments#section-ruby-comments)

I swapped in the \s+ 👍🏻

(and for that matter, in ERB we wouldn't be matching the entire line either:

<%# locals: (foo:) %>

Of course, I'm super rusty on regexes so forgive me if I'm missing something here ❤️

Ah right, we're paring the source template, not the generated code, that makes sense. My bad!

All good! I greatly appreciate the rigor of your reviews!

byroot · 2022-08-01T21:49:06Z

Thanks for your patience!

joelhawksley · 2022-08-01T21:59:47Z

@byroot thanks for the helping hand! FWIW I thought the pace on this PR was just fine. I'd rather take the time to get things buttoned up nicely.

joelhawksley · 2022-08-01T23:02:52Z

guides/source/action_view_overview.md

@@ -318,6 +318,28 @@ You can also specify a second partial to be rendered between instances of the ma

 Rails will render the `_product_ruler` partial (with no data passed to it) between each pair of `_product` partials.

+#### Explicit Locals


@byroot thinking about this some more, what do you think about calling this Strict Locals instead of Explicit Locals? Strict feels better to me.

seanpdoyle · 2022-08-01T23:16:35Z

I'm sorry to chime in so late to this discussion, but as an alternative to a magic comment, have we considered overloading local_assigns?

To preserve backwards compatibility, invoking local_assigns without arguments could return a Hash-like object that responds to #[], #keys, etc.

However, when invoked with arguments, it'd invoke the same code that the magic comment results in.

To get the point across, imagine a method like:

def local_assigns(*arguments, **options): arguments.empty? && options.empty? ? @hash : do_magic(*arguments, **options)

Then, partials could call:

<% local_assigns :title, body_with_default: "a default value" %>

<%# elsewhere ... %>

<%= local_assigns[:title] %>

That one big risk I see with a method instead of a comment is that the method name essentially becomes a reserved word, and risks colliding with anything passed into locals: {}. In this case, local_assigns is already reserved, so that isn't a risk.

Per rails#45602 (comment)

joelhawksley · 2022-08-01T23:28:31Z

@seanpdoyle you raise a good point. I'm not sure if you saw the buried thread, but we discussed something similar at one point. (I know that GitHub collapses discussions on long PRs like this): #45602 (comment)

I see your point and think that there are definitely compromises in both approaches (magic comment vs. interpreted ruby).

adamdebono · 2022-08-07T21:57:41Z

Does this work with other template languages, or is it just limited to ERB templates?

For example could I do this with Jbuilder:

# locals: (message:)
json.message message

It might be worth adding a line to the guide to indicate this (either way). Happy to do this myself.

byroot · 2022-08-08T07:38:14Z

It should yes, hence why the regexp for the magic comment is relatively lax. If it doesn't work please open an issue.

joelhawksley · 2022-08-08T21:32:08Z

@byroot @adamdebono I think the same issue would exist with the encoding magic comment that we modeled this feature after, FWIW.

collimarco · 2023-11-19T11:17:54Z

In a utility-first approach for styling (e.g. Tailwind), does it make sense to use this new feature instead of ViewComponents?

What are the pros / cons of this over ViewComponents?

The main advantage that I see with this solution is that you don't need an extra .rb file

byroot · 2023-11-19T15:04:45Z

It's orthogonal. You can use either or both together.

rails-bot bot added actionview docs labels Jul 14, 2022

jhawthorn reviewed Jul 14, 2022

View reviewed changes

joelhawksley force-pushed the strict-templates branch 4 times, most recently from e1d8ce9 to 9207b20 Compare July 15, 2022 17:14

joelhawksley requested a review from jhawthorn July 15, 2022 17:16

jonathanhefner reviewed Jul 15, 2022

View reviewed changes

joelhawksley changed the title ~~Add strict: option for templates enabling pre-compilation~~ Add strict: option for defining template arguments Jul 16, 2022

joelhawksley force-pushed the strict-templates branch 4 times, most recently from f5a2bf9 to 714326b Compare July 19, 2022 22:12

joelhawksley changed the title ~~Add strict: option for defining template arguments~~ Allow templates to define which locals they accept Jul 19, 2022

joelhawksley requested a review from jonathanhefner July 20, 2022 14:53

jonathanhefner reviewed Jul 20, 2022

View reviewed changes

joelhawksley force-pushed the strict-templates branch 3 times, most recently from 80e9343 to 9844ad8 Compare July 20, 2022 22:35

joelhawksley requested a review from jonathanhefner July 20, 2022 22:36

jonathanhefner reviewed Jul 21, 2022

View reviewed changes

joelhawksley requested a review from byroot August 1, 2022 21:00

joelhawksley force-pushed the strict-templates branch 2 times, most recently from b032005 to 428ea86 Compare August 1, 2022 21:17

byroot reviewed Aug 1, 2022

View reviewed changes

actionview/lib/action_view/template.rb Outdated Show resolved Hide resolved

joelhawksley force-pushed the strict-templates branch from 428ea86 to abae401 Compare August 1, 2022 21:28

joelhawksley force-pushed the strict-templates branch 2 times, most recently from ddf4f37 to a4f2749 Compare August 1, 2022 21:32

byroot reviewed Aug 1, 2022

View reviewed changes

joelhawksley force-pushed the strict-templates branch from a4f2749 to 5f5ce9a Compare August 1, 2022 21:38

Allow templates to define which locals they accept.

bbe7d19

joelhawksley force-pushed the strict-templates branch from 5f5ce9a to bbe7d19 Compare August 1, 2022 21:42

byroot merged commit f9f2644 into rails:main Aug 1, 2022

joelhawksley deleted the strict-templates branch August 1, 2022 21:49

joelhawksley commented Aug 1, 2022

View reviewed changes

joelhawksley added a commit to joelhawksley/rails that referenced this pull request Aug 1, 2022

Rename 'Explicit Locals to Strict Locals`

b7908a6

Per rails#45602 (comment)

joelhawksley mentioned this pull request Aug 1, 2022

Rename 'Explicit Locals to Strict Locals` #45727

Merged

fsateler mentioned this pull request Aug 12, 2022

Support multiple views ViewComponent/view_component#387

Open

akhilgkrishnan mentioned this pull request Sep 1, 2023

[skip ci] Description added for "Allow templates to set strict locals" in 7.1 release note #49107

Merged

4 tasks

HolyWalley mentioned this pull request Oct 25, 2023

Strict locals cause an error when implicitly rendering a collection #49761

Closed

andyw8 mentioned this pull request Jan 12, 2024

New linter suggestion: Enforce strict locals Shopify/erb-lint#338

Open

		@@ -318,6 +318,24 @@ You can also specify a second partial to be rendered between instances of the ma

		Rails will render the `_product_ruler` partial (with no data passed to it) between each pair of `_product` partials.

		#### Explicit locals

		@@ -318,6 +318,28 @@ You can also specify a second partial to be rendered between instances of the ma

		Rails will render the `_product_ruler` partial (with no data passed to it) between each pair of `_product` partials.

		#### Explicit Locals

Allow templates to define which locals they accept #45602

Allow templates to define which locals they accept #45602

Conversation

joelhawksley commented Jul 14, 2022 • edited

Summary

Problem statement

Proposed solution

Example

Alternatives considered

Compiling templates to one object per template, using object initializer for arguments

Implicit signature compilation

jhawthorn commented Jul 14, 2022 • edited

Choose a reason for hiding this comment

joelhawksley commented Jul 15, 2022

jonathanhefner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanhefner Jul 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented Jul 17, 2022

joelhawksley commented Jul 19, 2022

joelhawksley commented Jul 19, 2022

byroot commented Jul 20, 2022

joelhawksley commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelhawksley commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented Aug 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented Aug 1, 2022

joelhawksley commented Aug 1, 2022

joelhawksley Aug 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpdoyle commented Aug 1, 2022 • edited

joelhawksley commented Aug 1, 2022

adamdebono commented Aug 7, 2022

byroot commented Aug 8, 2022

joelhawksley commented Aug 8, 2022

collimarco commented Nov 19, 2023

byroot commented Nov 19, 2023

joelhawksley commented Jul 14, 2022 •

edited

jhawthorn commented Jul 14, 2022 •

edited

jonathanhefner Jul 15, 2022 •

edited

joelhawksley Aug 1, 2022 •

edited

seanpdoyle commented Aug 1, 2022 •

edited