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

Introduce turbo:frame-missing event #445

Merged
merged 5 commits into from
Aug 3, 2022

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Nov 14, 2021

Closes hotwired/turbo#432
Follow-up to hotwired/turbo#94
Follow-up to hotwired/turbo#31

When a response from within a frame is missing a matching frame, fire
the turbo:frame-missing event.

There is an existing contract that dictates a request from within a
frame stays within a frame.

However, if an application is interested in reacting to a response
without a frame, dispatch a turbo:frame-missing event. The event's
target is the FrameElement, and the detail contains the
fetchResponse: key. Unless it's canceled (by calling
event.preventDefault()), Turbo Drive will visit the frame's URL as a
full-page navigation.

The event listener is also a good opportunity to change the
<turbo-frame> element itself to prevent future missing responses.

For example, if the reason the frame is missing is access (an expired
session, for example), the call to visit() can be made with { action: "replace" } to remove the current page from Turbo's page history.

@tleish
Copy link
Contributor

tleish commented Nov 15, 2021

In the above example, does Turbo.visit(url, { response }) update the page/url with the existing response or generate another server request?

@seanpdoyle
Copy link
Contributor Author

@tleish if there's a response key in the Partial<VisitOptions> passed to the visit call, the HTTP request is "simulated" by the Visit, and does not result in an additional call:

https://github.com/hotwired/turbo/blob/v7.1.0-rc.1/src/core/drive/visit.ts#L171-L173

As part of this draft, I'm investigating ways to more easily transform the FetchResponse into Turbo.visit arguments so that client applications don't need to deconstruct them themselves.

@seanpdoyle seanpdoyle force-pushed the missing-frame-in-response branch 2 times, most recently from 7904c5f to a62fa18 Compare November 15, 2021 01:51
@seanpdoyle seanpdoyle force-pushed the missing-frame-in-response branch 2 times, most recently from 5af1e0b to 712516d Compare November 15, 2021 02:24
@tleish
Copy link
Contributor

tleish commented Nov 15, 2021

there's a response key in the Partial<VisitOptions> passed to the visit call, the HTTP request is "simulated" by the Visit, and does not result in an additional call:

A good solution. It would be even better if a developer didn't have to recreate response object. Something like:

addEventListener("turbo:frame-missing", async ({ target, detail: { fetchResponse } }) => {
  // the details of `shouldRedirectOnMissingFrame(element: FrameElement)`
  // are up to the application to decide
  if (shouldRedirectOnMissingFrame(target)) {
    Turbo.visit(fetchResponse.location, { response: fetchResponse })
  }
})

Learned something new, thanks!

@tobyzerner
Copy link

Working beautifully for the most part. But I think I'm running into a bug? I'm using the exact code sample suggested:

addEventListener("turbo:frame-missing", async ({ target, detail: { fetchResponse } }) => {
    const { location, redirected, statusCode, responseHTML } = fetchResponse
    const response = { redirected, statusCode, responseHTML: await responseHTML }

    Turbo.visit(location, { response })
})

The first time this event is triggered by a form submission, it works great – the whole page is replaced with the new HTML. But if I repeat the submission a second time, it breaks. Turbo seems to revert to a cached snapshot of the whole page, and never completes the visit ([aria-busy=true] and [data-turbo-preview] attributes remain on body). Not sure if that's enough info for a reproduction?

@seanpdoyle
Copy link
Contributor Author

I'll investigate that.

I'm not sure if it's related, but it's also worth mentioning that if you're using turbo-rails, you'll have to manually override the controller to render a layout so that the response is a fully formed HTML document.

@tleish
Copy link
Contributor

tleish commented Nov 15, 2021

you'll have to manually override the controller to render a layout so that the response is a fully formed HTML document

Assuming 'frame-missing' still logs a message to the console, should Turbo.visit also log a message of "responseHTML not a fully formed HTML document"?

@tleish
Copy link
Contributor

