Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple views #387

Open
fsateler opened this issue Jun 30, 2020 · 17 comments · May be fixed by #451 or #1096
Open

Support multiple views #387

fsateler opened this issue Jun 30, 2020 · 17 comments · May be fixed by #451 or #1096

Comments

@fsateler
Copy link
Contributor

Feature request

Allow refactoring small view parts into a separate view, and be able to render them into the template. These views should not be available outside the component.

app/components/my_component/my_component.html.erb
app/components/my_component/a_part.html.erb
<%# my_component.html.erb %>
<%= render 'a_part' %>

Motivation

Sometimes it is useful to separate a sub-component from the component. For example, if I want to render a popover with extra info about a particular model, I would want to extract that popover into a separate view.

Alternative Solutions:

  1. Extract a new component. Because the popover is not a generic one to be used in other parts of the application, it doesn't make much sense to create a (now public) component for it.
  2. Use content_for. This is not suitable if the partial needs to take arguments. In my example, it wouldn't work if I'm displaying a list of models and I want to show the popover for each model.

Prior art:
ActionView does support partials, of course.
Cells does allow multiple views per cell, and allows using render view: 'a_part'.

Possible solutions I see

  1. Provide a method that prefixes the component dir and then goes on to call render template: with the full path. I would suspect this is relatively slow given that render is slow in ActionView.
  2. Precompile all the templates into methods with a reserved prefix, and allow calling them from the view. For example, using extra_views_{view_name}.
@joelhawksley
Copy link
Member

Precompile all the templates into methods with a reserved prefix, and allow calling them from the view.

@fsateler too funny. My colleague @fermion and I have a work-in-progress branch for this feature! It's next on my plate once Slots lands.

@fsateler
Copy link
Contributor Author

@fsateler too funny. My colleague @fermion and I have a work-in-progress branch for this feature!

Great minds think alike! 😆

If you can publish that branch somewhere I can test it to check if it covers the use cases we have (we are slowly transitioning from Cells to ViewComponent)

@joelhawksley
Copy link
Member

@fsateler that would be great! The branch is multiple-templates: https://github.com/github/view_component/tree/multiple-templates

@fsateler
Copy link
Contributor Author

fsateler commented Jul 1, 2020

Hey, I tried out that branch. The good news: it works for partials without locals. Things I couldn't get to work:

  1. Partials with locals. I have been googling around and I could not find a simple way to fix this. Alternatives I found:
    a. Use a locals or similar keyword argument, and have the partial use locals[:foo] instead of foo directly.
    b. Recompile the template on each invocation. Possibly with caching based on the keys (local variable names) provided.
    c. Have the Component pass the arguments out-of-bound. This could be a bit annoying but could provide argument-checking benefits. Something like partial_arguments :my_partial [:foo, :bar] would generate a method call_my_partial foo:, bar:.
  2. Using helpers that assume an ActionView-like environment for rendering partials cannot reach these partials. I figure this could be out of scope/worked around by a render monkey patch on the component.

@fsateler
Copy link
Contributor Author

fsateler commented Jul 25, 2020

So, I know I agreed to file a new issue, but I think what I have written here covers this issue, while also touching on things like #39 and #410 . So I'll just post it here, and then maybe copy paste the relevant parts into new issues.

I'm currently trying to migrate from Cells to ViewComponent. We are using cells for rendering whole pages, not just the individual components. There are a couple of use cases that (AFAICT) are not possible (yet) with view components. I'm going to show them in a simplified form, but sticking to what we actually do.

Use case 1: splitting parts out

Say we have a regular form, for an object with many fields. We split the fields into sections:

<%# show.erb %>
<%= simple_form_for model do |f| %>
  <%= render view: 'personal_information', locals: { form: f } %>
  <%= render view: 'work_information', locals: { form: f } %>
  <%= render view: 'actions' %>
