diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 9834fc583..1124d352c 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* + ## 4.15.0 * Add experimental caching support, opt-in per component via `include ViewComponent::ExperimentallyCacheable`. diff --git a/docs/guide/caching.md b/docs/guide/caching.md index 183de010d..400c1adad 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. + ## 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 diff --git a/lib/view_component/cache_digest.rb b/lib/view_component/cache_digest.rb index 98694c8dc..98185cbad 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 @@ -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] synthetic virtual paths def component_paths_in(source) @@ -223,7 +234,16 @@ 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 @@ -231,7 +251,7 @@ def install! 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 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 71b76dd95..4b18bd203 100644 --- a/test/sandbox/config/routes.rb +++ b/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" diff --git a/test/sandbox/test/experimentally_cacheable_integration_test.rb b/test/sandbox/test/experimentally_cacheable_integration_test.rb index 2c08015de..1c7feeefc 100644 --- a/test/sandbox/test/experimentally_cacheable_integration_test.rb +++ b/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", "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 bb0566163..8fa04f937 100644 --- a/test/sandbox/test/experimentally_cacheable_test.rb +++ b/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", + "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", @@ -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)