Track components that haven't opted into cacheability

docs/CHANGELOG.md

@@ -10,6 +10,14 @@ nav_order: 6
 
 ## main
 
+* Track every component in a fragment's render tree when the experimental caching feature is enabled, not only the components that included `ViewComponent::ExperimentallyCacheable`.
+
+    Dependency tracking used to be transitively opt-in: a parent that included the module got a digest covering only the children that also included it. The digest looked complete regardless, and the gap surfaced as stale HTML at an arbitrary later time, whenever an unrelated tracked component happened to change.
+
+    Only the component wrapped in the `<% cache %>` block needs the include now. Applications that never opt in are unaffected, since dependency tracking still short-circuits until the first component registers.
+
+    *Erik Axel Nielsen*
+
 ## 4.15.0
 
 * Add experimental caching support, opt-in per component via `include ViewComponent::ExperimentallyCacheable`.

docs/guide/caching.md

@@ -28,7 +28,7 @@ Editing `PostComponent`'s template, Ruby class, or sidecar files doesn't invalid
 
 ## Opting in
 
-Include `ViewComponent::ExperimentallyCacheable` in each component that should participate in caching:
+Include `ViewComponent::ExperimentallyCacheable` in the component rendered inside the `cache` block:
 
 ```ruby
 class PostComponent < ViewComponent::Base
@@ -42,6 +42,8 @@ end
 
 That's all that's needed for the `<% cache %>` block above to work. The component is registered with Rails' digest tree, and the fragment is invalidated when the component's template, Ruby class, sidecar files, superclasses, child components, or rendered partials change, including components and partials rendered from an inline template or a `#call` method.
 
+Once any component in the application has opted in, the whole render tree is tracked: the child components `PostComponent` renders, and the components *they* render, invalidate the fragment even when they don't include the module.
+
 ## Self-caching
 
 To have a component cache its own output without needing a `cache` block, use `cache_on` to declare methods used for the component's cache key.
@@ -187,7 +189,7 @@ The same works in a template, where the branch is often the more natural place f
 <%= render component.new(post: @post) %>
 ```
 
-Declared components must include `ViewComponent::ExperimentallyCacheable` themselves, since a component that hasn't opted in has no digest to depend on.
+Declared components don't need to include `ViewComponent::ExperimentallyCacheable` themselves. A component that overrides `virtual_path` does, since it's otherwise digested under a path that doesn't lead back to it.
 
 ## Caveats
 

lib/view_component/cache_digest.rb

@@ -21,9 +21,11 @@ module ViewComponent
   #    not just its template.
   #
   # This module fixes both, reusing Rails' own `ActionView::Digestor` rather than
-  # reimplementing static analysis. Components opt in individually by including
-  # `ViewComponent::ExperimentallyCacheable`; until at least one component does,
-  # every hook here short-circuits.
+  # reimplementing static analysis. Until at least one component opts in by
+  # including `ViewComponent::ExperimentallyCacheable`, every hook here
+  # short-circuits. Once one has, every component reachable from a digested
+  # template is tracked, whether or not it included the module: a digest that
+  # covered only part of the render tree would look exactly like a complete one.
   #
   # @private
   module CacheDigest
@@ -90,17 +92,26 @@ def virtual_path_for(component)
 
       # Resolve a synthetic virtual path back to the component that owns it.
       #
+      # Components that opted in are looked up in the registry. Everything else
+      # is derived from the path, which `ViewComponent::Base` builds by
+      # underscoring the class name. The derived constant has to underscore back
+      # to the same path, so a component that overrides `virtual_path` — and
+      # would therefore be digested under a path that isn't its own — is left
+      # unresolved rather than confused with another component.
+      #
       # @return [Class, nil]
       def component_for(virtual_path)
         return unless virtual_path.start_with?("#{VIRTUAL_PATH_PREFIX}/")
 
-        name = registry[virtual_path.delete_prefix("#{VIRTUAL_PATH_PREFIX}/")]
-        return unless name
+        path = virtual_path.delete_prefix("#{VIRTUAL_PATH_PREFIX}/")
+        name = registry[path]
+        return constantize_component(name) if name
 
-        constantize_component(name)
+        component = constantize_component(path.camelize)
+        component if component&.virtual_path == path
       end
 
-      # Scan a template's source for renders of cacheable components.
+      # Scan a template's source for renders of components.
       #
       # Called for every template Rails digests, so it exits early when the
       # feature is unused.
@@ -113,7 +124,7 @@ def dependencies_in(template)
       end
 
       # Scan arbitrary source (a template or a component's Ruby file) for
-      # renders of cacheable components.
+      # renders of components.
       #
       # @return [Array<String>] synthetic virtual paths
       def component_paths_in(source)
@@ -223,15 +234,24 @@ def install!
 
       private
 
-      # Resolve a constant name to a component that opted into caching.
+      # Resolve a constant name to a component.
+      #
+      # Any component counts, not only those that included
+      # `ExperimentallyCacheable`. Tracking only the ones that opted in makes
+      # dependency tracking transitive: a parent's digest covers the children
+      # that happen to have included the module and silently omits the rest,
+      # which is indistinguishable from a complete digest until the untracked
+      # child changes and stale HTML stays on. Applications that never opt in
+      # are unaffected either way, because every hook here short-circuits while
+      # the registry is empty.
       #
       # Returns nil for anything else, including constants that don't exist.
       # Autoloading here is safe: the template is about to render this constant
       # anyway.
       def constantize_component(constant_name)
         component = constant_name.safe_constantize
         return unless component.is_a?(Class)
-        return unless component.respond_to?(:__vc_cacheable?) && component.__vc_cacheable?
+        return unless component < ViewComponent::Base
 
         component
       rescue

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

@@ -0,0 +1 @@
+<div class="cacheable-untracked-parent"><%= render UntrackedChildComponent.new %></div>

test/sandbox/app/components/cacheable_untracked_parent_component.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+# Renders a child that never opted in, so changes to the child must still
+# invalidate the parent.
+class CacheableUntrackedParentComponent < ViewComponent::Base
+  include ViewComponent::ExperimentallyCacheable
+end

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

@@ -0,0 +1 @@
+<span class="untracked-child">untracked</span>

test/sandbox/app/components/untracked_child_component.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+
+# Deliberately does not include `ViewComponent::ExperimentallyCacheable`, so
+# nothing registers it with the digest tree.
+class UntrackedChildComponent < ViewComponent::Base
+end

test/sandbox/app/views/integration_examples/cached_untracked_component.html.erb

@@ -0,0 +1,4 @@
+<% cache "cached-untracked-component-fragment" do %>
+  <%= render CacheableComponent.new(title: "cached") %>
+  <%= render UntrackedChildComponent.new %>
+<% end %>

test/sandbox/config/routes.rb

@@ -28,6 +28,7 @@
   get :cached_partial, to: "integration_examples#cached_partial"
   get :cached_component, to: "integration_examples#cached_component"
   get :cached_nested_component, to: "integration_examples#cached_nested_component"
+  get :cached_untracked_component, to: "integration_examples#cached_untracked_component"
   get :inherited_sidecar, to: "integration_examples#inherited_sidecar"
   get :inherited_from_uncompilable_component, to: "integration_examples#inherited_from_uncompilable_component"
   get :unsafe_component, to: "integration_examples#unsafe_component"

test/sandbox/test/experimentally_cacheable_integration_test.rb

@@ -65,6 +65,36 @@ def test_cache_block_is_invalidated_when_a_nested_component_changes
     end
   end
 
+  # The failure this guards against is silent: the digest looks complete while
+  # covering only the children that happened to opt in, so an edit to an
+  # untracked one sits invisible until an unrelated tracked component changes.
+  def test_cache_block_is_invalidated_when_an_untracked_component_changes
+    get "/cached_untracked_component"
+    assert_select(".untracked-child", text: "untracked")
+
+    before = fragment_digest_for("integration_examples/cached_untracked_component")
+
+    modify_file "app/components/untracked_child_component.html.erb", "<span class=\"untracked-child\">changed</span>\n" do
+      clear_digest_cache
+
+      refute_equal before, fragment_digest_for("integration_examples/cached_untracked_component")
+    end
+  end
+
+  def test_cached_markup_of_an_untracked_component_is_not_served_stale
+    get "/cached_untracked_component"
+    assert_select(".untracked-child", text: "untracked")
+
+    modify_file "app/components/untracked_child_component.html.erb", "<span class=\"untracked-child\">changed</span>\n" do
+      clear_digest_cache
+      with_new_cache do
+        get "/cached_untracked_component"
+
+        assert_select(".untracked-child", text: "changed")
+      end
+    end
+  end
+
   def test_cache_block_digest_is_unaffected_by_unrelated_components
     before = fragment_digest_for("integration_examples/cached_component")
 

test/sandbox/test/experimentally_cacheable_test.rb

@@ -73,6 +73,24 @@ def test_cache_digest_changes_when_a_child_component_ruby_file_changes
     ) { CacheableParentComponent.cache_digest }
   end
 
+  # A child that never included `ExperimentallyCacheable` is still part of what
+  # the parent renders, so it has to be part of the parent's digest.
+  def test_cache_digest_changes_when_an_untracked_child_component_template_changes
+    assert_digest_changes(
+      "app/components/untracked_child_component.html.erb",
+      "<span class=\"untracked-child\">changed</span>\n"
+    ) { CacheableUntrackedParentComponent.cache_digest }
+  end
+
+  def test_cache_digest_changes_when_an_untracked_child_component_ruby_file_changes
+    original = File.read(Rails.root.join("app/components/untracked_child_component.rb"))
+
+    assert_digest_changes(
+      "app/components/untracked_child_component.rb",
+      original + "\n# a comment\n"
+    ) { CacheableUntrackedParentComponent.cache_digest }
+  end
+
   def test_cache_digest_changes_when_a_superclass_template_changes
     assert_digest_changes(
       "app/components/cacheable_component.html.erb",
@@ -121,9 +139,9 @@ def test_declared_template_paths_are_left_alone
     assert_includes dependencies, "integration_examples/erb_partial"
   end
 
-  def test_declared_names_that_are_not_cacheable_components_are_left_alone
+  def test_declared_names_that_are_not_components_are_left_alone
     assert_empty ViewComponent::CacheDigest.explicit_component_dependencies(
-      "# Template Dependency: ErbComponent"
+      "# Template Dependency: IntegrationExamplesController"
     )
     assert_empty ViewComponent::CacheDigest.explicit_component_dependencies("no declarations here")
   end
@@ -407,10 +425,29 @@ def test_component_for_ignores_paths_outside_the_prefix
     assert_nil ViewComponent::CacheDigest.component_for("integration_examples/cached_component")
   end
 
-  def test_component_for_returns_nil_for_unregistered_paths
+  def test_component_for_returns_nil_for_paths_that_name_no_component
     assert_nil ViewComponent::CacheDigest.component_for("view_component/cache_digest/nope")
   end
 
+  # Unregistered components aren't in the registry to look up, so they're
+  # resolved by reversing the underscoring `ViewComponent::Base` applies.
+  def test_component_for_resolves_a_component_that_did_not_opt_in
+    refute_includes ViewComponent::CacheDigest.registry, "erb_component"
+
+    assert_equal ErbComponent, ViewComponent::CacheDigest.component_for("view_component/cache_digest/erb_component")
+  end
+
+  # Deriving the constant from the path only holds while the path is the
+  # underscored class name. A component that moved its own path is digested
+  # somewhere its name doesn't lead, so it has to register to be found.
+  def test_component_for_ignores_a_component_that_moved_its_virtual_path
+    Object.const_set(:MovedPathComponent, Class.new(ViewComponent::Base) { self.virtual_path = "somewhere/else" })
+
+    assert_nil ViewComponent::CacheDigest.component_for("view_component/cache_digest/moved_path_component")
+  ensure
+    Object.send(:remove_const, :MovedPathComponent)
+  end
+
   def test_dependencies_are_not_scanned_for_sources_without_render
     assert_empty ViewComponent::CacheDigest.dependencies_in(build_template("no calls here"))
   end
@@ -422,8 +459,19 @@ def test_dependencies_are_found_for_component_renders
     )
   end
 
-  def test_dependencies_ignore_components_that_did_not_opt_in
-    assert_empty ViewComponent::CacheDigest.dependencies_in(build_template("<%= render ErbComponent.new(message: 'a') %>"))
+  # Tracking only the components that opted in would make the digest silently
+  # partial: it would look complete while omitting every child that hasn't.
+  def test_dependencies_are_found_for_components_that_did_not_opt_in
+    assert_equal(
+      ["view_component/cache_digest/erb_component"],
+      ViewComponent::CacheDigest.dependencies_in(build_template("<%= render ErbComponent.new(message: 'a') %>"))
+    )
+  end
+
+  def test_dependencies_ignore_constants_that_are_not_components
+    assert_empty(
+      ViewComponent::CacheDigest.dependencies_in(build_template("<%= render IntegrationExamplesController %>"))
+    )
   end
 
   def test_resolver_is_identified_by_class
@@ -455,7 +503,7 @@ def test_dependency_tracking_falls_back_when_scanning_fails
 
   def test_constantizing_swallows_unexpected_errors
     Object.const_set(:BoomComponent, Class.new do
-      def self.__vc_cacheable?
+      def self.<(other)
         raise ArgumentError
       end
     end)