Request formats addition breaks variants lookup

[sfnelson]

Steps to reproduce

1. Define a view component with a default example_component.html.erb template and an example_component.html+custom.erb template
2. Render the component from a turbo stream request, setting the variant to custom.

Expected behavior

The custom variant should be rendered.

Actual behavior

The default variant is rendered

System configuration

Rails version:
7.2.1

Ruby version:
3.3.5

Gem version:
3.17.0

Context

Prior to 3.15 adding request formats, this worked as expected because the request type was not considered. With 3.15 the addition of request formats it's now impossible to fall back to a default response type (i.e. html when the response is a turbo stream) without losing the variant. This is a problem because it means variants cannot be used for both html and turbo streams after this change.

This is the generated render_template_for method:

if (format == :html || format.nil?) && variant&.to_sym == :custom
  _call_custom_example_component
elsif (format == :html || format.nil?) && variant.nil?
  _call_example_component
else
  _call_example_component
end

When render_template_for is called, the format is :turbo_stream and the variant is :custom, but because the format doesn't match the variant is never considered. I'm not sure what the intended behaviour of this code is, but it seems at odds with the way that Rails view templates behave (i.e. choosing the appropriate response type based on the accepts header).

Example controller method:

def create
  ...
  if record.save
    redirect_to record, status: :see_other
  else
    render :new, status: :unprocessable_entity, variants: [:custom]
  end
end

Example view partial new.turbo_stream.erb:

<%= turbo_stream.replace("dom_id") do %>
  <%= render ExampleComponent.new %>
<% end %>

[sfnelson]

I don't understand why this feature looks at the request rather than the response. For example, here's the lookup context for this situation:

{:locale=>[:en], :formats=>[:turbo_stream, :html], :variants=>[:custom], :handlers=>[:raw, :erb, :html, :builder, :ruby]}

Wouldn't it make more sense to look at the formats defined in the lookup_context instead of looking at the request? The request format might be different to the accepts formats.

[leonvogt]

I've encountered the same issue. Upgrading to version 3.15 or higher breaks our Hotwire Native mobile apps.
Since the variant :mobile_app seems to be ignored, when rendering a component inside a Turbo Stream.

[joelhawksley]

Wouldn't it make more sense to look at the formats defined in the lookup_context instead of looking at the request? The request format might be different to the accepts formats.

@sfnelson That generally sounds right to me, would you like to take a crack at writing a fix? 2128-variant has a failing test 😄

While we've done the best we can to try to test all Rails rendering paths, I continue to be amazed at all of the cases we have to handle to be compatible 😂

[sfnelson]

@joelhawksley I've had a look at how Rails handles this logic, and the key code path to understand is ActionView::Template::Resolver#_find_all.

My superficial understanding of this code path is that the view context is used to construct an instance of ActionView::TemplateDetails::Requested which includes information about the constraints on locale, handlers, formats, and variants, as well as priority orderings for each. ActionView uses this object to sort the available templates that could possibly satisfy the request, and returns a priority ordered list of templates as the result.

Given your understanding of the goals of the ViewComponents project, would it be better to try and re-use ActionView's logic or re-implement something simpler and give up some compatibility?

I think that aiming for compatibility with the full complexity of ActionView would result in a fairly significant change to the approach currently used by ViewComponent (render_template_for).

[sfnelson]

To give you an idea of what this could look like, side-stepping compilation and short circuit optimisations, here's an implementation that uses Rails' ordering logic:

templates = @templates

@component.silence_redefinition_of_method(:render_template_for)
@component.define_method(:render_template_for) do |*|
  requested_details = ActionView::TemplateDetails::Requested.new(**self.lookup_context.send(:detail_args_for, {}).first)

  filtered_templates = templates.select do |template|
    details = ActionView::Resolver::PathParser.new.parse(template.instance_variable_get(:@path)).details
    details.matches?(requested_details)
  end

  if filtered_templates.count > 1
    filtered_templates.sort_by! do |template|
      details = ActionView::Resolver::PathParser.new.parse(template.instance_variable_get(:@path)).details
      details.sort_key_for(requested_details)
    end
  end

  send(filtered_templates.first.safe_method_name)
end

Is this strategy worth pursuing further?

A key difference between this strategy and the current ViewComponent logic is that the ordering can vary based on the request – i.e. the priority ordering of templates can be different based on the lookup context. Because of this, I don't think there would be any benefit from pre-compiling the switch logic.