<% end %>
<%# personal_information.erb %>
<%= form.input :first_name %>
<%= form.input :last_name %>
<%# work_information.erb %>
<%= form.input :company_name %>
<%= form.input :company_role %>
<%# actions.erb %>
<%= cell(SubmitButtonCell) %>
<%= cell(GoBackButtonCell) %>

Use case 2: polymorphism

This is an extension of the previous one. We are using STI for some of our models. For example, we store data from people from multiple countries, and since some of the data would differ between countries (think different national id regimes or different geographical units), we use STI to be able to validate the data for each country. Thus we would have Person::Country1, Person::Country2 etc for each country we operate in.

I'm going to present a simplified version of the form component for said Person entity:

app/cells/person/form_cell.rb
app/cells/person/country1/form_cell.rb
app/cells/person/country2/form_cell.rb
app/cells/person/form/show.erb
app/cells/person/country1/form/address_information.erb
app/cells/person/country2/form/address_information.erb

The base Person::FormCell implements a cells builder that automatically instantiates the right cell depending on the country. For example, if the model is a Person::Country1, it would instantiate Person::Country1::FormCell.

The main template (show.erb) would be as follows:

<%= simple_form_for model do |f| %>
  <%# we render the common parts in the main template %>
  <%= f.input :first_name %>
  <%= f.input :last_name %>
  <%= render view: 'address_information', locals: { form: f } %>
<% end %>

And then each country would implement a different version:

<%# app/cells/person/country1/form/address_information.erb %>
<%= form.input :state %>
<%= form.input :county %>
<%# app/cells/person/country2/form/address_information.erb %>
<%= form.input :region %>
<%= form.input :city %>
<%= form.input :town %>

Usage in the edit and new views does not need to know about which country we are dealing with. We just call:

<%= cell(Person::FormCell, @person) %>

Use case 3: List/Table components

Sometimes, a component will render a list or a table of elements. In those cases, it is nice to be able to iterate over the list, and render each element from a separate template:

app/cells/person/table_cell.rb
app/cells/person/table/show.erb
app/cells/person/table/row.erb

Where the templates are as follows:

<%# show.erb %>
<table class="table table-condensed">
  <tbody>
    <% items.each_with_index do |item, i| %>
      <%= render view: 'row', locals: { item: item, index: i } %>
    <% end %>
  </tbody>
</table
<%# row.erb %>
<tr data-index="<%= index %>" class="<%= index.even? ? 'even' : 'odd' %>">
    <td><%= item.first_name %></td>
    <td><%= item.last_name %></td>
</tr>

Requirements to be solved

  1. Be able to subclass components.
  2. Break out partials, without extra parameters
  3. Break out partials, with extra parameters.
  4. Be able to override said partials in subclasses.

Ideas

There is the idea of allowing slot templates. This would solve the issues 2 and 3, but may need to be carefully implemented to allow polymorphism (issue 4). Issue 1 is currently possible. What I don't understand is, given there are slot templates, are multiple templates needed? AFAICT, slot templates would be strictly more featureful than multiple templates (as currently implemented).

Another idea, which we didn't get the chance to discuss, is: keep the multiple templates approach, but allow specifying (in the component), what arguments does the template take:

class MyComponent < ViewComponent::Base
  template_arguments row: [:item, :color] 
  # This would cause the row template to be compiled into:
  def call_row item:, color:
    # compiled erb template here
  end
end
A basic implementation on top of the multiple-templates branch:
diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb
index 2f31aaf..c928904 100644
--- a/lib/view_component/base.rb
+++ b/lib/view_component/base.rb
@@ -262,11 +262,21 @@ module ViewComponent
             else
               "#{call_method_name(template[:variant])}_#{pieces.first.to_sym}"
             end
