Pass render options and block to calls to #render_in

[seanpdoyle]

Motivation / Background

Closes #45432

Support for objects that respond to #render_in was introduced in #36388 and #37919. Those implementations assume that the instance will all the context it needs to render itself. That assumption doesn't account for call-site arguments like locals: { ... } or a block.

Detail

This commit expands support for rendering with a :renderable option to incorporate locals and blocks. For example:

class Greeting
  def render_in(view_context, **options, &block)
    if block
      view_context.render html: block.call
    else
      view_context.render inline: <<~ERB.strip, **options
        Hello, <%= name %>
      ERB
    end
  end
end

render(Greeting.new)                    # => "Hello, World"
render(Greeting.new, name: "Local")     # => "Hello, Local"
render(Greeting.new) { "Hello, Block" } # => "Hello, Block"

Since existing tools depend on the #render_in(view_context) signature
without options, this commit deprecates that signature in favor of one
that accepts options and a block.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[seanpdoyle]

The support for blocks was somewhat incidental here. My main goal was to support passing :locals provided at the call sites.

@joelhawksley (and @BlakeWilliams, since you've authored #45432) does this change support behavior that's desirable to ViewComponents?

[BlakeWilliams]

Thanks for adding block support! I lost that one in my TODO's a while back since I've been pretty busy lately.

I'm not positive the locals changes as-is would help us much since we expect all data to be passed in to components via the constructor, but also with the new API it's no longer clear to me when to pass something as a local vs constructor argument. e.g.

render(Greeting.new(name: "Fox Mulder"), name: "Dana Scully"))

I think the API changes make that valid code, but it's not obvious to me which argument would take precedence or how Greeting should respond to locals from a ViewComponent perspective (we use render_in as a hook into ActionView, but otherwise components are largely just Ruby including the constructor). I don't think this would be limited to ViewComponents, but all classes that implement render_in would have to reconcile constructor args with locals and provide guidance on when to use each.

Coming at this with a relatively heavy ViewComponent bias, I could see an API that delegates to the constructor making sense, maybe like:

render(Greeting.new(name: "local"))
render(Greeting, name: "local") # Equivalent to the above since Greeting has a `render_in` instance_method. `name:` is passed to the constructor

All of that has a heavy ViewComponent bias of course, but I'm curious if you had any particular use-cases for the locals changes in mind, ViewComponent or otherwise?

[seanpdoyle]

All of that has a heavy ViewComponent bias of course

I can relate to that! Since ViewComponent was the pilot tool for driving out render_in, that makes a lot of sense. In my understanding of where ViewComponent fits in, its constructor state is its render context. That's also reasonable, since it's aiming to serve as a View Model. In my mind, ViewComponent has an opportunity to take a stance on requiring constructor-time state and rejecting render-time locals.

I could see an API that delegates to the constructor making sense

With access to the options, that'd be possible as a class-method:

class ViewComponent::Base
  def self.render_in(view_context, locals: {}, **options, &block)
    view_context.render renderable: new(locals, &block), **options 
  end

  def render_in(view_context, **, &)
    # render without :locals or &block
  end
end

class Greeting < ViewComponent::Base
  # ...
end

render Greeting, name: "local" do 
  "from a block"
end

# the abbreviated form above is equivalent to the long-form below
render renderable: Greeting, locals: { name: "local" } do 
  "from a block"
end

Since render_in is meant to be flexible enough to hand-off all rendering responsibilities to the object, it has an opportunity to make as much information available at render-time. ViewComponent made the design decision to exclude the local_assigns in favor of attr_{accessor,reader}, and instance variables. I don't think imposing that design decision on other integrators is necessary. A separate integrator might envision a View Model as a different balance between View and Model. Instead of accepting :name as a :locals value, it might expect that to be available from the object whereas a :size or :variant might be expected as a :locals value.

I'm curious if you had any particular use-cases for the locals changes in mind, ViewComponent or otherwise?

At the moment, the main motivating use case is internal to Rails. I'm in the process of exploring an Action Text branch that adds support for editors other than Trix.