[joelhawksley]

@sfnelson wow! Thank you for digging into the Rails side to see how it compares to the ViewComponent implementation.

My two main concerns with going this direction are 1) using private APIs and 2) performance.

Regarding #1, I'd want to work with Rails Core to make that API public before we can use it.
For #2 (and perhaps we'd do this first), I'd want to investigate the performance implications of going this direction.

In general, if we can address those two concerns, I'm amenable to heading in this direction to better align with Rails. I can imagine there are many more edge case bugs waiting to be found in this space due to our current implementation being completely different from Rails'.

[sfnelson]

@joelhawksley that all makes sense. Do you have a benchmark suite for testing performance changes? I've got a background in compilers, I'd be happy to assist further.

Re (2) my instincts are that there isn't a significant difference between the old and new changes for the majority of users as the priority ordering is only relevant to users who are using multiple templates for the same component. Given that these issues haven't come up before I suspect this is a fringe group of users, but hard to tell without a better understanding of how the community uses the gem, which I suspect would be hard to quantify.

Re (1) I also had reservations, but this was the most pragmatic solution to get some code in front of you. I doubt that this is a feature that the Rails core team would be interested in exposing as it really does seem like implementation detail. There are other options however, just nothing that was appropriate for a spike like this:

1. Make ViewComponents more Rails-like so we can use the public interfaces. Rails entry point is ActionView::Resolver.find, which is responsibly for both identifying and loading templates. I suspect that we could provide a ViewComponents::Resolver implementation that follows those conventions, which would allow access to the implementation detail without reflection.
2. Re-implement the Rails logic in ViewComponents. The logic isn't complicated, it would be easy to port. However I think the major benefit of using the Rails logic is that ViewComponents would behave consistently with Rails.

There are several benefits to ViewComponents to adopting the Rails approach:

1. Support for locale-specific templates https://guides.rubyonrails.org/i18n.html#localized-views
2. Support for multiple variants (currently only the first variant is supported)
3. Support for different templates that respect the Accepts header
4. Support for different templates based on the controller renders_with format

I think this could be a great feature for v4.

[joelhawksley]

@sfnelson thank you for your interest in helping out here! ❤ This is a great time to be looking into something that might be a breaking change as we set the path for v4: #2085.

Yes, we do have a benchmark suite in the repo: https://github.com/ViewComponent/view_component/blob/main/Rakefile

You're right to point out that these cases are very fringe. ViewComponent has been widely adopted since 2019 and it's telling that we've not seen this issue arise for very many folks when we made the request format change.

I could see the Rails core team going either way. They are keen to enable us to build our framework, and I'm guessing the resolver logic is pretty stable.

I think the path to staying in sync with Rails would be best. Perhaps we verify that using Resolver will not regress on performance, then look into making it a public API?

Otherwise, I think we port the logic.

FWIW, we do already have locale-specific templates.

[sfnelson]

@joelhawksley that sounds like a good approach to me as an approach. I've run the benchmarks and the performance seems comparable – there's more variation from run to run that there is from this change, any effect is lost in the noise.

Having read more of the source, LookupContext seems to be essentially a thin behaviour wrapper around @details, which is the data we actually want. In general it makes sense to have an abstraction, and the Rails LookupContext wrapper adds key features like cache key generation and template caching. The ViewComponents use case is very tightly coupled to the desire to move view templates to the app/components directory and support features like sidecar. If it weren't for these needs, I think the recommendation from Rails would be to create our own LookupContext, but I think that would be a non-starter due to how much overhead would be added – each component would need to create a new LookupContext per request. There might be some middle ground that I haven't thought of, but I think it's unlikely to be palatable to the Rails team to expose the internals of LookupContext. You can ask, of course :-)

I've had some time today to look at code approaches and I think I can prepare a PR without requiring immediate input from the Rails team by using Ruby refinements. This allows ViewComponents to access the internal @details from LookupContext without reflection and without exposing the implementation details or monkey patching. I think it's quite elegant, and given the @details var is a memoization of the LookupContext request parsing, I think it's very likely stable. Using this approach will remove some of the 'weird' edge cases using ViewComponents and help maintain the principle of least surprise for users experienced with more edge-case Rails feature.

I've found a few problems in this code exploration that I think are worth documenting separately. I'll create separate issues and link to this one.

[sfnelson]

See discussion about dropping support for . in variant names here: #2155

Rails does not support variant names containing ., the regex is literally [^.]*