hotwired / turbo

The speed of a single-page web application without having to write any JavaScript
https://turbo.hotwired.dev
MIT License
6.74k stars 430 forks source link

Break out of turbo frame loses flash #897

Open gap777 opened 1 year ago

gap777 commented 1 year ago

Hi, I'm trying to use the recent changes to allow server-side decision to break out of a turbo frame. Here's my sequence of activity:

  1. GET from within a turbo frame
  2. Server detects error condition, and instead of responding with turbo-frame, redirects WITH flash alert. Redirect is to page having meta name="turbo-visit-control" content="reload"
  3. Page is fetched, containing alert, and discarded.
  4. Page is fetched again. No flash.

The issue I'm reporting is the server logs show the redirected-to page being fetched twice (steps 3 and 4), and the flash message (included in the response to 3) is discarded and not included with step 4.

Am I doing something wrong? Is this a bug?

kevinmcconnell commented 1 year ago

Hi @gap777, the page being fetched twice is part of how turbo-visit-control reload works. In this case the first request (following the redirect) is made in the context of the frame, and it's not until the response is received from it that Turbo knows that a reload is needed.

It has to make that second request to do the reload. That's partly because it's the mechanism for doing a full-page (not JS fetch) load, which is what that meta tag specifies is required. But it's also because the initial response may have omitted some content that wasn't needed in a frame context, but would be needed for the full page. Turbo Rails will do this by default -- it'll use a minimal layout, rather than the application layout, when responding to a frame request.

So unfortunately if you render the flash message in that first response then it will get lost, as you say.

The simplest solution I can think of is to render the flash content in your application layout. Since that's not used when responding to the frame request, it should be present on the full page load. That should avoid this problem in general (and is how I normally have things set up anyway).

If having the flash in the layout doesn't suit, and you want to fix this for a specific page, you could potentially persist the flash explicitly between those two requests, by doing something like this in the controller action:

flash.keep if turbo_frame_request?

Or you could make rendering the flash conditional in some other way; whatever way best suits your app.

I've just quickly tested those two approaches and both seem to do the trick. But let me know if it doesn't work out for you.

gap777 commented 1 year ago

@kevinmcconnell Thank you for such a quick response!

Actually, my flash content is a part of my layout:

/ layouts/application.html.slim
doctype html
html lang='en' data-theme-mode='light'
  = render 'core_ui/layouts/head'

  body.app-wrapper
    = render 'core_ui/shared/modal'
    = render 'core_ui/shared/confirm'

    / Side Navigation
    = render 'core_ui/layouts/side_nav'
    .app__main
      = render 'core_ui/shared/flash'
      .page__header
        = render 'core_ui/layouts/page_header'
      .page__subheader
        = yield(:subheader)
      .page__content
        .page__content-area
          = content_for?(:content) ? yield(:content) : yield

It seems like you're surprised that the application layout is being included in the turbo frame response. Why? I respond to the turbo frame GET request with a redirect:

     # controller logic, resulting in determining an error state .... 
     format.html do
          redirect_to that_other_path, status: :see_other, alert: @error_message
     end

Are you suggesting that the subsequent GET from redirect does not include application layout?

kevinmcconnell commented 1 year ago

Ah yes, sorry, I was speaking specifically about how it works with Turbo Rails. I should have been clearer about that!

Turbo Rails will override the layout when the request is coming from inside a turbo frame. Such requests have a Turbo-Frame header, and the presence of that header is what's used to skip rendering the default layout. It does that to help performance (no need to render a big layout if you only need the frame content). But it does also have the side effect of stopping that request from rendering the flash in a case like this. So in that case, yes, the GET resulting from the redirect will not include the application layout.

But if you're using Turbo outside of Turbo Rails (or if you are using Turbo Rails but are overriding the layout), that's not going to happen. I don't know how your app is set up. But you always have the option of doing something like this yourself, if it suits your case. You could consider skipping the flash portion when that Turbo-Frame header is present, since a normal frame request won't use it if the flash section it outside of the frame.

This is the part where Turbo Rails does this, which might explain this better than I have :)

I think what it boils down to, in either case, is you either need to not render your flash on that first GET, or you need to use a flash.keep to keep it around for the second one. It's a bit fiddly that you have to do take that into account, but it's due to the fact that turbo-visit-control requires that second request to get the expected behaviour, and flash messages don't persist past that first rendering.

davidalejandroaguilar commented 1 year ago

Just encountered this issue too. I'm using the workaround you suggested here https://github.com/hotwired/turbo/pull/863#issuecomment-1470184953 and using flash.keep as you suggest here, yet the flash is not persisted.

I mentioned a particular use case here https://github.com/hotwired/turbo/pull/867#issuecomment-1483757113 where this also applies.

Wondering if the solution is rather finding a way to break out of the frame from the controller. Allowing the flash to work as before in this case.