Incidentally, one part of the code I'm exploring re-structuring involves the ActionText::ContentHelper helper module, namely the render_action_text_attachment method:

rails/actiontext/app/helpers/action_text/content_helper.rb

Lines 43 to 55 in b294a25

	def render_action_text_attachment(attachment, locals: {}) # :nodoc:
	options = { locals: locals, object: attachment, partial: attachment }
	if attachment.respond_to?(:to_attachable_partial_path)
	options[:partial] = attachment.to_attachable_partial_path
	end
	if attachment.respond_to?(:model_name)
	options[:as] = attachment.model_name.element
	end
	render(**options).chomp
	end

It's a :nodoc: method, and is only called within the module itself:

rails/actiontext/app/helpers/action_text/content_helper.rb

Line 30 in b294a25

node.inner_html = render_action_text_attachment attachment, locals: { in_gallery: false }

rails/actiontext/app/helpers/action_text/content_helper.rb

Line 36 in b294a25

attachment.node.inner_html = render_action_text_attachment attachment, locals: { in_gallery: true }

The implementation is fairly concerned with checking whether or not the attachment responds to various methods, all for the sake of building up the correct Hash or options to pass to render. One extra layer on top of that is that between the two call sites, the only difference is the :in_gallery local variable (true in one place, false in the other).

I'm imagining a world where each editor adapter (ckeditor5, tinymce, etc) have an opportunity to subclass ActionText::Attachment and want to override the default behavior. They wouldn't be able (or advised to) reach into the module-private :nodoc: method. However, if render_in accepted options, they'd be available to the subclasses at call time.

class ActionText::Attachment
  def to_attachable_partial_path
    # to be overridden
    to_partial_path
  end

  def render_in(view_context, **options)
    view_context.render options.with_defaults(
      partial: to_attachable_partial_path,
      object: self,
      as: model_name.element
    )
  end
end

That's a circumstance where the constructor arguments to the object and the values passed into the :locals wouldn't be similar enough to cause confusion. In this use case, it's more about the context with which they're rendered. The ActionText::Attachment objects are constructed in far-flung parts of Action Text very separate from the HTML rendering render_in calls. It would also provide the framework with an opportunity to maintain the consistency in the construction of the ActionText::Attachment instances while providing integrators with flexibility at render-time.

Outside of that use case, I'm imagining other scenarios where an application might want to build renderable objects (maybe even ViewComponents) prior to their render. I don't have a concrete example handy, but I'm imagining a component that yields itself into another component without knowledge of how the receiving component would want to treat it.

[seanpdoyle]

@BlakeWilliams if this change is accepted, I'm also exploring adding a default #render_in implementation to the ActiveModel::Conversion module:

seanpdoyle/rails@action-view-render-in-options...seanpdoyle:rails:active-model-conversion-render-in#diff-8ec888bfb07b8d4a67ffa913e3286d4ffcb052d1bf3b232d2874c7177cd96fbc

The idea there would be that any Active Model or Active Record instance would provide a seam for applications to take over control of rendering (for example, mapping a Person to a PersonComponent class).

[rafaelfranca]

This is a breaking change for those objets. We probably need to deprecate the old signature and tell people to upgrade their objects

[joelhawksley]

@rafaelfranca I could also see Rails checking the arity?

Regardless, it'd be a huge bummer to have a breaking change like this. I'd really hope we could avoid going down that route.

[rafaelfranca]

Yeah, I was thinking on doing the arity check to show the deprecation.

Why would it be a bummer if we deprecated the old signature? The old code would work, showing a deprecation, and if the library changed to the new one, as the arguments are optional, things would just work in old versions of Rails. In fact you can release a new version of a library right now with the new signature.

[rafaelfranca]

Why are we trying to call the format now?

[seanpdoyle]

@rafaelfranca this is related to #50622.

When #render_in was introduced, there wasn't any public documentation citing that in addition to #render_in, objects would also need to define #format.

The TestRenderable in the test suite defines it (and, for example, I think ViewComponent::Base also defines it). It's called internal to Action View, so it's part of the public API despite the fact that it wasn't part of any of the published documentation.