+          method_args =
+            # If the template matches the name of the component,
+            # set the method name with call_method_name
+            if pieces.first == name.demodulize.underscore
+              []
+            # Otherwise, append the name of the template to
+            # call_method_name
+            else
+              (@template_arguments || {}).fetch(pieces.first, []).map { |arg| "#{arg}:" }
+            end
 
           undef_method(method_name.to_sym) if instance_methods.include?(method_name.to_sym)
 
           class_eval <<-RUBY, template[:path], line_number
-            def #{method_name}
+            def #{method_name}(#{method_args.join(", ")})
               @output_buffer = ActionView::OutputBuffer.new
               #{compiled_template(template[:path])}
             end
@@ -276,6 +286,11 @@ module ViewComponent
         @compiled = true
       end
 
+      def template_arguments(**args)
+        @template_arguments ||= {}
+        @template_arguments.merge! args.stringify_keys
+      end
+
       # we'll eventually want to update this to support other types
       def type
         "text/html"
diff --git a/test/app/components/multiple_templates_component.rb b/test/app/components/multiple_templates_component.rb
index b42e5a6..cb4973f 100644
--- a/test/app/components/multiple_templates_component.rb
+++ b/test/app/components/multiple_templates_component.rb
@@ -7,12 +7,14 @@ class MultipleTemplatesComponent < ViewComponent::Base
     @items = ["Apple", "Banana", "Pear"]
   end
 
+  template_arguments list: [:number], summary: [:string]
+
   def call
     case @mode
     when :list
-      call_list
+      call_list number: 1
     when :summary
-      call_summary
+      call_summary string: "foo"
     end
   end
 end
diff --git a/test/app/components/multiple_templates_component/list.html.erb b/test/app/components/multiple_templates_component/list.html.erb
index 02bf027..767328f 100644
--- a/test/app/components/multiple_templates_component/list.html.erb
+++ b/test/app/components/multiple_templates_component/list.html.erb
@@ -1,4 +1,5 @@
 <ul>
+  <%= number %>
   <% @items.each do |item| %>
     <li><%= item %></li>
   <% end %>
diff --git a/test/app/components/multiple_templates_component/summary.html.erb b/test/app/components/multiple_templates_component/summary.html.erb
index ef9845a..6b988c1 100644
--- a/test/app/components/multiple_templates_component/summary.html.erb
+++ b/test/app/components/multiple_templates_component/summary.html.erb
@@ -1 +1 @@
-<div>The items are: <%= @items.to_sentence %></div>
+<div>The items are: <%= @items.to_sentence %>, <%= string %></div>
diff --git a/test/view_component/view_component_test.rb b/test/view_component/view_component_test.rb
index b6a8b7a..1ec3c97 100644
--- a/test/view_component/view_component_test.rb
+++ b/test/view_component/view_component_test.rb
@@ -552,10 +552,12 @@ class ViewComponentTest < ViewComponent::TestCase
     assert_selector("li", text: "Apple")
     assert_selector("li", text: "Banana")
     assert_selector("li", text: "Pear")
+    assert_selector("ul", text: "1")
 
     render_inline(MultipleTemplatesComponent.new(mode: :summary))
 
     assert_selector("div", text: "Apple, Banana, and Pear")
+    assert_selector("div", text: "foo")
   end
 
   private

@fsateler fsateler linked a pull request Aug 23, 2020 that will close this issue
@stale
Copy link

stale bot commented Aug 29, 2020

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.

@stale stale bot added the stale label Aug 29, 2020
@stale stale bot closed this as completed Sep 5, 2020
@joelhawksley joelhawksley reopened this Sep 14, 2020
@fsateler
Copy link
Contributor Author

So, we chatted with @BlakeWilliams @camertron @joelhawksley and the way forward for now is:

  1. A DSL for defining method arguments is kind of unpleasant. Unfortunately, ruby does not provide us with a way to define signatures without actually defining a method. Therefore, we will move forward with a first version of allowing a single argument locals.
  2. Each template would define a method called render_<template_name>(locals = {}). While somethign like render template: :foo would be very rails-like, we are already defining a method so it doesn't make much sense to add an indirection layer.
  3. We will not be defining the locals inside the generated method like rails does. They would need to be accessed with locals[:foo].
  4. This means we will lose static argument checking, but we gain not defining a custom dsl. This is an area for future experimentation. Ideas ranged from AST parsing, sorbet-style sigs, or even new ruby features.

I think these are the main conclusions of what we discussed. Please comment if I forgot something.

I'll begin work on updating the PR along these lines.

@jsolas jsolas linked a pull request Oct 13, 2021 that will close this issue
@fsateler
Copy link
Contributor Author

Hi! I saw @joelhawksley just landed a PR in rails, adding a syntax for defining the arguments a view template accepts: rails/rails#45602 . I would be happy to update #451 to allow this.

@joelhawksley
Copy link
Member

@fsateler we were just chatting about this last week! I wrote that Rails PR in part as inspiration from #451. I'm still trying to wrap my head around how we should incorporate the change into VC. I'd love to hear your thoughts and recommendations ❤️

@fsateler
Copy link
Contributor Author

I think because VC does not have the legacy AV does, we can be a bit more strict about the locals marker: a missing locals marker means no arguments, that is, no marker == locals: ().

That being said, there is still one point which this new marker does not address, is how to expose the compiled method. AV requires you to call render :viewname, as the compiled method name is almost gibberish (_{view_path_slug}_{hash}_{__id__}). On #451 and #1096 several alternatives were discussed, but I believe the slots-based proposal (I understand this is still your preference, please correct me if this has changed) will introduce performance issues.

I'd like to summarize what I believe are the options:

  1. Expose the methods directly. This means we compile to somehting like render_{view_name_slug}, document that, and it becomes part of the API.
  2. Using the slots-based API. This currently has performance issues. I think (but don't really know) that these are surmountable, but would require a big refactor to how slots work.
  3. Expose a render-like API. We have not explored this option very much, perhaps we should?

@joelhawksley
Copy link
Member

@fsateler I'm leaning towards #1.

However, @BlakeWilliams and I are starting to wonder whether we should implement this feature in Rails first, generating render_ methods for templates that have the locals marker. Given that idea, I'm not sure how we would want to proceed here as we would likely collide with those methods or at the least create confusion between the two implementations.

What do you think?

@fsateler
Copy link
Contributor Author

@joelhawksley what sort of conflict are you envisioning? Creating rails view objects is not something you can (easily) do, so how would you access them? Or perhaps you are intending to define them with a slug of the full view path. That is, given a view app/views/users/new.html.erb, would it become a global helper render_app_views_users_new? And what about partials? Anyway, I think they can be disambiguated by having an object in VC that can delegate to the rails view engine: you can't call render_app_views_users_new directly, but rather via a "namespace" object: global_views.render_app_views_users_new.

@joelhawksley
Copy link
Member

would it become a global helper render_app_views_users_new

Yes. And I'm not sure a namespace is a good way to go, as I don't think consumers would expect that behavior. I'd expect such global helpers to feel just like the existing Rails URL/path helpers.

How about we do render_template_x for now?

@fsateler
Copy link
Contributor Author

For VC that sounds good to me! Because the methods are scoped to the component object, there is no ambiguity between templates with the same name but in different components. Just so we are all on the same page, what I want to do is, given the following component:

app/components/table_component.rb
app/components/table_component/table_component.html.erb
app/components/table_component/row.html.erb

I would generate the method render_template_row within TableComponent.

@fsateler
Copy link
Contributor Author

Hi! I have updated #451 as discussed. Please take a look.

@gap777
Copy link

gap777 commented Mar 3, 2023

What's the status of this excellent idea?

@fsateler
Copy link
Contributor Author

fsateler commented Aug 3, 2023

Hi! @joelhawksley! Can I do anything to move this forward? I'd really like to go forward with this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants