From 4497c3ebc121e8847a402ff7110b77fac9b72e9d Mon Sep 17 00:00:00 2001 From: Blake Williams Date: Mon, 18 Jul 2022 12:57:42 -0400 Subject: [PATCH] Consistently evaluate passed in block content (#1407) * Consistently evaluate passed in block content Currently slots aren't composable due to the lifecycle of components, where we attempt to avoid evaluating the block passed to `render_in` (aka `content` in the library). This creates an inconsistent experience where delegation works only if `content` was already evaluated. The following code looks correct, but the `c.title` code will never be executed: ```ruby class MyComponent delegate :title, to: :@sidebar def initialize @sidebar = SidebarComponent.new end def call content_tag :div do render @sidebar end end end ``` ```erb <%= render MyComponent.new(:title) do |c| %> <%= c.title("My sidebar") %> <% end %> ``` The block passed that includes the call to `c.title("My sidebar")` is never evaluated. This is a side-effect of a performance optimization where we attempt to lazily evaluate the block passed to `render_in`. This was primarily evaluating the entire render chain for components return `false` when `render?` is called since it's unnecessary work. If the component were slightly different and called `content` for any reason the above code would work as expected: ```ruby class MyComponent delegate :title, to: :@sidebar def initialize @sidebar = SidebarComponent.new end def call # this causes `content` to be evaluated, resulting in # <%= c.title(title: "My sidebar") %> being called. Now this # component will work as expected. modified_content = content.replace('foo', 'bar') content_tag :div do render @sidebar end end end ``` This is an example of the optimization causing inconsistent behavior in the library. The change above causes `content` to be called before `#call`, so any setup/mutations/side-effects are now guaranteed to be reflected in the component output. Here's what the change in the component lifecycle looks like: ``` \#before_render -> #render? -> #call (#content can be called anywhere in the above chain, or never called) ``` Now the lifecycle looks like: ``` \#before_render -> #render? -> #content -> #call (#content isn't used, it's just evaluated to ensure side-effects are performed) ``` * Remove title kwarg * Add changelog entry * Add caveat to changelog entry Co-authored-by: Joel Hawksley --- docs/CHANGELOG.md | 4 ++++ lib/view_component/base.rb | 4 ++++ .../components/composable_slots_component.rb | 17 +++++++++++++++++ .../slots_v2_without_content_block_component.rb | 2 +- test/sandbox/test/slotable_v2_test.rb | 10 +++++++++- 5 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 test/sandbox/app/components/composable_slots_component.rb diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index b8406e072..efcaaed1d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,6 +9,10 @@ title: Changelog ## main +* Ensure side-effects in `content` are consistently evaluated before components are rendered. This change effectively means that `content` is evaluated for every component render where `render?` returns true. As a result, code that is passed to a component via a block/content will now always be evaluated, before `#call`, which can reveal bugs in existing components. + + *Blake Williams* + ## 2.60.0 * Add support for `render_preview` in RSpec tests. diff --git a/lib/view_component/base.rb b/lib/view_component/base.rb index d5b6e6f6a..a2821fa06 100644 --- a/lib/view_component/base.rb +++ b/lib/view_component/base.rb @@ -124,6 +124,10 @@ def render_in(view_context, &block) before_render if render? + # preload content to support slot delegation and to consistently perform + # side-effects that may exist in the block + content + perform_render else "" diff --git a/test/sandbox/app/components/composable_slots_component.rb b/test/sandbox/app/components/composable_slots_component.rb new file mode 100644 index 000000000..81acfcc39 --- /dev/null +++ b/test/sandbox/app/components/composable_slots_component.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class ComposableSlotsComponent < ViewComponent::Base + delegate :title, to: :@parent + + def initialize + @parent = SlotsV2WithoutContentBlockComponent.new + end + + def call + content_tag :div, class: "composable-slot-component" do + capture do + render @parent + end + end + end +end diff --git a/test/sandbox/app/components/slots_v2_without_content_block_component.rb b/test/sandbox/app/components/slots_v2_without_content_block_component.rb index c29fd9cc4..fb95ff656 100644 --- a/test/sandbox/app/components/slots_v2_without_content_block_component.rb +++ b/test/sandbox/app/components/slots_v2_without_content_block_component.rb @@ -4,7 +4,7 @@ class SlotsV2WithoutContentBlockComponent < ViewComponent::Base renders_one :title, "MyTitleComponent" class MyTitleComponent < ViewComponent::Base - def initialize(title:) + def initialize(title) @title = title end diff --git a/test/sandbox/test/slotable_v2_test.rb b/test/sandbox/test/slotable_v2_test.rb index 50c0f6ed1..951deeaee 100644 --- a/test/sandbox/test/slotable_v2_test.rb +++ b/test/sandbox/test/slotable_v2_test.rb @@ -271,7 +271,7 @@ def test_slots_accessible_in_render_predicate def test_slots_without_render_block render_inline(SlotsV2WithoutContentBlockComponent.new) do |component| - component.title(title: "This is my title!") + component.title("This is my title!") end assert_selector("h1", text: "This is my title!") @@ -579,4 +579,12 @@ def test_lambda_slot_content_can_be_provided_via_a_block assert_selector("h1.some-class", text: "This is a header!") end + + def test_composable_slots + render_inline ComposableSlotsComponent.new do |c| + c.title("The truth is out there") + end + + assert_selector("div h1", text: "The truth is out there") + end end