https://github.com/rails/rails/pull/50622/files#diff-70344644d049724d12c9a0c2c387dd1ea31663fbfad4c6a1acdd49efc3cb9512R20 is actually the source of the Object#try change. The motivation behind that change was that since it was newly documented, prior implementers of #render_in could get by without defining #format. The absence of the method means that TemplateRenderable#format returns nil, and somewhere down the chain the content gets treated as :html by default.

Since this change makes more of the options available to the #render_in call, it could be useful for the caller to decide how to render itself (as HTML, JSON, etc) based on that newly available information.

If the Object#try doesn't feel correct, would we be able to change this interface to accept the :formats available from the render call as arguments?

[seanpdoyle]

@rafaelfranca based on this discussion, I've made two changes:

• omit support for render formats: ... from this PR
• deprecation warning for @renderable objects that don't respond to #format

[rafaelfranca]

deprecation warning for @Renderable objects that don't respond to #format
Why we need that?

[seanpdoyle]

@rafaelfranca @joelhawksley I've added a deprecation here along with test coverage. I struggled to convey a more detailed deprecation message. If you're able, please share any suggestions you have.

[seanpdoyle]

I think these examples could be improved, but I'm not entirely sure how.

In my mind, there's a few points worth highlighting:

1. #render_in and the :renderable key as an alternative to :partial is novel in its own right.
2. The object that defines #render_in can decide on its own how to mix and match its own internal state with the state provided at the render call site through the :locals options
3. The object that defines #render_in can also decide how to mix the block passed to render
4. The object can decide which format to render into

Prior to this commit, 1. is only point mentioned at all.

With the proposed changes, 2. poses an interesting opportunity. I consider it similar to React's split between State and Props. Maybe there's an Avatar object that manages its own internal state like the URL and alt text, but accepts overrides from :locals like :size:

class Avatar
  include ActiveModel::Model
  include ActiveModel::Attributes

  attribute :url, :string
  attribute :alt, :string
  attribute :size, :string, default: "medium"

  def render_in(view_context, **options)
    # ...
  end
end

avatar = Avatar.new(url: "https://example.com/avatar.jpg", alt: "A smiling person")

render avatar
# => <img src="https://example.com/avatar.jpg" alt="A smiling person" class="avatar avatar--medium">
render avatar, size: "small"
# => <img src="https://example.com/avatar.jpg" alt="A smiling person" class="avatar avatar--small">

Block-centric rendering objects like ViewComponent::Base could benefit from 3. by forwarding the render call's block to its own constructor. Prior to this commit, calls like this wouldn't behave like you'd expect (as highlighted by #45432):

# ignores the block
render MyComponent.new do
  "Content"
end

# ignores the block
render(MyComponent.new) do
  "Content"
end

# captures the block
render(MyComponent.new do
  "Content"
end)

Support for determining the templating format (4.) is potentially desirable for objects that want to support multiple formats (like HTML, JSON, etc.). Unfortunately, that support might require more broad changes, and need to be incorporated in a subsequent PR. At the moment, it isn't clear the precedent between:

• view_context.lookup_context.formats
• the :formats option passed to render
• the #format method defined by the same object as #render_in. At this point in time, the #format call would lack access to both the view_context and the render options

There's even potential for applications (or Rails itself) to extend models to know how to render themselves, if they want more control than what Action View provides out of the box:

class ApplicationRecord < ActiveRecord::Base
  # Renders the object into an Action View context.
  #
  #   # app/models/person.rb
  #   class Person < ApplicationRecord
  #   end
  #
  #   # app/views/people/_person.html.erb
  #   <p><%= person.name %></p>
  #
  #   person = Person.new name: "Ralph"
  #
  #   render(person)              # => "<p>Ralph</p>
  #   render(renderable: person)  # => "<p>Ralph</p>
  def render_in(view_context, **options, &block)
    view_context.render(partial: to_partial_path, object: self, **options, &block)
  end
end

class Person < ApplicationRecord
end

render Person.new(name: "Renders as a Partial")

