Fix render_parent when variants are involved (#1801)

* Fix render_parent when variants are involved

* Ok, yield seems to work

* Got it working for variants and inline templates

* Refactor a bit

* Fix Vale grammar issues; add #render_parent test back in; fix linting issues

* Merge functionality instead

* wip

* Fix tests

* Clean up

* Small fixes

* Update docs/api.md

Co-authored-by: Blake Williams <blakewilliams@github.com>

* Fix linting issues

* Regen docs

* Update lib/view_component/compiler.rb

Co-authored-by: Blake Williams <blakewilliams@github.com>

---------

Co-authored-by: Blake Williams <blake@blakewilliams.me>
Co-authored-by: Blake Williams <blakewilliams@github.com>

docs/CHANGELOG.md

@@ -14,6 +14,10 @@ nav_order: 5
 
     *Chris Nitsas*
 
+* Improve implementation of `#render_parent` so it respects variants and deep inheritance hierarchies.
+
+    *Cameron Dutro*
+
 * Add CharlieHR to users list.
 
     *Alex Balhatchet*

docs/api.md

@@ -103,14 +103,29 @@ Returns HTML that has been escaped by the respective template handler.
 ### `#render_parent`
 
 Subclass components that call `super` inside their template code will cause a
-double render if they emit the result:
+double render if they emit the result.
 
 ```erb
 <%= super %> # double-renders
-<% super %> # does not double-render
+<% super %> # doesn't double-render
 ```
 
-Calls `super`, returning `nil` to avoid rendering the result twice.
+`super` also doesn't consider the current variant. `render_parent` renders the
+parent template considering the current variant and emits the result without
+double-rendering.
+
+### `#render_parent_to_string`
+
+Renders the parent component to a string and returns it. This method is meant
+to be used inside custom #call methods when a string result is desired, eg.
+
+```ruby
+def call
+  "<div>#{render_parent_to_string}</div>"
+end
+```
+
+When rendering the parent inside an .erb template, use `#render_parent` instead.
 
 ### `#request` → [ActionDispatch::Request]
 

docs/guide/templates.md

@@ -120,15 +120,54 @@ end
 Since 2.55.0
 {: .label }
 
-To render a parent component's template from a subclass, call `render_parent`:
+To render a parent component's template from a subclass' template, use `#render_parent`:
 
 ```erb
 <%# my_link_component.html.erb %>
 <div class="base-component-template">
-  <% render_parent %>
+  <%= render_parent %>
 </div>
 ```
 
+If the parent supports the current variant, the variant will automatically be rendered.
+
+`#render_parent` also works with inline templates:
+
+```ruby
+class MyComponent < ViewComponent::Base
+  erb_template <<~ERB
+    <div>
+      <%= render_parent %>
+    </div>
+  ERB
+end
+```
+
+Finally, `#render_parent` also works inside `#call` methods:
+
+```ruby
+class MyComponent < ViewComponent::Base
+  def call
+    content_tag("div") do
+      render_parent
+    end
+  end
+end
+```
+
+When composing `#call` methods, keep in mind that `#render_parent` does not return a string. If a string is desired, call `#render_parent_to_string` instead. For example:
+
+```ruby
+class MyComponent < ViewComponent::Base
+  # "phone" variant
+  def call_phone
+    content_tag("div") do
+      "<div>#{render_parent_to_string}</div>"
+    end
+  end
+end
+```
+
 ## Trailing whitespace
 
 Code editors commonly add a trailing newline character to source files in keeping with the Unix standard. Including trailing whitespace in component templates can result in unwanted whitespace in the HTML, eg. if the component is rendered before the period at the end of a sentence.

lib/view_component/base.rb

@@ -116,18 +116,42 @@ def render_in(view_context, &block)
     end
 
     # Subclass components that call `super` inside their template code will cause a
-    # double render if they emit the result:
+    # double render if they emit the result.
     #
     # ```erb
     # <%= super %> # double-renders
-    # <% super %> # does not double-render
+    # <% super %> # doesn't double-render
     # ```
     #
-    # Calls `super`, returning `nil` to avoid rendering the result twice.
+    # `super` also doesn't consider the current variant. `render_parent` renders the
+    # parent template considering the current variant and emits the result without
+    # double-rendering.
     def render_parent
-      mtd = @__vc_variant ? "call_#{@__vc_variant}" : "call"
-      method(mtd).super_method.call
-      nil
+      @__vc_parent_render_level ||= 0 # ensure a good starting value
+
+      begin
+        target_render = self.class.instance_variable_get(:@__vc_ancestor_calls)[@__vc_parent_render_level]
+        @__vc_parent_render_level += 1
+
+        target_render.bind_call(self, @__vc_variant)
+        nil
+      ensure
+        @__vc_parent_render_level -= 1
+      end
+    end
+
+    # Renders the parent component to a string and returns it. This method is meant
+    # to be used inside custom #call methods when a string result is desired, eg.
+    #
+    # ```ruby
+    # def call
+    #   "<div>#{render_parent_to_string}</div>"
+    # end
+    # ```
+    #
+    # When rendering the parent inside an .erb template, use `#render_parent` instead.
+    def render_parent_to_string
+      capture { render_parent }
     end
 
     # Optional content to be returned after the rendered template.
@@ -459,6 +483,13 @@ def render_template_for(variant = nil)
         # Set collection parameter to the extended component
         child.with_collection_parameter provided_collection_parameter
 
+        if instance_methods(false).include?(:render_template_for)
+          vc_ancestor_calls = defined?(@__vc_ancestor_calls) ? @__vc_ancestor_calls.dup : []
+
+          vc_ancestor_calls.unshift(instance_method(:render_template_for))
+          child.instance_variable_set(:@__vc_ancestor_calls, vc_ancestor_calls)
+        end
+
         super
       end
 

lib/view_component/compiler.rb

@@ -56,17 +56,17 @@ def call
           RUBY
           # rubocop:enable Style/EvalWithLocation
 
+          component_class.define_method("_call_#{safe_class_name}", component_class.instance_method(:call))
+
           component_class.silence_redefinition_of_method("render_template_for")
           component_class.class_eval <<-RUBY, __FILE__, __LINE__ + 1
           def render_template_for(variant = nil)
-            call
+            _call_#{safe_class_name}
           end
           RUBY
         end
       else
         templates.each do |template|
-          # Remove existing compiled template methods,
-          # as Ruby warns when redefining a method.
           method_name = call_method_name(template[:variant])
 
           redefinition_lock.synchronize do
@@ -95,15 +95,20 @@ def #{method_name}
 
     def define_render_template_for
       variant_elsifs = variants.compact.uniq.map do |variant|
-        "elsif variant.to_sym == :'#{variant}'\n    #{call_method_name(variant)}"
+        safe_name = "_call_variant_#{normalized_variant_name(variant)}_#{safe_class_name}"
+        component_class.define_method(safe_name, component_class.instance_method(call_method_name(variant)))
+
+        "elsif variant.to_sym == :'#{variant}'\n    #{safe_name}"
       end.join("\n")
 
+      component_class.define_method("_call_#{safe_class_name}", component_class.instance_method(:call))
+
       body = <<-RUBY
         if variant.nil?
-          call
+          _call_#{safe_class_name}
         #{variant_elsifs}
         else
-          call
+          _call_#{safe_class_name}
         end
       RUBY
 
@@ -276,12 +281,17 @@ def normalized_variant_name(variant)
       variant.to_s.gsub("-", "__").gsub(".", "___")
     end
 
+    def safe_class_name
+      @safe_class_name ||= component_class.name.underscore.gsub("/", "__")
+    end
+
     def should_compile_superclass?
-      development? && templates.empty? && !has_inline_template? &&
-        !(
-          component_class.instance_methods(false).include?(:call) ||
-            component_class.private_instance_methods(false).include?(:call)
-        )
+      development? && templates.empty? && !has_inline_template? && !call_defined?
+    end
+
+    def call_defined?
+      component_class.instance_methods(false).include?(:call) ||
+        component_class.private_instance_methods(false).include?(:call)
     end
   end
 end

test/sandbox/app/components/inline_level1_component.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class InlineLevel1Component < ViewComponent::Base
+  def call
+    content_tag(:div, class: "level1-component")
+  end
+end

test/sandbox/app/components/inline_level2_component.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class InlineLevel2Component < Level2Component
+  def call
+    "<div level2-component base>#{render_parent_to_string}</div>"
+  end
+
+  def call_variant
+    "<div level2-component variant>#{render_parent_to_string}</div>"
+  end
+end

test/sandbox/app/components/inline_level3_component.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class InlineLevel3Component < Level2Component
+  def call
+    content_tag(:div, class: "level3-component base") do
+      render_parent
+    end
+  end
+
+  def call_variant
+    content_tag(:div, class: "level3-component variant") do
+      render_parent
+    end
+  end
+end

test/sandbox/app/components/level1_component.html.erb

@@ -0,0 +1 @@
+<div class="level1-component"></div>

test/sandbox/app/components/level1_component.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+class Level1Component < ViewComponent::Base
+end

test/sandbox/app/components/level2_component.html+variant.erb

@@ -0,0 +1,3 @@
+<div class="level2-component variant">
+  <%= render_parent %>
+</div>

test/sandbox/app/components/level2_component.html.erb

@@ -0,0 +1,3 @@
+<div class="level2-component base">
+  <%= render_parent %>
+</div>

test/sandbox/app/components/level2_component.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+class Level2Component < Level1Component
+end

test/sandbox/app/components/level3_component.html+variant.erb

@@ -0,0 +1,3 @@
+<div class="level3-component variant">
+  <%= render_parent %>
+</div>

test/sandbox/app/components/level3_component.html.erb

@@ -0,0 +1,3 @@
+<div class="level3-component base">
+  <%= render_parent %>
+</div>

test/sandbox/app/components/level3_component.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+class Level3Component < Level2Component
+end

test/sandbox/test/inline_template_test.rb

@@ -30,6 +30,9 @@ def initialize(name)
   class InlineErbSubclassComponent < InlineErbComponent
     erb_template <<~ERB
       <h1>Hey, <%= name %>!</h1>
+      <div class="parent">
+        <%= render_parent %>
+      </div>
     ERB
   end
 
@@ -76,6 +79,14 @@ def initialize(name)
     end
   end
 
+  class InlineComponentDerivedFromComponentSupportingVariants < Level2Component
+    erb_template <<~ERB
+      <div class="inline-template">
+        <%= render_parent %>
+      </div>
+    ERB
+  end
+
   test "renders inline templates" do
     render_inline(InlineErbComponent.new("Fox Mulder"))
 
@@ -112,6 +123,20 @@ def initialize(name)
     assert_selector("h1", text: "Hey, Fox Mulder!")
   end
 
+  test "child components can render their parent" do
+    render_inline(InlineErbSubclassComponent.new("Fox Mulder"))
+
+    assert_selector(".parent h1", text: "Hello, Fox Mulder!")
+  end
+
+  test "inline child component propagates variant to parent" do
+    with_variant :variant do
+      render_inline(InlineComponentDerivedFromComponentSupportingVariants.new)
+    end
+
+    assert_selector ".inline-template .level2-component.variant .level1-component"
+  end
+
   test "calling template methods multiple times raises an exception" do
     error = assert_raises ViewComponent::MultipleInlineTemplatesError do
       Class.new(InlineErbComponent) do