ViewComponent / view_component

A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
https://viewcomponent.org
MIT License
3.28k stars 429 forks source link

Request formats addition breaks variants lookup #2128

Open sfnelson opened 1 week ago

sfnelson commented 1 week ago

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 commented 1 week ago

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 commented 3 days ago

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 commented 3 days ago

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 😂