Use a global output buffer

[camertron]

Summary

The Problem

Since the beginning, ViewComponent has striven for deep integration with ActionView. One of the known issues however has been using Rails' form helpers with components. Consider this template for the LabelComponent:

<div>
  <%= form.label :published do %>
    <%= form.check_box :published %>
  <% end %>
</div>

Now let's render it from the FormForComponent:

<%= form_for Post.new, url: "/" do |form| %>
  <%= render LabelComponent.new(form: form) %>
<% end %>

The latest version of ViewComponent produces the following HTML:

<form class="new_post" id="new_post" action="/" accept-charset="UTF-8" method="post">
  <input name="utf8" type="hidden" value="&#x2713;" autocomplete="off" />
  <div>
    <input name="post[published]" type="hidden" value="0" autocomplete="off" />
    <input type="checkbox" value="1" name="post[published]" id="post_published" />
    <label for="post_published"></label>
  </div>
</form>

Notice that the <label> tag appears below the inputs even though it's supposed to appear above them. This happens because components render to their own output buffer while the form helpers (and indeed the rest of ActionView) render to another one.

Potential Solution

The solution proposed in this PR is to use a single, global output buffer. For now it continues to make sense for ViewComponents to render into their own buffer (mostly because they inherit from ActionView::Base). Accordingly I've introduced a data structure called the OutputBufferStack that is used in place of the usual ActionView::OutputBuffer. Components push new OutputBuffers onto the stack whenever they are rendered. The buffer at the top of the stack is the one all ActionView::Bases write to, making it appear as if there is only one, global buffer. Instances of OutputBufferStack behave exactly like OutputBuffer instances, meaning they can be swapped in and out without any changes to ActionView. Moreover, only component classes need this behavior, so no Rails monkeypatches are required.

The global output buffer behavior is opt-in and available either by manually prepending the ViewComponent::GlobalOutputBuffer module into individual component classes, or globally by setting the config.view_component.use_global_output_buffer Rails configuration setting.

With the global output buffer enabled, the following HTML is produced for the example above:

<form class="new_post" id="new_post" action="/" accept-charset="UTF-8" method="post">
  <input name="utf8" type="hidden" value="&#x2713;" autocomplete="off" />
  <div>
    <label for="post_published">
      <input name="post[published]" type="hidden" value="0" autocomplete="off" />
      <input type="checkbox" value="1" name="post[published]" id="post_published" />
    </label>
  </div>
</form>

It works! 🎉

Other Information

Unfortunately managing the global output buffer adds some overhead. My benchmarks show that it decreases performance by about 18%. While not ideal, I've tried to mitigate the performance hit by submitting this PR in tandem, which increases overall performance by about 15%. Here are the raw numbers for just this PR (run bundle exec ruby performance/global_output_buffer_benchmark.rb):

Warming up --------------------------------------
           component   239.000  i/100ms
  component with GOB   242.000  i/100ms
Calculating -------------------------------------
           component      2.820k (± 4.8%) i/s -     28.202k in  10.026443s
  component with GOB      2.339k (± 3.9%) i/s -     23.474k in  10.050467s

Comparison:
           component:     2819.8 i/s
  component with GOB:     2339.5 i/s - 1.21x  (± 0.00) slower

We hope to fully integrate the global output buffer in ViewComponent v3, which will mean merging all the code from ViewComponent::GlobalOutputBuffer into ViewComponent::Base. Doing so should result in fewer method calls and get us back that remaining 3% of performance.

[BlakeWilliams]

I left a few comments, but otherwise this LGTM!

[github-actions]

1 file(s) had their final line ending fixed:

• test/sandbox/app/views/integration_examples/_erb_partial.html.erb

[percyhanna]

Unfortunately, this branch does not work with our use case, while the original PR, #974, does work fine. I have a simple test case in our application that was used to test the "fixed" behaviour to ensure the ViewComponent's would render correctly. This test case can be reduced to this simple reproduction:

# A helper method in our application
def my_helper_method(options, &block)
  content_tag(:dig, options, &block)
end

<%# Inside a ViewComponent %>
<%= my_helper_method({ foo: :bar }) do %>
  <p>Content inside helper method</p>
<% end %>

Unfortunately, the content does not render properly with this branch:

       expected: "<div> <p>Content inside helper method</p> </div>"
            got: "<p>Content inside helper method</p> <div> &lt;p&gt;Content inside helper method&lt;/p&gt; </div>"

[camertron]

Thanks for reporting this @percyhanna :)

The problem you're running into is caused by the fact that view components are their own instances of ActionView::Base. In order to access helpers using the helpers method, instances of ViewComponent::Base hold on to a reference to the calling view context (a separate instance of ActionView::Base), which has all the helper modules included. This means that any helper method that calls #capture (as #content_tag does) will do so in the original view context and not in the view context of the component. PR #974 avoids this problem by monkeypatching Action View so that the output buffer is truly global, i.e. is shared by both Action View and all view components. In this PR, I've scoped the changes so they only affect view components. The buffer is global in the sense that it is shared between the first component in the render hierarchy and any other components rendered within it, but local in the sense that it is not shared with the calling view context provided by Action View. Perhaps I should have titled the PR "Use a global-ish output buffer."

View components have always had a sort of weird relationship with helpers. Normally, all the helpers in your app are (as far as I know) automatically included into the module that houses your compiled templates and therefore directly available inside your views. Historically however view components have favored defining methods on component classes themselves instead of relying on the application-wide grab bag of helper methods, which can not only override each other in unintuitive ways but also increase coupling between the component and Action View.

So, given all that, I'd say the issue is more related to helpers than it is to the global output buffer, although the two are certainly related. As things stand, you have three options:

