diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 36f2da188..4969310d6 100644 --- a/docs/CHANGELOG.md +++ b/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* + * Invalidate Action View's memoized template digests when a component registers with `ViewComponent::CacheDigest`, so a digest computed before the component loaded isn't served for the rest of the process. *Erik Axel Nielsen* diff --git a/docs/guide/caching.md b/docs/guide/caching.md index 5cbcfe3f0..509fde855 100644 --- a/docs/guide/caching.md +++ b/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. + ## Caching inside a component template A `<% cache %>` block written inside a component's own template has the same problem, for the same reason: Rails digests the template that's rendering, and a component's template isn't in the view paths, so there's nothing to digest. @@ -202,7 +204,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. ## When a digest can't be computed diff --git a/lib/view_component/cache_digest.rb b/lib/view_component/cache_digest.rb index 4db6bc48c..b57c2c0be 100644 --- a/lib/view_component/cache_digest.rb +++ b/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 @@ -98,17 +100,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. @@ -121,7 +132,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] synthetic virtual paths def component_paths_in(source) @@ -235,7 +246,16 @@ def expire_digests ActionView::LookupContext::DetailsKey.digest_caches.each(&:clear) end - # 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 @@ -243,7 +263,7 @@ def expire_digests 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 end diff --git a/test/sandbox/app/components/cacheable_untracked_parent_component.html.erb b/test/sandbox/app/components/cacheable_untracked_parent_component.html.erb new file mode 100644 index 000000000..6898f8e50 --- /dev/null +++ b/test/sandbox/app/components/cacheable_untracked_parent_component.html.erb @@ -0,0 +1 @@ +
<%= render UntrackedChildComponent.new %>
diff --git a/test/sandbox/app/components/cacheable_untracked_parent_component.rb b/test/sandbox/app/components/cacheable_untracked_parent_component.rb new file mode 100644 index 000000000..c3ee32981 --- /dev/null +++ b/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 diff --git a/test/sandbox/app/components/untracked_child_component.html.erb b/test/sandbox/app/components/untracked_child_component.html.erb new file mode 100644 index 000000000..f152f206b --- /dev/null +++ b/test/sandbox/app/components/untracked_child_component.html.erb @@ -0,0 +1 @@ +untracked diff --git a/test/sandbox/app/components/untracked_child_component.rb b/test/sandbox/app/components/untracked_child_component.rb new file mode 100644 index 000000000..63246faaa --- /dev/null +++ b/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 diff --git a/test/sandbox/app/views/integration_examples/cached_untracked_component.html.erb b/test/sandbox/app/views/integration_examples/cached_untracked_component.html.erb new file mode 100644 index 000000000..eacbad1b0 --- /dev/null +++ b/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 %> diff --git a/test/sandbox/config/routes.rb b/test/sandbox/config/routes.rb index c29f6e4b3..cb88a3f82 100644 --- a/test/sandbox/config/routes.rb +++ b/test/sandbox/config/routes.rb @@ -29,6 +29,7 @@ get :cached_component, to: "integration_examples#cached_component" get :cached_nested_component, to: "integration_examples#cached_nested_component" get :cache_block_component, to: "integration_examples#cache_block_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" diff --git a/test/sandbox/test/experimentally_cacheable_integration_test.rb b/test/sandbox/test/experimentally_cacheable_integration_test.rb index b1d152248..2e515d57e 100644 --- a/test/sandbox/test/experimentally_cacheable_integration_test.rb +++ b/test/sandbox/test/experimentally_cacheable_integration_test.rb @@ -66,6 +66,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", "changed\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", "changed\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") diff --git a/test/sandbox/test/experimentally_cacheable_test.rb b/test/sandbox/test/experimentally_cacheable_test.rb index ebf6cd10c..a034ddeed 100644 --- a/test/sandbox/test/experimentally_cacheable_test.rb +++ b/test/sandbox/test/experimentally_cacheable_test.rb @@ -117,6 +117,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", + "changed\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", @@ -165,9 +183,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 @@ -490,10 +508,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 @@ -505,8 +542,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