Add OutputBuffer#raw and #capture to reduce the need to swap the buffer#45731
Merged
byroot merged 1 commit intorails:mainfrom Aug 3, 2022
Merged
Add OutputBuffer#raw and #capture to reduce the need to swap the buffer#45731byroot merged 1 commit intorails:mainfrom
byroot merged 1 commit intorails:mainfrom
Conversation
casperisfine
force-pushed
the
action-view-buffer-slice
branch
from
e67f02f to
665c9d8
Compare
casperisfine
commented
Aug 2, 2022
Contributor
Author
There was a problem hiding this comment.
I don't see any legit reason for using SafeBuffer as @output_buffer and its holding us back.
casperisfine
force-pushed
the
action-view-buffer-slice
branch
from
665c9d8 to
60792cc
Compare
casperisfine
pushed a commit
to casperisfine/temple
that referenced
this pull request
Aug 2, 2022
In rails/rails#45614 I modified `ActionView::OutputBuffer` to no longer be a subclass of `ActiveSupport::SafeBuffer` and as such it's not recommended to use `SafeBuffer` for buffering views anymore. Additionally in rails/rails#45731 I start doing some changes so that we stop mutating `@output_buffer`. So template should no longer assign `@output_buffer` themselves.
casperisfine
pushed a commit
to casperisfine/temple
that referenced
this pull request
Aug 2, 2022
In rails/rails#45614 I modified `ActionView::OutputBuffer` to no longer be a subclass of `ActiveSupport::SafeBuffer` and as such it's not recommended to use `SafeBuffer` for buffering views anymore. Additionally in rails/rails#45731 I start doing some changes so that we stop mutating `@output_buffer`. So template should no longer assign `@output_buffer` themselves.
casperisfine
force-pushed
the
action-view-buffer-slice
branch
from
60792cc to
4a75b85
Compare
Member
|
also cc @BlakeWilliams who has looked at output buffer swapping |
jhawthorn
reviewed
Aug 2, 2022
casperisfine
pushed a commit
to Shopify/rails
that referenced
this pull request
Aug 3, 2022
It's really not what it's meant for. `@renderer` is a better use for this as it's the variable used by the DOM testing helpers. Ref: rails#45731
Right now many helpers have to deal with two modes of operation to capture view output. The main one is to swap the `@output_buffer` variable with a new buffer. But since some view implementations such as `builder` keep a reference on the buffer they were initialized with, this doesn't always work. So additionally, the various capturing helpers also record the buffer length prior to executing the block, and then `slice!` the buffer back to its original size. This is wasteful and make the code rather unclear. Now that `OutputBuffer` is a delegator, I'd like to refactor all this so that: - @output_buffer is no longer re-assigned - A single OutputBuffer instance is used for the entire response rendering - Instead capturing is done through `OutputBuffer#capture` Once the above is achieved, it should allow us to enabled Erubi's `:chain_appends` option and get some reduced template size and some performance. Not re-assigning `@output_buffer` will also allow template to access the local variable instead of an instance variable, which is cheaper. But more importantly, that should make the code easier to understand and easier to be compatible with `StreamingBuffer`.
casperisfine
force-pushed
the
action-view-buffer-slice
branch
from
4a75b85 to
fc0db35
Compare
judofyr
pushed a commit
to judofyr/temple
that referenced
this pull request
Aug 7, 2022
In rails/rails#45614 I modified `ActionView::OutputBuffer` to no longer be a subclass of `ActiveSupport::SafeBuffer` and as such it's not recommended to use `SafeBuffer` for buffering views anymore. Additionally in rails/rails#45731 I start doing some changes so that we stop mutating `@output_buffer`. So template should no longer assign `@output_buffer` themselves.
skipkayhil
pushed a commit
to skipkayhil/rails
that referenced
this pull request
Jan 19, 2023
It's really not what it's meant for. `@renderer` is a better use for this as it's the variable used by the DOM testing helpers. Ref: rails#45731
This was referenced Jan 27, 2023
camertron
added a commit
to camertron/rails
that referenced
this pull request
Jan 30, 2023
A recent change designed to avoid swapping references to the view context's output buffer (rails#45731) added a #capture method to the OutputBuffer class. The original CaptureHelper#capture method however still swaps the buffer via CaptureHelper#with_output_buffer, meaning the @output_buffer reference is still reassigned by form helpers, etc. This change delegates capture behavior to @output_buffer so a single buffer is used per request.
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Right now many helpers have to deal with two modes of operation to capture view output.
The main one is to swap the
@output_buffervariable with a new buffer. But since some view implementations such asbuilderkeep a reference on the buffer they were initialized with, this doesn't always work.So additionally, the various capturing helpers also record the buffer length prior to executing the block, and then
slice!the buffer back to its original size.This is wasteful and make the code rather unclear.
Now that
OutputBufferis a delegator, I'd like to refactor all this so that:@output_bufferis no longer re-assignedOutputBuffer#captureOnce the above is achieved, it should allow us to enabled Erubi's
:chain_appendsoption and get some reduced template size and some performance.Not re-assigning
@output_bufferwill also allow template to access the local variable instead of an instance variable, which is cheaper.But more importantly, that should make the code easier to understand and easier to be compatible with
StreamingBuffer.NB: This will need a followup or two to remove the remaining
@output_bufferre-assignments.FYI @jhawthorn