1. Define the helper method on your component directly, eg:

   class MyComponent < ApplicationComponent
     def my_helper_method
       ...
     end
   end

2. Include the specific helper module into each component you want to use the helpers in, eg:

   class MyComponent < ApplicationComponent
     include MyHelper
   end

3. Wait for view_component v3.

Yes, that's right. We're considering including all helpers in view components by default in v3 😄

[BlakeWilliams]

I think this is a valid edge case that we may have to handle on our end. I generally recommend using helpers over alternatives like include MyHelper since that allows you to re-use any memoization that may exist in the original view context.

When it comes to V3 and including all helpers by default, I think it's relatively likely we'd end up with something like:

def method_missing(method, *args, **kwargs)
  return super unless helpers.respond_to?(method)
  helpers.send(method, *args, **kwargs)
end

The reason I think that approach is likely is due to the memoization/ivar problem mentioned above. This PR that adds the original view context has more details on the impacts of using/not using the default helpers instance. If we were to include all helpers on ViewComponent's themselves, each component would have its own ivars (and possible collisions, but that's another issue entirely). The "single renderer object" (AKA one object that inherits from ActionView::Base) could alleviate some of that if we included helpers there, but components would still need to use method_missing.

All that being said, I think the example provided is something that we have to expect users of the library to run into and expect to work. Before we commit to anything specific, I think we should verify the helper approach and global buffer approach we land on are compatible with one another.

[camertron]

Hmm ok thanks for that additional context @BlakeWilliams. Let me see if I can get something working.

[camertron]

Hey everyone, I ended up having to monkeypatch ActionView, but helpers should be working now. Can you confirm @percyhanna?

[camertron]

Consensus on the team is to ship this as-is for now, then look into a more maintainable approach that doesn't monkeypatch ActionView. The implementation will likely come in the form of an upstream change to Rails (maybe a config option?) and a corresponding update to view_component. Enabling the global output buffer by default is now a stretch goal for v3. If we aren't confident when v3 release time rolls around, we'll punt it to v4.

[percyhanna]

Thanks for the thoughts and context for the changes. I can appreciate that having to reach inside ActionView to fix my use case is not ideal

I think this is a valid edge case that we may have to handle on our end.

I would assume the same. While some of our helper methods could probably be either converted into their own view components, others really are just basic helper methods, so being able to use them within normal Rails views as well as view components would be ideal. For now, the solution I shared in the other PR is working for us.

[camertron]

@percyhanna it may not be ideal, but as @BlakeWilliams mentioned it's still important to address because we will continue to delegate to the original view context for helpers in v3 anyway.

The unbind/bind idea is really interesting, thanks for sharing :) As I understand things, even unbinding and binding won't re-use instance variables, but maybe that's not a concern for your use-case.

Would you be able to test this branch to see if it works for you? I'm planning on merging it on Monday.

[percyhanna]

@camertron Unfortunately it looks like this branch still doesn't work with our use case. I tried calling our helper method directly in the component partial, e.g. helpers.my_helper, and I also used this in the base view component class, neither of which worked.

def method_missing(method, *args, **kwargs, &block)
  if helpers.respond_to?(method)
    helpers.send(method, *args, **kwargs, &block)
  else
    super
  end
end

From some quick testing, I think that the ivars are being re-used, which was a little surprising to me, given how it's written. Perhaps it depends on which ivars we are talking about: ivars from the Rails helpers, or from inside the view component class?

The theory behind my "hack" code above was that it would execute the helper method from within the context of the view component, so that calls to lower-level rendering helpers like content_tag and capture would be executed in the view component context, rather than in the helper/view context.

[camertron]

@percyhanna hmm interesting. Just to rule this out, did you set config.view_component.use_global_output_buffer = true in your Rails config, i.e. in config/development.rb? I ask because I turned your example into a test case last week and it passes if the GoB is turned on.

[percyhanna]

@camertron Sorry, my bad. I think I missed that step in the comments.

I enabled that setting, and it has fixed the regression spec I had created, however, in the application I am now presented with an even worse issue. The symptoms appear to be that some helper methods are getting double-escaped, even in our non-ViewComponent partials. I'm not sure if this is strictly a bug in this PR, or some kind of conflict with some other helpers we are using. The most trivial example is this:

test.erb

<%= javascript_tag do %>
  alert('hello');
<% end %>

This file produces the following output:

<script>
//<![CDATA[
 
alert(&#39;hello&#39;);
 
//]]>
</script>

[percyhanna]

Another example, using the link_to helper:

test.rb

<%= link_to("/some_url") do %>
  <i class="fa-regular fa-bullseye"></i><span>Content</span>
<% end %>

Produces:

<a href="/some_url">
    &lt;i class="fa-regular fa-bullseye"&gt;&lt;/i&gt;&lt;span&gt;Content&lt;/span&gt;
</a>

[github-actions]

2 file(s) had their final line ending fixed:

• test/sandbox/app/components/javascript_tag_component.rb
  ,- test/sandbox/app/components/link_to_tag_component.rb

[percyhanna]

I just wanted to follow up to confirm what I was seeing: the double-escaped HTML was coming from normal Rails views/partials, not a ViewComponent's views. I didn't try this out on a brand new Rails app without anything else, so it's entirely possible that some other part of our application is interfering with the rendering/escaping.

[camertron]

Awesome, thanks @percyhanna! At first I couldn't reproduce, but then I tried using link_to outside a component and was able to reproduce. I wouldn't have caught this without your help, so thank you!

I have since fixed the problem and added a test. Can you confirm things are working in your app?

[percyhanna]

Awesome! My regression test case passes, and the double-escaping issue has been resolved. Thanks! 🎉