From 4e864d518db066044f423683239a34b6ccbe0efa Mon Sep 17 00:00:00 2001 From: Joel Hawksley Date: Thu, 23 Jul 2020 15:15:27 -0600 Subject: [PATCH 1/2] Add support for multiple templates. Co-authored-by: Rob Sterner --- docs/CHANGELOG.md | 4 +++ docs/guide/templates.md | 33 +++++++++++++++++++ lib/view_component/base.rb | 2 +- lib/view_component/compiler.rb | 32 ++++++++++++++++-- .../multiple_templates_component.rb | 18 ++++++++++ .../list.html.erb | 5 +++ .../summary.html.erb | 1 + test/view_component/view_component_test.rb | 25 ++++++++++++++ 8 files changed, 116 insertions(+), 4 deletions(-) create mode 100644 test/sandbox/app/components/multiple_templates_component.rb create mode 100644 test/sandbox/app/components/multiple_templates_component/list.html.erb create mode 100644 test/sandbox/app/components/multiple_templates_component/summary.html.erb diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index d482a1c47..524aa1507 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -113,6 +113,10 @@ title: Changelog *Joel Hawksley* +* Add support for multiple templates. + + *Rob Sterner*, *Joel Hawksley* + ## 2.36.0 * Add `slot_type` helper method. diff --git a/docs/guide/templates.md b/docs/guide/templates.md index f3144f5e0..d55b8ae84 100644 --- a/docs/guide/templates.md +++ b/docs/guide/templates.md @@ -92,3 +92,36 @@ Component subclasses inherit the parent component's template if they don't defin class MyLinkComponent < LinkComponent end ``` + +#### Multiple templates + +ViewComponents can render multiple templates defined in the sidecar directory: + +``` +app/components +├── ... +├── test_component.rb +├── test_component +| ├── list.html.erb +| └── summary.html.erb +├── ... +``` + +Templates are compiled to methods in the format `call_#{template_basename}`, which can then be called in the component. + +```ruby +class TestComponent < ViewComponent::Base + def initialize(mode:) + @mode = mode + end + + def call + case @mode + when :list + call_list + when :summary + call_summary + end + end +end +``` \ No newline at end of file diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb index 611bb7e0f..28ff812e0 100644 --- a/lib/view_component/base.rb +++ b/lib/view_component/base.rb @@ -331,7 +331,7 @@ def _sidecar_files(extensions) # view files in the same directory as the component sidecar_files = Dir["#{directory}/#{component_name}.*{#{extensions}}"] - sidecar_directory_files = Dir["#{directory}/#{component_name}/#{filename}.*{#{extensions}}"] + sidecar_directory_files = Dir["#{directory}/#{component_name}/*.*{#{extensions}}"] (sidecar_files - [source_location] + sidecar_directory_files + nested_component_files).uniq end diff --git a/lib/view_component/compiler.rb b/lib/view_component/compiler.rb index 620075b95..5fe6a0200 100644 --- a/lib/view_component/compiler.rb +++ b/lib/view_component/compiler.rb @@ -43,7 +43,18 @@ def compile(raise_errors: false) templates.each do |template| # Remove existing compiled template methods, # as Ruby warns when redefining a method. - method_name = call_method_name(template[:variant]) + pieces = File.basename(template[:path]).split(".") + + method_name = + # If the template matches the name of the component, + # set the method name with call_method_name + if pieces.first == component_class.name.demodulize.underscore + call_method_name(template[:variant]) + # Otherwise, append the name of the template to + # call_method_name + else + "#{call_method_name(template[:variant])}_#{pieces.first.to_sym}" + end if component_class.instance_methods.include?(method_name.to_sym) component_class.send(:undef_method, method_name.to_sym) @@ -98,7 +109,14 @@ def template_errors errors << "Could not find a template file or inline render method for #{component_class}." end - if templates.count { |template| template[:variant].nil? } > 1 + # Ensure that template base names are unique + # for each variant + unique_templates = + templates.map do |template| + template[:base_name] + template[:variant].to_s + end + + if unique_templates.length != unique_templates.uniq.length errors << "More than one template found for #{component_class}. " \ "There can only be one default template file per component." @@ -118,7 +136,14 @@ def template_errors "There can only be one template file per variant." end - if templates.find { |template| template[:variant].nil? } && inline_calls_defined_on_self.include?(:call) + default_template_exists = + templates.find do |template| + pieces = File.basename(template[:path]).split(".") + + template[:variant].nil? && pieces.first == component_class.name.demodulize.underscore + end + + if default_template_exists && inline_calls_defined_on_self.include?(:call) errors << "Template file and inline render method found for #{component_class}. " \ "There can only be a template file or inline render method per component." @@ -151,6 +176,7 @@ def templates pieces = File.basename(path).split(".") memo << { path: path, + base_name: path.split(File::SEPARATOR).last.split(".").first, variant: pieces.second.split("+").second&.to_sym, handler: pieces.last } diff --git a/test/sandbox/app/components/multiple_templates_component.rb b/test/sandbox/app/components/multiple_templates_component.rb new file mode 100644 index 000000000..b42e5a6aa --- /dev/null +++ b/test/sandbox/app/components/multiple_templates_component.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +class MultipleTemplatesComponent < ViewComponent::Base + def initialize(mode:) + @mode = mode + + @items = ["Apple", "Banana", "Pear"] + end + + def call + case @mode + when :list + call_list + when :summary + call_summary + end + end +end diff --git a/test/sandbox/app/components/multiple_templates_component/list.html.erb b/test/sandbox/app/components/multiple_templates_component/list.html.erb new file mode 100644 index 000000000..02bf02702 --- /dev/null +++ b/test/sandbox/app/components/multiple_templates_component/list.html.erb @@ -0,0 +1,5 @@ + diff --git a/test/sandbox/app/components/multiple_templates_component/summary.html.erb b/test/sandbox/app/components/multiple_templates_component/summary.html.erb new file mode 100644 index 000000000..ef9845aa5 --- /dev/null +++ b/test/sandbox/app/components/multiple_templates_component/summary.html.erb @@ -0,0 +1 @@ +
The items are: <%= @items.to_sentence %>
diff --git a/test/view_component/view_component_test.rb b/test/view_component/view_component_test.rb index 4e6adf427..030edfc0e 100644 --- a/test/view_component/view_component_test.rb +++ b/test/view_component/view_component_test.rb @@ -776,6 +776,18 @@ def test_collection_component_with_trailing_comma_attr_reader assert_match(/ProductReaderOopsComponent initializer is empty or invalid/, exception.message) end + def test_render_multiple_templates + render_inline(MultipleTemplatesComponent.new(mode: :list)) + + assert_selector("li", text: "Apple") + assert_selector("li", text: "Banana") + assert_selector("li", text: "Pear") + + render_inline(MultipleTemplatesComponent.new(mode: :summary)) + + assert_selector("div", text: "Apple, Banana, and Pear") + end + def test_renders_component_using_rails_config render_inline(RailsConfigComponent.new) @@ -888,4 +900,17 @@ def test_output_postamble assert_text("Hello, World!") end + + private + + def modify_file(file, content) + filename = Rails.root.join(file) + old_content = File.read(filename) + begin + File.open(filename, "wb+") { |f| f.write(content) } + yield + ensure + File.open(filename, "wb+") { |f| f.write(old_content) } + end + end end From dff7c0df35a5ea8de6fd78f639d18f8e61a50270 Mon Sep 17 00:00:00 2001 From: Jose Solas Date: Wed, 13 Oct 2021 14:08:10 -0300 Subject: [PATCH 2/2] Add ability to define templates that take parameters --- adr/0003-multiple-templates-per-component.md | 88 +++++++++++++++++++ docs/CHANGELOG.md | 8 +- docs/guide/templates.md | 14 +-- lib/view_component/compiler.rb | 12 +-- .../multiple_templates_component.rb | 13 +-- .../list.html.erb | 2 +- .../multiple_templates_component.html.erb | 4 + .../summary.html.erb | 2 +- test/view_component/view_component_test.rb | 20 +---- 9 files changed, 116 insertions(+), 47 deletions(-) create mode 100644 adr/0003-multiple-templates-per-component.md create mode 100644 test/sandbox/app/components/multiple_templates_component/multiple_templates_component.html.erb diff --git a/adr/0003-multiple-templates-per-component.md b/adr/0003-multiple-templates-per-component.md new file mode 100644 index 000000000..6392ff2eb --- /dev/null +++ b/adr/0003-multiple-templates-per-component.md @@ -0,0 +1,88 @@ +# 1. Allow multiple templates + +Date: 2021-10-13 + +## Status + +Proposed. + +## Context + +As components become larger (for example, because you are implementing a whole page), it becomes +useful to be able to extract sections of the view to a different file. ActionView has +partials, and ViewComponent lacks a similar mechanism. + +ActionView partials have the problem that their interface is not introspectable. Data +may be passed into the partial via ivars or locals, and it is impossible to know +which without actually opening up the file. Additionally, partials are globally +invocable, thus making it difficult to detect if a given partial is in use or not, +and who are its users. + +## Considered Options + +* Introduce component partials to components +* Keep components as-is + +### Component partials + +Allow multiple ERB templates available within the component, and make it possible to +invoke them from the main view.Templates are compiled to methods in the format `render_#{template_basename}(locals = {})` + +**Pros:** +* Better performance due to lack of GC pressure and object creation +* Reduces the number of components needed to express a more complex view. +* Extracted sections are not exposed outside the component, thus reducing component library API surface. + +**Cons:** +* Another concept for users of ViewComponent to learn and understand. +* Components are no longer the only way to encapsulate behavior. + +### Partial components by [fstaler](https://github.com/fsateler) + +In this approach the template arguments are previously defined in the `template_arguments` method + +[See here](https://github.com/github/view_component/pull/451): + +**Pros:** +* Better performance due to lack of GC pressure and object creation +* Reduces the number of components needed to express a more complex view. +* Extracted sections are not exposed outside the component, thus reducing component library API surface. + +**Cons:** +* Another concept for users of ViewComponent to learn and understand. +* Components are no longer the only way to encapsulate behavior. +* `call_foo` api feels awkward and not very Rails like +* Declare templates and their arguments explicitly before using them + +### Keeping components as-is + +**Pros:** +* The API remains simple and components are the only way to encapsulate behavior. +* Encourages creating reusable sub-components. + +**Cons:** +* Extracting a component results in more GC and intermediate objects. +* Extracting a component may result in tightly coupled but split components. +* Creates new public components thus expanding component library API surface. + +## Decision + +We will allow having multiple templates in the sidecar asset. Each asset will be compiled to +it's own method `render_`. I think it is simple, similar to rails and meets what is expected + +## Consequences + +This implementation has better performance characteristics over both an extracted component +and ActionView partials, because it avoids creating intermediate objects, and the overhead of +creating bindings and `instance_exec`. +Having explicit arguments makes the interface explicit. + +TODO: The following are consequences of the current approach, but the approach might be extended +to avoid them: + +The interface to render a sidecar partial would be a method call, and depart from the usual +`render(*)` interface used in ActionView. + +The generated methods cannot have arguments with default values. + +The generated methods are public, and thus could be invoked by a third party. \ No newline at end of file diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 524aa1507..7105f662b 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -36,6 +36,10 @@ title: Changelog *Hans Lemuet* +* Add support for multiple templates. + + *Rob Sterner*, *Joel Hawksley* + ## 2.40.0 * Replace antipatterns section in the documentation with best practices. @@ -113,10 +117,6 @@ title: Changelog *Joel Hawksley* -* Add support for multiple templates. - - *Rob Sterner*, *Joel Hawksley* - ## 2.36.0 * Add `slot_type` helper method. diff --git a/docs/guide/templates.md b/docs/guide/templates.md index d55b8ae84..abe693b81 100644 --- a/docs/guide/templates.md +++ b/docs/guide/templates.md @@ -93,11 +93,11 @@ class MyLinkComponent < LinkComponent end ``` -#### Multiple templates +## Multiple templates with arguments -ViewComponents can render multiple templates defined in the sidecar directory: +ViewComponents can render multiple templates defined in the sidecar directory and send arguments to it: -``` +```console app/components ├── ... ├── test_component.rb @@ -107,7 +107,7 @@ app/components ├── ... ``` -Templates are compiled to methods in the format `call_#{template_basename}`, which can then be called in the component. +Templates are compiled to methods in the format `render_#{template_basename}(locals = {})`, which can then be called in the component. ```ruby class TestComponent < ViewComponent::Base @@ -118,10 +118,10 @@ class TestComponent < ViewComponent::Base def call case @mode when :list - call_list + render_list number: 1 when :summary - call_summary + render_summary string: "foo" end end end -``` \ No newline at end of file +``` diff --git a/lib/view_component/compiler.rb b/lib/view_component/compiler.rb index 5fe6a0200..4b369bb87 100644 --- a/lib/view_component/compiler.rb +++ b/lib/view_component/compiler.rb @@ -50,20 +50,22 @@ def compile(raise_errors: false) # set the method name with call_method_name if pieces.first == component_class.name.demodulize.underscore call_method_name(template[:variant]) - # Otherwise, append the name of the template to - # call_method_name + # Otherwise, append the name of the template to render_ else - "#{call_method_name(template[:variant])}_#{pieces.first.to_sym}" + "render_#{pieces.first.to_sym}" end if component_class.instance_methods.include?(method_name.to_sym) component_class.send(:undef_method, method_name.to_sym) end - component_class.class_eval <<-RUBY, template[:path], -1 - def #{method_name} + component_class.class_eval <<-RUBY, template[:path], -2 + def #{method_name}(**locals) + old_buffer = @output_buffer if defined? @output_buffer @output_buffer = ActionView::OutputBuffer.new #{compiled_template(template[:path])} + ensure + @output_buffer = old_buffer end RUBY end diff --git a/test/sandbox/app/components/multiple_templates_component.rb b/test/sandbox/app/components/multiple_templates_component.rb index b42e5a6aa..62aff04ea 100644 --- a/test/sandbox/app/components/multiple_templates_component.rb +++ b/test/sandbox/app/components/multiple_templates_component.rb @@ -1,18 +1,7 @@ # frozen_string_literal: true class MultipleTemplatesComponent < ViewComponent::Base - def initialize(mode:) - @mode = mode - + def initialize @items = ["Apple", "Banana", "Pear"] end - - def call - case @mode - when :list - call_list - when :summary - call_summary - end - end end diff --git a/test/sandbox/app/components/multiple_templates_component/list.html.erb b/test/sandbox/app/components/multiple_templates_component/list.html.erb index 02bf02702..13683cae4 100644 --- a/test/sandbox/app/components/multiple_templates_component/list.html.erb +++ b/test/sandbox/app/components/multiple_templates_component/list.html.erb @@ -1,4 +1,4 @@ -
    +
      <% @items.each do |item| %>
    • <%= item %>
      • <% end %> diff --git a/test/sandbox/app/components/multiple_templates_component/multiple_templates_component.html.erb b/test/sandbox/app/components/multiple_templates_component/multiple_templates_component.html.erb new file mode 100644 index 000000000..e3d1c440f --- /dev/null +++ b/test/sandbox/app/components/multiple_templates_component/multiple_templates_component.html.erb @@ -0,0 +1,4 @@ +
      + <%= render_summary string: "foo" %> + <%= render_list number: 1 %> +
      diff --git a/test/sandbox/app/components/multiple_templates_component/summary.html.erb b/test/sandbox/app/components/multiple_templates_component/summary.html.erb index ef9845aa5..853913d47 100644 --- a/test/sandbox/app/components/multiple_templates_component/summary.html.erb +++ b/test/sandbox/app/components/multiple_templates_component/summary.html.erb @@ -1 +1 @@ -
      The items are: <%= @items.to_sentence %>
      +
      The items are: <%= @items.to_sentence %>, <%= locals[:string] %>
      diff --git a/test/view_component/view_component_test.rb b/test/view_component/view_component_test.rb index 030edfc0e..842f6cba4 100644 --- a/test/view_component/view_component_test.rb +++ b/test/view_component/view_component_test.rb @@ -777,15 +777,14 @@ def test_collection_component_with_trailing_comma_attr_reader end def test_render_multiple_templates - render_inline(MultipleTemplatesComponent.new(mode: :list)) + render_inline(MultipleTemplatesComponent.new) + assert_selector("div", text: "The items are: Apple, Banana, and Pear, foo") assert_selector("li", text: "Apple") assert_selector("li", text: "Banana") assert_selector("li", text: "Pear") - render_inline(MultipleTemplatesComponent.new(mode: :summary)) - - assert_selector("div", text: "Apple, Banana, and Pear") + assert_selector("div.container") end def test_renders_component_using_rails_config @@ -900,17 +899,4 @@ def test_output_postamble assert_text("Hello, World!") end - - private - - def modify_file(file, content) - filename = Rails.root.join(file) - old_content = File.read(filename) - begin - File.open(filename, "wb+") { |f| f.write(content) } - yield - ensure - File.open(filename, "wb+") { |f| f.write(old_content) } - end - end end