gap777 commented 1 year ago

@kevinmcconnell @davidalejandroaguilar

Huh....

I'm using Rails 7, and my package.json includes turbo-rails, so I think I should be in the "Turbo Rails" camp, but I see different results.

Maybe the next question is, "Why is my application layout being used? (given the logic https://github.com/hotwired/turbo-rails/blob/main/app/controllers/turbo/frames/frame_request.rb#L24".

gap777 commented 1 year ago

Ok, I think I have an idea...

My controller has a layout statement at the top. Perhaps this clobbers the layout directive from frame_request.rb#L24?

I tried replacing my controller layout statement with the logic blend between frame_request (to use turbo_rails/frame layout for turbo frame requests, and my layout for others).

This causes the turbo_rails/frame layout to be fetched properly for the redirect-in-the-context-of-a-turbo-frame, but misses the entire point: I'm trying to break out of a turbo frame, and the only way I can break as a result of a server-side decision is to include the meta name="turbo-visit-control" content="reload" in the response HEAD, which is rendered by the layout, which I can't control b/c I'm using the turbo_rails/frame layout (because this is performed in the context of a turbo frame).

So... how do I break out of a turbo frame and redirect w/ a flash (alert)?

gap777 commented 1 year ago

I'll allow that this is all new to me, so maybe I'm missing something... but the solution proposed here (https://stackoverflow.com/a/75750578/438205) of a custom turbo action seems exactly what I want.

If Turbo Rails has an answer for this scenario, I'd prefer to use the canonical one... but I can accept a custom one, given the custom one is extending things in "blessed" ways. ;-)

kevinmcconnell commented 1 year ago

@gap777 yes, your layout statement is probably what's causing this to happen, as it will override the Turbo Rails one.

The way this works with the Turbo Rails version is that it has a <%= yield :head %> in its layout, so pages can still return head content if they need to by rendering into that block.

That means your page's view can include the meta name="turbo-visit-control" content="reload". You don't need to put the actual turbo-visit-control directive in the layout. If you see what I mean?

Turbo Rails provides a helper for this: if you're using its layout (or you have a <%= yield :head %> in your own frame-specific layout) then you can do this in your view:

<% turbo_page_requires_reload %>

Or if you want to do it without the helper, a block like this should work:

<% content_for :head do %>                                                                                                          
  <meta name="turbo-visit-control" content="reload">                                                                                
<% end %>

In your situation, that is probably what I'd do: make sure the layout I'm using for frame content has a :head block, and use the Turbo Rails helper to set that meta tag in there.

But a Turbo Streams action is an interesting idea too! There's certainly many different ways to approach it :)

gap777 commented 1 year ago

@kevinmcconnell Thanks for explaining things so patiently. I tried your recommended approach, and am able to do what I want.

My scenario is specific to wanting to break out of a turbo frame with a redirect to another page (eg, in an error scenario). I don't want to unconditionally include that turbo_page_requires_reload on that other page, nor on every page (as that would disable turbo drive across my app). I only want it to be included when rendering that page in this context.

So I've put a conditional guard around the turbo_page_requires_reload (depending on a query param being present), and then redirect to that page, with the query param.

I'd really like this "guarded turbo_page_requires_reload" expression on every page, so that any page can break out of a turbo frame and redirect to wherever it wants... but then I have to litter every page with this expression. I can't include it in the layout, b/c of course, layouts aren't used from within the context of a turbo frame request.

So.. you've helped me in the immediate problem.

But the larger strategic pattern seems be escaping me.

laptopmutia commented 1 year ago

Okay here we go!

so for everyone and anyone that been through all of this like me

this is how you could redirecting and rendering notice from a turbo frame

davidalejandroaguilar commented 1 year ago

@laptopmutia Unfortunately, that'd make the page where you add <% turbo_page_requires_reload %> always do a full page load. This works well for the login page example everyone seems to have, but for other uses, we don't want this behavior.

Example:

  1. Being in a /carts/1 page, within a turbo frame for the cart (for modifying the quantities in place).
  2. Submitting the cart and redirecting to /menu

If we added <% turbo_page_requires_reload %> to the /menu path, then this page would always do a full reload, effectively disabling turbo drive for that page.

davidalejandroaguilar commented 1 year ago

A better workaround is adding flash.keep if turbo_frame_request? to the controller action where you're redirecting to (/menu in my example above), as @kevinmcconnell recommended.

But it's only needed because of the turbo:frame-missing workaround that causes 2 requests when the frame is missing and losing the flash in the process.

For me, this is appropriate:

class ApplicationController < ActionController::Base
  before_action :keep_flash_if_turbo_frame_request

  def keep_flash_if_turbo_frame_request
    flash.keep if turbo_frame_request?
  end
end

The PR for breaking out of a frame from the server internally handles the missing flash, so that really is the best solution.