Skip to content

Commit

Permalink
Consistently evaluate passed in block content (#1407)
Browse files Browse the repository at this point in the history
* 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 <joel@hawksley.org>
  • Loading branch information
BlakeWilliams and joelhawksley authored Jul 18, 2022
1 parent 6efde7b commit 4497c3e
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 2 deletions.
4 changes: 4 additions & 0 deletions docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions lib/view_component/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
""
Expand Down
17 changes: 17 additions & 0 deletions test/sandbox/app/components/composable_slots_component.rb
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
10 changes: 9 additions & 1 deletion test/sandbox/test/slotable_v2_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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!")
Expand Down Expand Up @@ -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

0 comments on commit 4497c3e

Please sign in to comment.