class Article < ApplicationRecord
  def render_in(view_context, ...)
    view_context.render ArticleComponent.new(name:)
  end
end

render Article.new(name: "Renders as a ViewComponent")

[rafaelfranca]

Documentation in a bunch of places on this PR says format is optional, here we are telling it is required. Which is the right requirement? If we need format, why?

[seanpdoyle]

I agree with you, the documentation should be specific about the fact that #format must be implemented. I'll make those changes now.

I've added this deprecation because the requirement for #format was never mentioned prior to https://github.com/rails/rails/pull/50622/files.

Is requiring that #format be implemented considered a breaking change, since there might be apps that have #render_in objects that don't define #format?

[seanpdoyle]

On the other hand, the code does not current require that #format be implemented thanks to https://github.com/rails/rails/pull/50622/files#r1445897182.

We could continue to treat #format as optional, in which case the deprecation would not be necessary, and the code could remain @renderable.try(:format).

For what it's worth, implementers of #render_in could also define #format and return nil, and the current implementation would still work.

Is that preferable over a deprecation?

[rafaelfranca]

Before #50622, format was already required. That PR made it optional, so there is no change in behavior there that needs to be deprecated.

Now, if #50622 changed from @renderable.try(:format) to @renderable.format we would need a deprecation. The opposite direction doesn't need a deprecation.

[seanpdoyle]

@rafaelfranca thank you for clarifying.

Since #50622 made that optional, I will change the documentation to reinforce that it is optional, then remove the deprecation.

[rafaelfranca]

👍🏽

[jonathanhefner]

I don't think this should change. Presumably, we will remove the arity check when we remove the deprecated code, and go back to rescuing NoMethodError. If we forget to also change this test back, we will lose test coverage.

Conversely, raising NoMethodError in this test can exercise both rescue NameError and rescue NoMethodError.

[BlakeWilliams]

I can relate to that! Since ViewComponent was the pilot tool for driving out render_in, that makes a lot of sense. In my understanding of where ViewComponent fits in, its constructor state is its render context. That's also reasonable, since it's aiming to serve as a View Model. In my mind, ViewComponent has an opportunity to take a stance on requiring constructor-time state and rejecting render-time locals.

I wouldn't want us to take that stance since that creates a pretty awkward API for consumers since there's two supported paths but only one is correct. I chatted with the rest of the ViewComponent team and I think we're in agreement that this API change is fine, but it will require us to make changes in ViewComponent to properly support it (which I'm somewhat excited about).

If this change is approved we'll need to update the library to support both paths:

render(MyComponent, name: "Fox Mulder")
render(MyComponent.new(name: "Fox Mulder"))

I'm excited because the first example above could be used to enable component authors to omit an initializer, which is something I've been wanting to see for a while but couldn't quite fit it in the existing API in a reasonable way. The second example would likely take that stance of dropping locals if we continue supporting it (which we likely will but I'm tempted to propose we don't).

So tl;dr is that this change seems fine (if not good) from a ViewComponent perspective. 👍

[seanpdoyle]

@jonathanhefner since you helped me workshop the NameError handling below specific to NameError instances that stem from #render_in and only #render_in, I'm curious if a similar approach would be safe for ArgumentError handling:

Suggested change

	def render(context, locals)
	options =
	if @renderable.method(:render_in).arity == 1
	ActionView.deprecator.warn <<~WARN
	Action View support for #render_in without options is deprecated.
	Change #render_in to accept keyword arguments.
	WARN
	{}
	else
	{ locals: locals }
	end
	@renderable.render_in(context, **options, &@block)
	rescue NameError
	def render(context, locals)
	@renderable.render_in(context, locals: locals, &@block)
	rescue ArgumentError
	if @renderable.method(:render_in).arity == 1
	ActionView.deprecator.warn <<~WARN
	Action View support for #render_in without options is deprecated.
	Change #render_in to accept keyword arguments.
	WARN
	@renderable.render_in(context, &@block)
	else
	raise
	end
	rescue NameError

The motivation here is to optimize adherents to the interface to avoid slowing the happy path down with conditionals. Does that feel like a good trade-off?