Use single @output_buffer when rendering

[BlakeWilliams]

When rendering components we sometimes run into incompatibilities with
Rails due to Rails expecting a single @output_buffer when rendering
templates. This isn't true when using view components due to each
component having its own @output_buffer.

The most obvious issue I've run into due us not having a single
@output_buffer can be recreated by repeating the following:

1. Create a form using form_for
2. Pass the f variable to a component
3. Call f.label while passing it a block

This will cause the block content to be rendered incorrectly, usually
above the resulting <label> tag.

The tl;dr of that behavior is that form_for captures the
view_context of the template it's called in. When trying to evaluate
the block passed to f.label it calls view_context.capture but the
block was defined and is run in the context of the component, not
view_context! This causes the block content to be rendered in the
component's @output_buffer (since it's not capturing).

This attempts to resolve the issue by tracking and using a single
@output_buffer. When a component is rendered we find the top-level
ActionView::Base instance in the render hierarchy and use its
@output_buffer. We then tell the top-level renderer that we are one of
its children.

Now, when any ActionView::Base instance in the rendering hierarchy
changes its @output_buffer, it propagates that change to all
ActionView::Base instances. This matches Rails' expectations and
results in the previously broken behavior working as-expected.

[BlakeWilliams]

This change isn't great performance wise. I'm hoping to find some time to try optimizing it.

Warming up --------------------------------------
          component:     1.594k i/100ms
            partial:   930.000  i/100ms
Calculating -------------------------------------
          component:     15.495k (± 5.2%) i/s -    154.618k in  10.005081s
            partial:      8.729k (± 5.3%) i/s -     87.420k in  10.043416s

Comparison:
          component::    15494.9 i/s
            partial::     8729.3 i/s - 1.78x  (± 0.00) slower

[BlakeWilliams]

Not requiring HAML/Slim seemed to increase the benchmark a bit.

view_component ❯ be rake benchmark
  * development - set it to false
  * test - set it to false (unless you use a tool that preloads your test environment)
  * production - set it to true

Warming up --------------------------------------
          component:     2.491k i/100ms
            partial:   890.000  i/100ms
Calculating -------------------------------------
          component:     25.042k (± 2.4%) i/s -    251.591k in  10.052708s
            partial:      9.043k (± 2.5%) i/s -     90.780k in  10.045240s

Comparison:
          component::    25042.5 i/s
            partial::     9043.0 i/s - 2.77x  (± 0.00) slower

[BlakeWilliams]

Looks like the change I made to the benchmark actually decreased the numbers too.

Here's main with the performance/benchmark.rb changes applied:

view_component ❯ be rake benchmark
Warming up --------------------------------------
          component:     3.265k i/100ms
            partial:   987.000  i/100ms
Calculating -------------------------------------
          component:     34.536k (± 3.6%) i/s -    346.090k in  10.035493s
            partial:     10.089k (± 5.2%) i/s -    100.674k in  10.010669s

Comparison:
          component::    34536.1 i/s
            partial::    10088.7 i/s - 3.42x  (± 0.00) slower

[BlakeWilliams]

Even more benchmarks:

Main

view_component ❯ be rake benchmark
Warming up --------------------------------------
          component:    11.677k i/100ms
            partial:     1.359k i/100ms
Calculating -------------------------------------
          component:    105.925k (± 9.6%) i/s -      1.051M in  10.026516s
            partial:     13.237k (± 6.5%) i/s -    131.823k in  10.003570s

Comparison:
          component::   105924.7 i/s
            partial::    13236.9 i/s - 8.00x  (± 0.00) slower

This branch

view_component ❯ be rake benchmark
Warming up --------------------------------------
          component:     5.265k i/100ms
            partial:     1.377k i/100ms
Calculating -------------------------------------
          component:     56.134k (± 4.8%) i/s -    563.355k in  10.061795s
            partial:     13.015k (± 3.8%) i/s -    130.815k in  10.066347s

Comparison:
          component::    56133.6 i/s
            partial::    13015.2 i/s - 4.31x  (± 0.00) slower

[23tux]

@BlakeWilliams Is there any news on this?

[BlakeWilliams]

@23tux Hi! There is. This mostly works, but there is an edge case I'm seeing in production applications that I haven't had time to track down.

It would be super helpful to have a failing test case for that scenario or other scenarios production apps might run into. If anyone would like to try to run this locally and write a failing test case, that would help me move forward with this.

[nicolas-brousse]

👋 @BlakeWilliams

I've the same or similar issue with capture used in components, by using some form helpers.
My latest case was with label with a block.

I'm busy this week, but I'll try to found time to write some failing test case.

I've tested your branch on our project and solve the issue btw 👍

[percyhanna]

I recently ran into the same problem, or my assumption is that it is the same root problem anyway. I was trying to call some vanilla Rails helpers from my view component with similar results. My pseudo-code looked like this:

<%= helpers.my_rails_helper do %>
  <h1>Content inside ViewComponent</h1>
<% end %>

A simplified version of the my_rails_helper helper method looked like:

def my_rails_helper
  contents = capture(&block)

  # run conditional logic based on captured contents
  # ...

  # main output
  content_tag(:div, contents, id: "my-id", ...)
end

The net result was that the captured content (the <h1>) would be rendered before the <div> tag that is returned by my_rails_helper, something like:

  <h1>Content inside ViewComponent</h1>
<div id="my-id"></div>

I tried various approaches to try and fix this, and was able to get the code below to work. It's entirely possible that this is a bad idea, but I wanted to share in case it provided additional insight into the matter. I added these methods to the ViewComponent class, which allowed seemingly transparent access to the normal Rails helpers.

def respond_to_missing?(method, include_all)
  helpers.respond_to?(method, include_all) || super
end

def method_missing(method, *args, &block)
  if helpers.respond_to?(method)
    helpers.method(method).unbind.bind(self).call(*args, &block)
  else
    super
  end
end

[tomclose]

Should this be FormWithComponent here?

[mhw]

I have a failing test case that I think could be caused by the issue you're trying to fix here, added as the commit mhw@58dacd7

I'm trying to bring some partials across to ViewComponent, and this test replicates an issue I ran into with a partial that uses turbo_frame_tag. I think it may also be the cause of #1099. The nesting of the elements gets mixed up, as seems to be the case in other failures, but I'm also seeing HTML markup getting quoted in the output. The generated markup from the test case is currently

<span>Hello</span>
<div id="container">&lt;/span&gt;
</div>

while I think it should be

<div id="container">
<span>Hello</span>
</div>

That's assuming I've not made a mistake in the use of ViewComponent.

[mhw]

I've been doing a bit more digging on this one and have a branch (here) that takes the work here and addresses a couple of additional issues to give something that's working for me in an app where I'm trying to convert a bunch of nested partials in to ViewComponents.

I think the basic approach here is sound: the Rails capture helper (and helpers that use it, like tag, form_with, form_for, etc.) rely on being able to temporarily overwrite the ActionView::Base @output_buffer to redirect the output from compiled templates. As ViewComponent::Base doesn't share this @output_buffer, evaluating a block from a template in one context within a template in a different one can result in content going to the wrong buffer. So we need to enlist the output buffers of all the view contexts involved in rendering a view.

The one issue I have run into is with Haml: Haml 5 also patches ActionView::Base output_buffer=, and this doesn't work nicely with the ActionViewCompatibility module in this branch. I've taken a quick look at this, but Haml's buffer handling and how it makes its helpers available in Haml templates looks a bit tricky to handle correctly. The good news is that Haml 6 is based on hamlit, and the ActionView::Base patches are gone. In my (brief) tests the combination of the branch linked above and hamlit correctly handles cases like passing a FormBuilder between view components.

[Spone]

@BlakeWilliams can you rebase this? What do you think about releasing this change behind a config setting, marked as experimental, so it can be tested easily on real-world apps?

My current use case is rendering view components in other view components, within a form managed by a custom FormBuilder (that's what we are experimenting with over at view_component-form). With the latest view_component, form inputs get rendered twice. If I use the branch from this PR, the problem is solved.

[BlakeWilliams]

@Spone I can write a TODO to rebase it, but as-is it's not working in a production application I've tried it on. There's something about the new behavior that breaks existing behavior in apps.

Unfortunately I don't have a ton of time to dedicate to the issue so it's difficult to consistently work on the problem since it requires a good amount of context to properly understand.

[Spone]

There's something about the new behavior that breaks existing behavior in apps.

If you can elaborate a bit on this, when you have time. I can dedicate some time to try and replicate the issue in our production apps, if I know what to look for.

[mhw]

@Spone You might want to look at the branch I'm working on: I've rebased it a couple of times so it is not too far behind main.

I also discovered a further problem with the implementation in this PR, which might be the same issue @BlakeWilliams mentions. The code in action_view_compatibility.rb tries to keep @output_buffer in sync across all the subclasses of ActionView::Base that are enlisted in rendering. with_output_buffer works by storing the previous output buffer on the stack and then restoring it before returning:

[...]
  self.output_buffer, old_buffer = buf, output_buffer
  yield
  output_buffer
ensure
  self.output_buffer = old_buffer
end

Broadly I think the issue is that old_buffer is not always restored correctly in all the subclasses involved in rendering. I'm not 100% clear on why, but I think it's partly because we're restoring a single value of old_buffer across all the ActionView::Base subclasses, even if all the subclasses were not sharing a single buffer when with_output_buffer was called. That seems to be possible because there's code that updates @output_buffer directly rather than going through the attribute writer - notably ActionView::Base#_run, which is called when rendering a partial. I reduced one instance of the issue to a test case here.

The approach I took to addressing this is to redefine with_output_buffer so that it handles both saving and restoring the output buffer across all ActionView::Base subclasses. It's basically the same approach, but lifted to with_output_buffer instead of output_buffer=.

In the app where I'm introducing View Component I am planning to continue converting a set of partials in to components to give me more confidence that the solution I've got is actually correct. I've tried it on #1227 because with_output_buffer looked likely to be the root cause, and the test there passes with my branch. I'd be interested if more testing in other places where this PR currently doesn't behave correctly.

I've also been meaning to spend a bit more time on the solution to see if there's a more efficient approach, but I've been working on other things recently.

[BlakeWilliams]

I just pushed up the results on what I've been hacking on since Sunday. I originally took a different approach, trying to use a proxy object that manages a buffer and is shared across ActionView::Base instances. Unfortunately there's a lot of issues with that approach and I didn't get as far in a real app as I did with the subscriber model.

I took another try at the subscriber model this morning and ran into the issue I mentioned earlier, but thanks to @mhw (seriously, great find!) pointing out that _run sets the buffer explicitly I was able to get problematic pages loading correctly! 🎉

See https://github.com/github/view_component/pull/974/files#diff-83ab3a050e037c39b32215700aff22c51283cf9e35710d0335c2bae28b5a81aeR35-R45 for what fixed the issue.

I think that could make a great upstream change for Rails, and any other place we explicitly set @output_buffer if anyone would like to try submitting a patch. It seems pretty low-risk so I think it may have a good chance of being accepted.

HAML is still broken, but that's somewhat expected. I can try to hack on this a bit more to see if I can gate it behind a flag and run some benchmarks to see if this is great/bad.

I'm really curious about how this could be upstreamed for Rails as some kind of generic buffer manager so we can remove the need for patches or a global buffer pub/sub object and if they'd accept a patch.

[BlakeWilliams]

Performance took a hit because of all the extra capture calls required to render things properly, but we're still faster than partials:

Warming up --------------------------------------
          component:    63.000  i/100ms
             inline:    69.000  i/100ms
            partial:    38.000  i/100ms
Calculating -------------------------------------
          component:    600.088  (±10.5%) i/s -      5.985k in  10.101153s
             inline:    572.589  (±12.4%) i/s -      5.658k in  10.043720s
            partial:    359.631  (± 8.9%) i/s -      3.572k in  10.018721s

Comparison:
          component::      600.1 i/s
             inline::      572.6 i/s - same-ish: difference falls within error
            partial::      359.6 i/s - 1.67x  (± 0.00) slower

[mhw]

Just wanted to confirm that your revised branch works great in the app I'm working on where I'd run in to issues previously. It's pretty simple, but I'm at the stage of switching partials to view components and hence was running in to these buffer handling issues.

I'd thought about what change in Rails might make this easier to do, and the simplest thing I'd come up with was to extend ActionView::OutputBuffer with a couple of methods to mark a position in the buffer and then take back any content that had been output. My thinking was to stop @output_buffer changing at all through the whole render - if it always points to the same object there should be no issue with it being shared. So the kind of pattern in with_output_buffer would become something like

  output_buffer.mark
  yield
ensure
  return output_buffer.take
end

That would be a breaking change though, at least with respect to any other gems that work by updating @output_buffer (e.g. temple and anything built on it). I'd also suspect it's likely to perform worse.

The other thought I'd had was whether anything could be inferred from the binding of the block being yielded to. Passing a block from one view context to be evaluated in a different view context seems to be the issue, as the block being passed will still be outputting to the original view context's buffer. I wondered if looking at block.binding.receiver could limit the set of view contexts that need to be kept in sync, but I wasn't convinced there wouldn't be holes in that approach and haven't had time to try it out.

[stevegeek]

Thanks for all the work on this!

I have just tried this branch and solves the problem for me 🎉 . In terms of performance in my unscientific tests, Im seeing ~26-28% slower rendering times compared to 2.49.0

2.49.0
-------

                                   user     system      total        real
Render Avatar                      0.000251   0.000007   0.000258 (  0.000251)

                                   user     system      total        real
Render Avatar                      0.000365   0.000014   0.000379 (  0.000366)

                                   user     system      total        real
Render Avatar                      0.000312   0.000012   0.000324 (  0.000313)



This branch
------------
                                   user     system      total        real
Render Avatar                      0.000333   0.000007   0.000340 (  0.000332)

                                   user     system      total        real
Render Avatar                      0.000434   0.000011   0.000445 (  0.000432)

                                   user     system      total        real
Render Avatar                      0.000437   0.000013   0.000450 (  0.000442)

[BlakeWilliams]

I have just tried this branch and solves the problem for me 🎉 . In terms of performance in my unscientific tests, Im seeing ~26-28% slower rendering times compared to 2.49.0

Thanks for sharing that! I think that's an expected result of this change, at least as-is. In order to have consistent rendering within the Rails @output_buffer context we need to capture each component render which is a bit costly.

[Spone]

I'm wondering if there could be a way to minimize the overall performance penalty by only applying this behavior (capturing the component render) when it is needed. Maybe some kind of flag to declare manually on the component class, when the component needs to reuse the existing @output_buffer?

In my current app, most components are currently working fine and don't need this, so I could apply the new behavior only to components interacting with forms.

[camertron]

Hey everyone! Over the last couple of days I've been hacking on @BlakeWilliams' original work in an attempt to implement the single output buffer concept using the proxy model he mentioned instead of the subscriber model. So far, things look very promising. I've been able to get the performance hit down to about 18%, which should be almost entirely mitigated by this companion performance PR. I as able to verify the changes in a large production application (github.com) and everything seems to be working. I would be grateful however if others could give it a spin! Please let me know if you run into anything weird :)

/cc @Spone @stevegeek @mhw @percyhanna @nicolas-brousse @23tux

[BlakeWilliams]

replaced with #1650, which is likely getting merged soon.