rails icon indicating copy to clipboard operation
rails copied to clipboard

Action View: use a local variable to hold the template buffer

Open casperisfine opened this issue 1 year ago • 2 comments

This isn't quite working yet, there is a case failing with Builder. I'll get back to it tomorrow but opening a draft now in case someone may have early feedback or ideas. (cc @jhawthorn, @BlakeWilliams)

The one failing test is:

# hello_world_container.builder
xml.test do
  render partial: "hello", locals: { xm: xml }
end

The above test rely on render concatenating directly into the buffer, while in most other cases we rely on the return value of render. The whole thing is quite confusing and I'm not quite sure still how it even work.

But the idea is that if we no longer swap the buffer in @output_buffer we can now reference it using the local variable instead of the instance variable.

If we manage to make this work, it would open the door to further optimizations such as Eruby's chain_appends.

casperisfine avatar Aug 03 '22 15:08 casperisfine

I really can't figure out how legit this builder test is. To me it seems like it rely on a weird behavior of Action View's render helper.

In "classic" ERB templates, <%= render ... %> returns a String which is then concatenated.

For builder however, it's just regular ruby code so:

xml.test do
  render partial: "hello", locals: { xm: xml }
end

The return value is just ignored, and this only work because the partial receive the xml object and write into it. I suppose that's the only way you can do partials with builder, but it's so far removed to how other handlers work that I don't see how to optimize this :/.

casperisfine avatar Aug 08 '22 09:08 casperisfine

Actually I may have found a solution. We could do the "capture" thing as part of the Erubi preamble / postamble.

This way other handlers wouldn't see any semantic change.

casperisfine avatar Aug 08 '22 10:08 casperisfine

@casperisfine 👋 I was looking at this the other day (sorry for the late reply btw) and ended up pairing with @camertron on a ViewComponent change that led us down a similar path so we opened up https://github.com/rails/rails/pull/47194.

We also did a quick test run of it on GitHub locally and it seemed to render everything correctly (except some custom code that has a lot of buffer logic).

BlakeWilliams avatar Feb 01 '23 14:02 BlakeWilliams

Oh thanks for pinging me on this. I never really got back to it. I'll try to review this soon.

casperisfine avatar Feb 01 '23 15:02 casperisfine

Closing in favor of https://github.com/rails/rails/pull/47194

casperisfine avatar Feb 13 '23 13:02 casperisfine