Multiple templates per component, allowing arguments

[fsateler]

Summary

Fixes #387

This PR builds on the branch by @joelhawksley and @fermion, and adds the ability to specify arguments for the generated functions.

The logic being that one may want to use templates to extract sections, without wanting to build a full component. For example, I could have a Table component, and I would want to have the row definition in a sidecar template. This requires being able to pass in each item for every iteration:

class TableComponent < ViewComponent::Base
  def initialize(models)
    @models = models
  end
  template_arguments :row, :model
end

<%# table_component.html.erb %>
<table>
  <% @models.each do |model| %>
    <%= call_row model: model %>
  <% end %>
</table>

<%# row.html.erb %>
<tr>
  <%# I can use `model` here %>
  <td><%= model.name  %></td>
</tr>

Conceptually, this is very similar to ActionView's usage of locals:. However, this way has the following benefits:

1. Performance is increased as the template needs to be compiled only once.
2. We don't need to instance_exec things in a custom-build binding, further improving performance.
3. Ruby checks our argument list.
4. We force the user to specify the locals, thus forcing them to specify the interface.

Other Information

I'm submitting as a draft, because there are some important API issues that need discussing:

1. Should we always use keyword arguments? Maybe we should use positional arguments? (I happen to like keyword arguments, but it may not be everyone's cup of tea).
2. This doesn't allow default values for any parameter. Is this a problem that needs addressing?

[BlakeWilliams]

Thanks for the PR! We're starting to think about slots v2 and this is a use-case we should definitely take into account. Would you be willing to share your thoughts on this discussion #454?

[jonspalmer]

I'm with @BlakeWilliams that this feels like a use case that Slots could/should support.

If this doesn't get baked into slots I'd encourage us to think hard about the rendering API and making it "Rails like". I agree with the four points you mention - they seem consistent with ViewComponents strong APIs and cached templates. However, the call_row api feels awkward and not very Rails like. What I love about the ViewComponent implementation is that you call 'render' in a view just like you would for a partial etc. Can we make this similar. Could it be render template: :row or render_template :row, .... ?

[fsateler]

Hi!

I remember discussing this with @joelhawksley , and he also said that slots v2 could take this role. Maybe it can, but I have not yet seen how that works. Discussion #454 doesn't mention slot templates, for example. I have posted my further thoughts there.

I think we first need to address this bigger point before tackling the one raised by @jonspalmer (although I do agree that call_foo is very ugly).

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[fsateler]

Hi!

Friendly ping to ask about this feature. Do you consider Slots V2 to cover this use case? AFAICT, it could be, but it is rather cumbersome:

1. Using a Component Slot requires creating a new component + view, and requires creating a new object for each slot item (when it renders_many).
2. Using a Delegate Slot does not allow using ERB syntax.
3. A pass through slot is not applicable.

All three options also expose the slot to the user, which is not desired.

If splitting templates is still a desired feature, I'd like to move this forward, and also give it a nicer interface than call_<template_name>

[BlakeWilliams]

1. Using a Component Slot requires creating a new component + view, and requires creating a new object for each slot item (when it renders_many).
2. Using a Delegate Slot does not allow using ERB syntax.
3. A pass through slot is not applicable.

All three options also expose the slot to the user, which is not desired.

I agree 2 and 3 are mostly incompatible with this goal (2 could work imo, depending on the use case). I do think option 1 could work though. Your point about the slots being exposed to the user is completely valid, but I wonder if that could be resolved by adding a private: true option to slots if this becomes a proven/valuable use of slots?

[fsateler]

Hi @BlakeWilliams !

I'm going to create an example. Imagine a Table Component:

class TableComponent < ApplicationComponent
  renders_many :columns, "ColumnComponent"
  def initialize(rows:)
    @rows = rows
  end

  class ColumnComponent < ApplicationComponent # This one actually doesn't need to be a component :)
     def initialize(attribute:)
        @attribute = attribute
     end

    def render_value model
       model.send(@attribute) # for brevity, we don't implement other ways of rendering the value
    end
  end
end

<%# table_component.html.erb %>
<table>
  <% @rows.each do |row| %>
    <tr>
      <% columns.each do |col| %>
        <td><%= col.render_value(row) %></td>
      <% end %>
   </tr>
  <% end %>
</table>

Now, imagine that I add a bunch of options, and thus creating each row is a bit more complicated. Thus, I want to extract a method to render the row. I can use option 1 above:

class RowComponent 
  def initialize(item:, columns:, **some_more_options)
    @item = item
    @columns = columns
    @options = some_more_options
  end

  def row_options
     # something based on @options
  end
end

<%# row_component.html.erb %>
<%= content_tag :tr, row_options do %>
    <% @columns.each do |col| %>
      <td><%= col.render_value(@item) %></td>
    <% end %>
<% end %>

class TableComponent < ApplicationComponent
  renders_many :columns, "ColumnComponent"
  # @api private
  renders_many :rows, -> (item) do
    RowComponent.new(item: item, columns: columns, **some_options)
  end

  def initialize(rows:)
    @rows = rows
  end

  def before_render
    rows(@rows.map{|item| {item: item, columns: columns} })
  end
end

<%# table_component.html.erb %>
<table>
  <% rows.each do |row| %>
    <%= row %>
  <% end %>
</table>

The problem with this solution is that:

1. RowComponent is now public. This can be worked around with some documentation though.
2. It creates a new Hash and a new RowComponent for each Row, each in one. This is actually the deadly problem, since creating large arrays is very problematic for the ruby GC and can create large pauses. A simple example with 6000 rows means passing 10% of the time in GC (as measured by miniprofiler flamegraph) vs 0% in the first version.
3. Actually, I also find the solution to be a bit ugly. I need to remember to populate the rows slots in before_render, with a weird map in between.

[joelhawksley]

@fsateler are you still interested in building out this functionality? If so, perhaps opening a PR with an ADR would be a good next step?

[fsateler]

Hi @joelhawksley ! Indeed I might. I have two questions though:

1. What changed since the last comment? I'm happy to update and move this forward but the previous messages here suggesteds slots as a way for this problem. I'd like to know what changed that something like this would be acceptable for you.
2. What is an ADR? Any pointers on how to create one?

[fsateler]

Hi! I have rebased the branch and added the ADR.

[fsateler]

I'm thinking this check should also be the other way around: we should only compile methods for the templates we have described (either via the DSL or the def render_foo). WDYT?

[camertron]

Hey @fsateler, I'm a latecomer to this PR and discussion, but I hope you'll allow me to state my opinion anyway. I'm concerned rendering partials in components breaks the component abstraction. One of the benefits of view_component is that it avoids many of the problems inherent in traditional Rails views, key among them being the problem of shared state. While there's nothing stopping you from rendering normal Rails view partials inside components today, I'm not sure it's a good idea to bake it into the view_component framework like this. Doing so further encourages using partials that should probably be their own components. There's already a fair amount of confusion around how slots are used, and I fear partials with their own partial methods will add to this confusion.

Is there something preventing you from converting your partials into components?

[fsateler]

@camertron I think it is best if you comment directly on the parts of the ADR that don't make sense to you. I don't know what is missing there to answer your question.

[fsateler]

Btw, this merge explicitly is not about invoking unrelated partials. Its about being able to extract some parts of a large method into several smaller methods

[camertron]

@fsateler ah I used the word "partial" when I meant to say "additional sidecar asset." Correct me if I'm wrong, but a big part of your proposal is providing additional .html.erb files scoped to the component (i.e. sidecar assets) that get rendered from another sidecar asset. Perhaps my earlier comment makes more sense with the right terminology gsubbed in 😅 I'll make additional comments on the ADR 😄

[fsateler]

Hmm, any hint on how to investigate pvc test? I don't see how this change could cause a accessibility failure..

[Spone]

Hmm, any hint on how to investigate pvc test? I don't see how this change could cause a accessibility failure..

I re-ran the pvc test and it passed. It can be a bit flaky, so never mind :)

[joelhawksley]

Very nice! I'll have another round of feedback around docs/ADR once these items are resolved ❤️

[joelhawksley]

Suggested change

	## Multiple templates
	## Sub-templates

[Spone]

What about "Partial templates" or "Partials"?

Partial templates - usually just called "partials" - are another device for breaking the rendering process into more manageable chunks. With a partial, you can move the code for rendering a particular piece of a response to its own file.

https://guides.rubyonrails.org/layouts_and_rendering.html#using-partials

[Spone]

Or maybe "Sidecar partials" to remove ambiguity?

Or "Component partials" to match the phrasing used in the ADR.

[joelhawksley]

I like the term in theory, but in practice I think it's already "taken" by Rails' usage of it. As this feature is quite different from Rails partials, I'm hesitant to use the term.

[fsateler]

I have applied (almost) all suggestions. Thanks for the review. I liked the new method name pattern. I did not apply @Spone 's suggestion for the new name in the documentation, but that could be applied easily if there is consensus about that. IMO, the name partial could cause confusion, but that is just a gut feeling, I have no data to back this up.

[joelhawksley]

Another round, I think we're getting pretty close ❤️

[joelhawksley]

Looking at this during office hours today, I think we're in a tough spot. We now have a situation where some ViewComponent templates support variants and some don't, which I think would be quite confusing to consumers. (Although I'm of the opinion generally that variants shouldn't exist at all). This makes me want to lean more on @BlakeWilliams's inline templates proposal.

[fsateler]

Hi!

We now have a situation where some ViewComponent templates support variants and some don't, which I think would be quite confusing to consumers

I don't understand this point. AFAICT, you can have variants in this PR. Perhaps you mean you need to know which variant to render? That may be addressable with something like I proposed earlier: #451 (comment)

def render_foo_template **args
   case @__vc_variant
   when 'some_variant'
      render_list_template_some_variant **args
   else
      render_list_template_default_variant **args
   end
end

This would allow rendering the current template forcing a variant (via the _some_variant suffix) or via the current variant (via the method without suffix).

[fsateler]

Hi @joelhawksley ! I'm still interested in moving this forward, but I'd need to understand better this objection:

We now have a situation where some ViewComponent templates support variants and some don't, which I think would be quite confusing to consumers.

You can have variants in this PR, although they are not very ergonomic. Do you want me to think about improving that? Or is something else you have in mind?

[joelhawksley]

@fsateler I haven't forgotten about this PR ❤️

We're iterating on #1556 and want to see where we end up API-wise to inform our decisions on your PR here.

[barmintor]

Reading over this (trying to get the full context of the direction of variants), I

It creates a new Hash and a new RowComponent for each Row, each in one. This is actually the deadly problem, since creating large arrays is very problematic for the ruby GC and can create large pauses. A simple example with 6000 rows means passing 10% of the time in GC (as measured by miniprofiler flamegraph) vs 0% in the first version.

Isn't this exactly the problem lazy enumerators are supposed to solve?

[fsateler]

Reading over this (trying to get the full context of the direction of variants), I

It creates a new Hash and a new RowComponent for each Row, each in one. This is actually the deadly problem, since creating large arrays is very problematic for the ruby GC and can create large pauses. A simple example with 6000 rows means passing 10% of the time in GC (as measured by miniprofiler flamegraph) vs 0% in the first version.

Isn't this exactly the problem lazy enumerators are supposed to solve?

Not really. Lots of transformations are more memory efficient in lazy enumerators. For example, some_list.select(&:something?).reject(&:other?).take(10) will be a lot more efficient with lazy enumerators, because you avoid all the intermediate arrays.

In this case, however, we are doing a map. So, while we would be avoiding a new array allocation, it doesn't avoid allocating a new Hash and Component for each item, even if the objects are then inmediately discarded.

[joelhawksley]

@fsateler responding to #387 (comment) here ❤️

Revisiting this with @camertron and @BlakeWilliams, we're wondering if it'd make sense to first land hooks to the compiler that would allow a gem to add this behavior. I don't think it would be too bad to do so. We're still ultimately feeling hesitant to confuse folks' use of Slots, which are already quite complex.

I also wonder if we might want to consider a Rails core contribution to add named render helper methods for views such as render_users_show. I might take a look into this idea over the holidays here!

[fsateler]

@joelhawksley awesome! if there is anything that I can help with I'm happy to do so.