Fix duplicate template error bug (#2121)

* Fix duplicate template error bug

This change fixes the bug reported where a component will report that it
has both a template and a `call` method defined despite only having an
HTML template.

I was able to reproduce this bug by pulling in Avo (like reported in the
initial bug report) and running `vegeta` (to trigger parallel requests
since I had a hunch this was a race condition):

```
echo "GET http://localhost:3000/avo/resources/cars/1" | vegeta attack -duration=2s
```

This reliably reproduced the error:

```
ActionView::Template::Error (Template file and inline render method found for Avo::CoverPhotoComponent. There can only be a template file or inline render method per component.
Template file and inline render method found for variant '' in Avo::CoverPhotoComponent. There can only be a template file or inline render method per variant.
Templates:
```

I added _a lot_ of debug logs to understand the race condition that I
thought was occurring, and realized that multiple threads are calling
`gather_templates` _and_ mutating the `@templates` array at the
same.When looking at the old compiler code and realized that this likely
isn't new behavior. This led the investigation towards how we collect
and surface errors or otherwise might modify templates. It turns out
there's a difference in the new and old compiler code after the
refactor:

```ruby
\# old
def template_errors
  @__vc_template_errors ||=

\# new
def gather_template_errors(raise_errors)
  errors = []
```

_We're not memoizing the errors like we used to_. This is more correct
behavior, but explains how a race condition would make this error case
much more difficult to occur in older versions of the compiler.

This change brings us back to the old behavior by memoizing the errors
we collect in `gather_template_errors` but the `@templates` ivar is
still being mutated. I don't want to change _too much_ in this PR, but a
subsequent change might be wrapping the entire `compile` method in the
`redefinition_lock` (and renaming it to `compile_lock`) to avoid similar
issues in the future.

I did not include a test because it's dfficult to reproduce this race
condition reliably in the test environment.

I believe this _should_ close out #2114

* Fix tests, raise errors for compiled components with errors

* There isn't always a default template when there are errors.

* Make standard happy

ugh

* Remove debug code

* Fail compilation when errors present

* Revert default template change

* Add informative comment

* Add changelog

---------

Co-authored-by: Joel Hawksley <[email protected]>

docs/CHANGELOG.md

@@ -28,6 +28,10 @@ nav_order: 5
 
 ## 3.16.0
 
+* Fix development mode race condition that caused an invalid duplicate template error.
+
+    *Blake Williams*
+
 * Add template information to multiple template error messages.
 
     *Joel Hawksley*

lib/view_component/compiler.rb

@@ -30,7 +30,12 @@ def compile(raise_errors: false, force: false)
         @component.superclass.compile(raise_errors: raise_errors)
       end
 
-      return if gather_template_errors(raise_errors).any?
+      if template_errors.present?
+        raise TemplateError.new(template_errors) if raise_errors
+
+        # this return is load bearing, and prevents the component from being considered "compiled?"
+        return false
+      end
 
       if raise_errors
         @component.validate_initialization_parameters!
@@ -98,70 +103,70 @@ def render_template_for(variant = nil, format = nil)
       end
     end
 
-    def gather_template_errors(raise_errors)
-      errors = []
+    def template_errors
+      @_template_errors ||= begin
+        errors = []
 
-      errors << "Couldn't find a template file or inline render method for #{@component}." if @templates.empty?
+        errors << "Couldn't find a template file or inline render method for #{@component}." if @templates.empty?
 
-      # We currently allow components to have both an inline call method and a template for a variant, with the
-      # inline call method overriding the template. We should aim to change this in v4 to instead
-      # raise an error.
-      @templates.reject(&:inline_call?)
-        .map { |template| [template.variant, template.format] }
-        .tally
-        .select { |_, count| count > 1 }
-        .each do |tally|
-        variant, this_format = tally.first
+        # We currently allow components to have both an inline call method and a template for a variant, with the
+        # inline call method overriding the template. We should aim to change this in v4 to instead
+        # raise an error.
+        @templates.reject(&:inline_call?)
+          .map { |template| [template.variant, template.format] }
+          .tally
+          .select { |_, count| count > 1 }
+          .each do |tally|
+          variant, this_format = tally.first
 
-        variant_string = " for variant `#{variant}`" if variant.present?
-
-        errors << "More than one #{this_format.upcase} template found#{variant_string} for #{@component}. "
-      end
+          variant_string = " for variant `#{variant}`" if variant.present?
 
-      default_template_types = @templates.each_with_object(Set.new) do |template, memo|
-        next if template.variant
+          errors << "More than one #{this_format.upcase} template found#{variant_string} for #{@component}. "
+        end
 
-        memo << :template_file if !template.inline_call?
-        memo << :inline_render if template.inline_call? && template.defined_on_self?
+        default_template_types = @templates.each_with_object(Set.new) do |template, memo|
+          next if template.variant
 
-        memo
-      end
+          memo << :template_file if !template.inline_call?
+          memo << :inline_render if template.inline_call? && template.defined_on_self?
 
-      if default_template_types.length > 1
-        errors <<
-          "Template file and inline render method found for #{@component}. " \
-          "There can only be a template file or inline render method per component."
-      end
+          memo
+        end
 
-      # If a template has inline calls, they can conflict with template files the component may use
-      # to render. This attempts to catch and raise that issue before run time. For example,
-      # `def render_mobile` would conflict with a sidecar template of `component.html+mobile.erb`
-      duplicate_template_file_and_inline_call_variants =
-        @templates.reject(&:inline_call?).map(&:variant) &
-        @templates.select { _1.inline_call? && _1.defined_on_self? }.map(&:variant)
-
-      unless duplicate_template_file_and_inline_call_variants.empty?
-        count = duplicate_template_file_and_inline_call_variants.count
-
-        errors <<
-          "Template #{"file".pluralize(count)} and inline render #{"method".pluralize(count)} " \
-          "found for #{"variant".pluralize(count)} " \
-          "#{duplicate_template_file_and_inline_call_variants.map { |v| "'#{v}'" }.to_sentence} " \
-          "in #{@component}. There can only be a template file or inline render method per variant."
-      end
+        if default_template_types.length > 1
+          errors <<
+            "Template file and inline render method found for #{@component}. " \
+            "There can only be a template file or inline render method per component."
+        end
 
-      @templates.select(&:variant).each_with_object(Hash.new { |h, k| h[k] = Set.new }) do |template, memo|
-        memo[template.normalized_variant_name] << template.variant
-        memo
-      end.each do |_, variant_names|
-        next unless variant_names.length > 1
+        # If a template has inline calls, they can conflict with template files the component may use
+        # to render. This attempts to catch and raise that issue before run time. For example,
+        # `def render_mobile` would conflict with a sidecar template of `component.html+mobile.erb`
+        duplicate_template_file_and_inline_call_variants =
+          @templates.reject(&:inline_call?).map(&:variant) &
+          @templates.select { _1.inline_call? && _1.defined_on_self? }.map(&:variant)
+
+        unless duplicate_template_file_and_inline_call_variants.empty?
+          count = duplicate_template_file_and_inline_call_variants.count
+
+          errors <<
+            "Template #{"file".pluralize(count)} and inline render #{"method".pluralize(count)} " \
+            "found for #{"variant".pluralize(count)} " \
+            "#{duplicate_template_file_and_inline_call_variants.map { |v| "'#{v}'" }.to_sentence} " \
+            "in #{@component}. There can only be a template file or inline render method per variant."
+        end
 
-        errors << "Colliding templates #{variant_names.sort.map { |v| "'#{v}'" }.to_sentence} found in #{@component}."
-      end
+        @templates.select(&:variant).each_with_object(Hash.new { |h, k| h[k] = Set.new }) do |template, memo|
+          memo[template.normalized_variant_name] << template.variant
+          memo
+        end.each do |_, variant_names|
+          next unless variant_names.length > 1
 
-      raise TemplateError.new(errors, @templates) if errors.any? && raise_errors
+          errors << "Colliding templates #{variant_names.sort.map { |v| "'#{v}'" }.to_sentence} found in #{@component}."
+        end
 
-      errors
+        errors
+      end
     end
 
     def gather_templates

lib/view_component/errors.rb

@@ -20,12 +20,6 @@ class TemplateError < StandardError
     def initialize(errors, templates = nil)
       message = errors.join("\n")
 
-      if templates
-        message << "\n"
-        message << "Templates:\n"
-        message << templates.map(&:inspect).join("\n")
-      end
-
       super(message)
     end
   end

test/sandbox/test/rendering_test.rb

@@ -512,11 +512,6 @@ def test_raise_error_when_variant_template_file_and_inline_variant_call_exist
         end
       end
 
-    assert_includes(
-      error.message,
-      "ViewComponent::Template:"
-    )
-
     assert_includes(
       error.message,
       "Template file and inline render method found for variant 'phone' in " \