tleish commented Nov 16, 2021

@tobyzerner - this might be unrelated to these changes as I think I stumbled across a similar issue on 7.0.1.

  1. Visit page with turbo-frame (first time) via redirect and the frame loads src.
  2. Leave page and update data which impacts turbo-frame
  3. Return to page via redirect with the turbo-frame, old data is still displayed in the turbo-frame
  4. Hard refresh of the page shows the updated data.

What I observed is that turbo renders the page and fetches the turbo-frame source. Inspecting the fetch request I can see in includes a full HTML document with the expected updates and the correct turbo-frame. It just does not update the turbo-frame.

Adding <meta name="turbo-cache-control" content="no-cache"> to this particular page resolves the issue, but unless I'm misunderstanding turbo caching, adding <meta name="turbo-cache-control" content="no-cache"> to every page with a turbo-frame shouldn't be a permanent solution.

@seanpdoyle - If needed, I can file a separate issue.

@tleish
Copy link
Contributor

tleish commented Nov 16, 2021

Turns out this has nothing to do with turbo-frames, but overall turbo-caching and server redirects.

Created issue #447

@seanpdoyle
Copy link
Contributor Author

@tleish I've extended the turbo:frame-missing to yield a callable visit() function that knows how to transform the FetchResponse into a visit. Callers can pass additional VisitOptions like action: to the function.

@tleish
Copy link
Contributor

tleish commented Dec 15, 2021

Will this be in the next release?

@seanpdoyle
Copy link
Contributor Author

I'm not sure when the next release will be. I've marked this as a Draft PR to try and start up a discussion.

I think there might be some details to work out:

  • Is turbo:frame-missing a good name? Should it be named turbo:frame-not-found, :frame-error, or something else?
  • Does dispatching a reference to the FetchResponse make sense? It'd effectively make the shape of FetchResponse objects public API, and would represent a commitment to support that API in future releases
  • Does treating turbo-frame[id][disabled] in the body as a "missing" frame deserve its own conversation or PR?

@lefermierperso
Copy link

Hey guys, any updates ?

@tbeemster
Copy link

This will be super helpful for the use case of redirecting to a login page if a frame tries to load and the user session has expired for example -- and even better because it will encourage proper status codes when redirecting (e.g. using a 401 in an authorization-check redirect instead of default 3xx).

I arrived here from the thread in #257 and am looking a solution to the exact use case mentioned above and am wondering if there are plans for this event to be added to Turbo Frames (or if another solution is in the works)

@pfeiffer
Copy link
Contributor

Could this be a good candidate for inclusion in 7.2.0?

An alternative could be to combine the PR in #560 introducing turbo:error with a change that returns a structured Error when a frame is missing instead of just an error message. This would at least allow developers to handle the missing frames, albeit not as elegant as this PR.

@eladmarg
Copy link

this should be merged.
I used this on production for long time, without any issues.

@seanpdoyle
Copy link
Contributor Author

I've opened hotwired/turbo-rails#367 to explore an alternative for "breaking out" of a frame from the server.

As far as what happens when a frame is missing, that seems to be an ongoing discussion.

Closes [hotwired#432][]
Follow-up to [hotwired#94][]
Follow-up to [hotwired#31][]

When a response from _within_ a frame is missing a matching frame, fire
the `turbo:frame-missing` event.

There is an existing [contract][] that dictates a request from within a
frame stays within a frame.

However, if an application is interested in reacting to a response
without a frame, dispatch a `turbo:frame-missing` event. The event's
`target` is the `FrameElement`, and the `detail` contains the
`fetchResponse:` key. Unless it's canceled (by calling
`event.preventDefault()`), Turbo Drive will visit the frame's URL as a
full-page navigation.

The event listener is also a good opportunity to change the
`<turbo-frame>` element itself to prevent future missing responses.

For example, if the reason the frame is missing is access (an expired
session, for example), the call to `visit()` can be made with `{ action:
"replace" }` to remove the current page from Turbo's page history.

[contract]: hotwired#94 (comment)
[hotwired#432]: hotwired#432
[hotwired#94]: hotwired#94
[hotwired#31]: hotwired#31
@seanpdoyle seanpdoyle marked this pull request as ready for review August 1, 2022 16:47
@@ -305,6 +305,13 @@ export class Session
this.notifyApplicationAfterFrameRender(fetchResponse, frame)
}

async frameMissing(frame: FrameElement, fetchResponse: FetchResponse): Promise<void> {
const responseHTML = await fetchResponse.responseHTML
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since servers (like Rails) can respond to Frame requests with incomplete HTML documents, I'm unsure about how to best handle a response without a matching frame.

I'm not sure whether or not Drive should re-use the response HTML or issue a new request to fetch a full page of content. My gut tells me to always make a full request, but I'm curious if there's a quick win I'm not considering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current default is correct, because the majority case here will be to deal with pages like 500 errors or authentication expired errors, where full pages will be returned. But I think we can help developers here by logging a warning to point them in the direction of what's going on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see you changed it to a full visit instead. I guess it doesn't matter that much. This behavior is for the exceptional case in any circumstance. Not for the normal, steady flow.

dhh added 2 commits August 3, 2022 17:26
* main:
  Add turbo:fetch-request-error event on frame and form network errors (hotwired#640)
  Return `Promise<void>` from `FrameElement.reload` (hotwired#661)
  Replace LinkInterceptor with LinkClickObserver (hotwired#412)
  Don't convert `data-turbo-stream` links to forms (hotwired#647)
@dhh dhh merged commit 9d2a3da into hotwired:main Aug 3, 2022
@dhh
Copy link
Member

dhh commented Aug 3, 2022

Would be good to follow up with a doc PR explaining the new event and its power to prevent the default visiting fallback behavior.

@seanpdoyle seanpdoyle deleted the missing-frame-in-response branch August 8, 2022 14:35
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Aug 8, 2022
After re-considering a comment on [hotwired#445][], this commit
re-purposes the response HTML from the `FetchResponse` instance, instead
of issuing a follow-up HTTP request.

[hotwired#445]: hotwired#445 (comment)
seanpdoyle added a commit to seanpdoyle/turbo that referenced this pull request Aug 8, 2022
After re-considering a comment on [hotwired#445][], this commit
re-purposes the response HTML from the `FetchResponse` instance, instead
of issuing a follow-up HTTP request.

[hotwired#445]: hotwired#445 (comment)
dhh pushed a commit that referenced this pull request Aug 11, 2022
After re-considering a comment on [#445][], this commit
re-purposes the response HTML from the `FetchResponse` instance, instead
of issuing a follow-up HTTP request.

[#445]: #445 (comment)
marienfressinaud added a commit to flusio/Flus that referenced this pull request Oct 7, 2022
Turbo 7.2 introduces the event `turbo:frame-missing` if the response
doesn't contain a matching frame. If this event is fired, Turbo will
perform a full-page navigation.

This allows to simplify a common use-case with forms in modals:

- the form is rendered with a `<turbo-frame id="modal-content">` and is
  loaded in the modal
- when the submitted form is successful, the controller returns a
  response _without_ any turbo-frame with id `modal-content` (and so
  Turbo follows the redirection)
- when the submitted form contains error, the controller returns the
  form with errors in a turbo-frame with id `modal-content` (and so
  Turbo loads its content in the modal)

Before that, I had to use a pretty complicated hack based on Turbo
Stream which rendered the forms conditionnaly with the normal layout
(e.g. `feeds/new.phtml`) or in a `<turbo-stream>` element (e.g.
`feeds/new.turbo_stream.phtml`). Now, the expected behaviour comes for
free, I just have to declare a view to be available for modals :).

Reference: hotwired/turbo#445
Revert commit: 7045806
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Error is logged in console when response has no matching <turbo-frame>