ViewComponent / view_component

A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
https://viewcomponent.org
MIT License
3.28k stars 429 forks source link

Rendering from controller causes double render related to turbo #1534

Open trappar opened 2 years ago

trappar commented 2 years ago

There appears to be a bug related to rendering view components directly from controllers. In the specific case shown in the linked reproduction repo, using view component results in a double page render. Beyond just being extremely inefficient, the double render also wipes out flash messages, which is how I originally discovered it.

Here's the reproduction repo: https://github.com/trappar/view-component-turbo-double-render-bug

All details, expected behavior, reproduction instructions, etc... are available in the repo. I also provide a "fixed" version which simply doesn't render the view component from the controller directly.

System configuration

Rails version: 7.0.4

Ruby version: 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux]

Gem version: 2.72.0

joelhawksley commented 2 years ago

@trappar thanks for taking the time to create a repository that demonstrates this issue ❤️

I'm curious why we would be calling redirect_to inside a render call. In my understanding, that would be a double render in effect.

trappar commented 2 years ago

@joelhawksley I'm not quite sure I understand what you mean. Can you link to where there's a redirect_to inside a render call? Maybe you're just missing the underscore in the route helper?

I.E.: redirect_to_broken_path vs redirect_to broken_path (admittedly poor naming, sorry about that)

joelhawksley commented 2 years ago

Oh duh! I misread. Yes, the naming confused me. I think it's worth moving this to a PR with a failing test on this repo. Would you be up for looking into a fix?

trappar commented 2 years ago

Unfortunately, no. I'm far too busy right now. Making the reproduction repo is about the most I can help. Also, I did originally try to look into what was going on under the hood that might be responsible, and I was left totally stumped - probably mostly due to my inexperience with the related concepts. I doubt I'd be able to make any more progress. Sorry :(

joelhawksley commented 2 years ago

@trappar no worries! I'll leave this open and ask for help.

reeganviljoen commented 1 year ago

@joelhawksley hasn't the capture compatibility patch fixed these issues

joelhawksley commented 1 year ago

@reeganviljoen it should have, yes. @trappar could you follow up here and close if this is no longer an issue?

reeganviljoen commented 1 year ago

@joelhawksley it seems @trappar isn't responding, later I will fork his issue pr and validate if it's still a problem

crespire commented 1 year ago

I'm not 100% sure right now, but I think we've also run into this issue with ViewComponents and Turbo. In my situation, visiting routes directly from a browser address bar was not causing a double render, but using any link or button in the app was.

trappar commented 1 year ago

Hey all, sorry to say but the project where I ran into this is long behind me at this point and I'm not even really working in the same ecosystem these days. Hopefully you're able to take the baton and figure it out :)

crespire commented 1 year ago

Just to also follow up, I ended up disabling Turbo by default via Turbo.session.drive = false and this stopped happening.

reeganviljoen commented 1 year ago

@crespire even though disabling turbo worked, for many apps already using hotwire this is not an option

reeganviljoen commented 1 year ago

@crespire i want to see later if enabling the capture compatibility patch fixes it

crespire commented 1 year ago

@crespire i want to see larer if enabling the capture compatibility patch fixes it

Can you point me to this patch and how I can apply it? I can test it locally, as I was getting the double render on my local dev env as well as on prod. I did a quick Ctrl + F but wasn't able to find a link.

Spone commented 1 year ago

@crespire it's a config option: https://viewcomponent.org/api.html#capture_compatibility_patch_enabled

reeganviljoen commented 1 year ago

@Spone @crespire @trappar after cloning the repo and adding the capture compatibility patch I have concluded that teh capture compatibility patch does not fix this bug

reeganviljoen commented 1 year ago

this only seems to happen when rendered from a controller does anyone have an idea why

crespire commented 1 year ago

@crespire it's a config option: https://viewcomponent.org/api.html#capture_compatibility_patch_enabled

Awesome, thanks! Should have also searched the docs. I can give this a spin, sounds promising. Will report back on results.

reeganviljoen commented 1 year ago

@crespire I have been able to fix it , look here for the fix https://github.com/reeganviljoen/view-component-turbo-double-render-bug

reeganviljoen commented 1 year ago

by removing content_type: "text/html", in teh render the component actualy is able to render with turbo but it does this image so I inspected the networks tab and the page inst double rendered but turbo does not stitch the page together properly but by wrapping the component in a turbo frame It works

crespire commented 1 year ago

Okay, I tried the compatibility patch and also didn't have the issue fixed. I will have to investigate your solution. For now, our quick fix is to do Turbo.session.drive = false which I know is not a general solution.

Dandush03 commented 1 year ago

I solve this issue by passing the component as a string to turbo_stream, here is my suggestion, which I think should be a standard when using turbo_stream.

  # NOTE: THIS WAS IMPLEMENTED WITH DEVISE IF YOU ARE WONDERING ABOUT THE UNKNOWN METHODS
  def render_unpresisted_resource
    set_minimum_password_length

    component = Users::Registrations::NewComponent.new(user: resource)
    component_html = render_to_string(component)

    respond_to do |format|
      format.html { render(Users::Registrations::NewComponent.new(user: resource)) }
      format.turbo_stream do
        render turbo_stream: turbo_stream.replace('new_user', component_html)
      end
    end
  end
KirtashW17 commented 7 months ago

Adding formats: :html to the render fixes the issue.

Explanation: After the form submission, turbo tries to fetch the location of the redirect. Since we send the header Accept: text/vnd.turbo-stream.html, text/html, application/xhtml+xml", the server will try to send us the first match, in this case the Turbo Stream, that we can see in the rails logs that is being rendered without layout (I think it's the default behavior in Turbo Streams)

Started GET "/" for ::1 at 2024-03-08 23:58:25 +0100
Processing by ButtonClicksController#broken as TURBO_STREAM
  Rendering PageComponent
  Rendered PageComponent (Duration: 1.9ms | Allocations: 577)
Completed 200 OK in 2ms (Views: 2.2ms | Allocations: 864)

So when turbo receives this response and detects that is not the same page, performs a full page reload:


Started GET "/" for ::1 at 2024-03-08 23:58:25 +0100
Processing by ButtonClicksController#broken as HTML
  Rendering layout layouts/application.html.erb
  Rendering PageComponent within layouts/application
  Rendered PageComponent within layouts/application (Duration: 1.8ms | Allocations: 539)
  Rendered layout layouts/application.html.erb (Duration: 3.5ms | Allocations: 2820)
Completed 200 OK in 4ms (Views: 4.0ms | Allocations: 3075)

Adding formats to the response, we are saying what formats we support, and this is done implicitly when rendering a template, but not when rendering a component. Maybe it should.

    render(
      PageComponent.new(redirect_to_broken_path),
      content_type: "text/html",
      formats: :html
    )

We could add to the rendering monkey patch this option by default. Components will always render HTML.

@joelhawksley do I make a PR with this change?

joelhawksley commented 7 months ago

@KirtashW17 thanks for taking the time to dig into this issue! Yes, please open a PR with a failing test and we can discuss further there ❤