Skip to content
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

Rendering from controller causes double render related to turbo #1534

Open
trappar opened this issue Sep 23, 2022 · 24 comments
Open

Rendering from controller causes double render related to turbo #1534

trappar opened this issue Sep 23, 2022 · 24 comments

Comments

@trappar
Copy link

trappar commented Sep 23, 2022

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
Copy link
Member

@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
Copy link
Author

trappar commented Oct 12, 2022

@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
Copy link
Member

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
Copy link
Author

trappar commented Oct 12, 2022

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
Copy link
Member

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

@reeganviljoen
Copy link
Collaborator

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

@joelhawksley
Copy link
Member

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

@reeganviljoen
Copy link
Collaborator

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

@crespire
Copy link

crespire commented Jun 2, 2023

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
Copy link
Author

trappar commented Jun 9, 2023

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
Copy link

crespire commented Jun 9, 2023

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

@reeganviljoen
Copy link
Collaborator

reeganviljoen commented Jun 10, 2023

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

@reeganviljoen
Copy link
Collaborator

reeganviljoen commented Jun 10, 2023

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

@crespire
Copy link

@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
Copy link
Collaborator

Spone commented Jun 10, 2023

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

@reeganviljoen
Copy link
Collaborator

@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
Copy link
Collaborator

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

@crespire
Copy link

@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
Copy link
Collaborator

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

@reeganviljoen
Copy link
Collaborator

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
Copy link

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
Copy link

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
Copy link

KirtashW17 commented Mar 8, 2024

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
Copy link
Member

@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 ❤

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants