Skip to content

Remove the global output buffer; compile on render_in #1432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 25, 2022

Conversation

camertron
Copy link
Contributor

What are you trying to accomplish?

A few months ago, I added an experimental feature called the global output buffer. We were attempting to build a forms framework at GitHub, and since it's a known issue that form helpers don't work very well with components, I was trying to work up a solution that would unblock the forms work. Unfortunately the final global output buffer solution monkeypatches actionview to an unsettling degree. It also turned out we didn't need it for the forms work after all. Finally, it appears it doesn't work with edge rails anymore. So let's get rid of it!

Because the global output buffer degraded performance a bit, I also worked up a patch to counteract it. Unfortunately the patch has led to a number of subtle bugs in development environments, so let's remove it too.

@camertron camertron requested a review from joelhawksley July 21, 2022 21:28
@camertron camertron requested a review from a team as a code owner July 21, 2022 21:28
@Spone
Copy link
Collaborator

Spone commented Jul 21, 2022

It also turned out we didn't need it for the forms work after all.

@camertron If you're able to elaborate on this, I'd be interested in knowing a bit more!

@camertron
Copy link
Contributor Author

camertron commented Jul 22, 2022

@camertron If you're able to elaborate on this, I'd be interested in knowing a bit more!

@Spone sure, happy to elaborate. I was able to get around the form helper issues by creating what I've been calling "view_component lite" inside the primer/rails_forms repo, which is where our new forms framework lives (for now). VC lite is really just a barebones VC implementation that doesn't inherit from ActionView::Base. The important bit is in the ActsAsComponent module. It works by delegating everything to the view context, including the output buffer. I tried a similar approach in view_component but unfortunately the delegation adds quite a bit of overhead. Forms generally represent a fairly small area of the page so the performance hit was acceptable, but not reasonable for all view components at large.

@stevegeek
Copy link

@camertron Understand the desire to remove nasty hacks etc, but just wondering if there is or will be any 'official' solution to the forms problem now? Was relying on the global output buffer to fix my form rendering woes! Could the solution in rails_forms be made into something that can be added more generally to other apps that dont use rails_forms? thanks for any suggestions!

@tmaier
Copy link
Contributor

tmaier commented Jan 27, 2023

Not for everyone looking primer/rails_form has been archived. The work has been merged with primer/view_components. You can find the latest code at primer/view_components#1238

@camertron
Copy link
Contributor Author

Woops @stevegeek, I totally missed your comment in August, sorry about that!

Understand the desire to remove nasty hacks etc, but just wondering if there is or will be any 'official' solution to the forms problem now?

A semi-recent change to action view has removed the need to reassign @output_buffer when capturing, which was one of the big problems the global output buffer was designed to solve. Unfortunately our action view compatibility tests still fail on Rails main, so the Rails change isn't enough. I'm planning on having a deeper look tomorrow.

Could the solution in rails_forms be made into something that can be added more generally to other apps that don't use rails_forms?

Maybe? We could certainly extract the ActsAsComponent module into its own gem. We have tests in primer/view_components that demonstrate view components can be rendered inside ActsAsComponent templates. Theoretically you could write a component which includes ActsAsComponent (instead of inheriting from ViewComponent::Base) and call form helpers + render other view components in the template. I think that would work?

Honestly the long-term fix is probably to stop inheriting from ActionView::Base, but that's going to take a Herculean amount of effort.

@stevegeek
Copy link

@camertron thanks for the reply! Ok understood. I will wait and see how it all evolves and look at ActsAsComponent. For now thanks for taking the time to elaborate on the situation!

@camertron
Copy link
Contributor Author

@stevegeek things move fast in the VC world! @BlakeWilliams has this PR up for review right now which is another take on fixing our buffer issues. It's very likely going to get merged this week I'd say.

@stevegeek
Copy link

@camertron amazing news!

claudiob pushed a commit to claudiob/view_component that referenced this pull request Dec 22, 2023
…1432)

* Remove the global output buffer; compile on render_in

* Use rails main

* Fix linting issues

* Fix bad merge

Co-authored-by: Joel Hawksley <[email protected]>
claudiob pushed a commit to claudiob/view_component that referenced this pull request Jan 3, 2024
…1432)

* Remove the global output buffer; compile on render_in

* Use rails main

* Fix linting issues

* Fix bad merge

Co-authored-by: Joel Hawksley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants