turbo:frame-missing event not dispatched when code is not 200

[davidjr82]

<turbo-frame id="my-frame">
    <a href="/non-existing-url">Link to 404 from inside frame</a>
</turbo-frame>

If in the code above you click in the link, turbo:frame-missing is not dispatched.

From my understanding, we are inside of a frame expecting a frame in the response, so the event should be dispatched.

Is this expected from the new turbo:frame-missing event, or am I wrong?

[marcoroth]

This event is dispatched if you navigate to a different page and the new page doesn't include the frame.

It will dispatch the event for every missing frame on the new page you had present on the previous page.

[davidjr82]

Then it looks like a bug.

If I navigate from a page within a frame, to another page without frames, the event is not being fired if the response from the destination URL has not a code 200.

In that situation, the event is not being fired, and it is expected as it fits in your description.

[dhh]

cc @seanpdoyle

[seanpdoyle]

Scenarios like this make me nervous about breaking the implicit contract that a navigation within a frame will stay within a frame.

The changes shipped in #445 aimed to handle successful submissions to help improve the developer experience, but I'm afraid its implications are more widespread.

By handling responses that aren't 2xx or 3xx, we're opening the door for errant <turbo-frame> request to take over the page. In cases like 401 and 403 codes, that can be a useful change.

Imagine a partial or gradual production outage, or a flicker in service. Prior to these changes, requests that respond with a 5xx would "break" the frame making the request, but would leave the rest of the page unaltered. If the outage were momentary, users would be able continue to navigating their current pages without interruption elsewhere. If we continue with this change, the frame's 5xx would be promoted to a page-wide render, and would halt their experience.

The same is true for a 404. Imagine a user is viewing a page with a <turbo-frame> that references some record that is deleted by another user while the current user is browsing. If that frame were refreshed in some way, or navigated to /edit that record, the response would be a 404. Prior to these changes, the frame would break, but the rest of the page would persist, and the user experience would have an opportunity to degrade gracefully. After these changes, the entire page would be taken over by the 404 status code page.

These circumstances could be mitigated by a turbo:frame-missing error handler, but I wonder if there are some more reasonable defaults we could establish.

[davidjr82]

I have been following some issues/discusions about this topic in the repo. I know there were other options on the table, but I think the decision of firing an event was the right one. And the reason is because you don't break the implicit contract that a navigation within a frame will stay within a frame.

The benefit of taking the decision of firing an event is that you don't break your design, but you allow developers to take the decision on their own development. So doing this, you don't accept that responsibility, you give it to the next one in the chain, the developer.

Having said that, I don't see any problem firing the event in a 404 or 500 errors. If the developers want the user to stay in the site, just breaking that frame, they can do it. If they want to give a error message and do a full reload, they can do it. They can check they response code and decide case by case, and the default would be the same than before these changes, but with a new option for the developers (instead of the previous console message).

In my opinion, I would fire the event always, because I see here an excelent example of what an event should be used for. And just a small section in docs to explain it, with a small example of full reload, alert of error, or ignorr and continue.

[seanpdoyle]

Thank you for sharing that @davidjr82. I think we're in agreement about the event.

In my message, I was unclear about my concern and implied some important subtext that should have been made explicit: my concern is about the new default behavior of dispatching a call to Turbo.visit when a turbo:frame-missing event isn't handled and canceled.

In my opinion, firing an event and providing applications with a seam to customize behavior is a clear and unqualified win. What I'm less clear on is whether or not it makes sense to have a default behavior outside of logging an error.

I hope that helps to clarify things a bit.

[davidjr82]

Ok, I didn't understand it then.

So we agree then that at least, firing the event even in 4xx or 5xx cases is ok, right?

Regarding to the default behaviour of dispatching a call to Turbo.visit, I wouldn't do it. Doing it or not, some developers needs to do something in their code to fit their needs. If you do it, some of them will need to cancel the visit to just break only that frame as you said. If you don't do it, some of them will need to add a event handler to decide what to do, like a full reload.

So both options have something to win and something to lose in equal parts, therefore, I would suggest to stay close to your design and just emit the event always (although I think there are situations where you are already breaking the contract, as targeting _top in the self frame).

If in the future there are more situations where the contract doesn't fit, then you can re-think the contract itself.

It's just my opinion, hope it helps.

Then would we let this issue open until we fire the event in all code responses?

Thanks for the effort, this is a great library!

[dhh]

I think the problem is that if you don't directly setup a turbo:frame-missing handler to deal with the 404, and the frame hits one, you're leaving the UI in a broken state with no explanation. Maybe the brokenness is contained to the frame, but that hardly makes it better? The user took an action, but got no feedback that it failed.

I think letting all error codes, and therefore all pages without the matching turbo frame, target top is the right default. Better to have the 404 take over the whole page than to fail silently. And then, if you're a developer who wants to explicitly let 404s be contained, you can catch the event and do your custom work.

[davidjr82]

It would be a good default also, but probably many developers will do their custom work anyways as the possibilities are many. For example, I guess many developers will handle a 419 session expired code to redirect to login again. But even with that, it is probably a better developer experience as you said.

As a suggestion, I would add in the docs section a small example to handle the 419 code, that I think it will be a common scenario.

[davidjr82]

@dhh I have been testing the idea of a default targeting top on missing frame, and I don't like it.

It happens that by default redirects me when I don't want to do it, and it is more difficult to handle those exceptions that handle what I want to do when the frame is missing, because usually when I want to do something when the frame is missing is because I have another clear indicator as a 403 code or whatever. But handle a regular 200 that I don't want to navigate is more difficult because I have no other element to distinguish from a different 200 code response.

For me, the @seanpdoyle default option of just firing an event and let the developer handle it is the right one.

[seanpdoyle]

I've opened #677 to propose rolling back to the existing behavior and to provide event-declaring applications with a drop-in solution for transforming a turbo:frame-missing event into a Turbo.visit call.

I'll investigate dispatching a turbo:frame-missing event for unsuccessful responses separately.

[dhh]

@davidjr82 I don't follow the objection here? When is the "this request resulted in a dead frame with no response" ever the correct response? And if one is to do a custom handling, why does it matter that the default falls back on the safer "whole page is changed, when the frame isn't matched"?

The session-expired case seems like a good default example that should be captured without a custom error handler. If the session is expired, you really should get redirected to a page-clearing login screen.

[davidjr82]

@dhh

For me, "this request resulted in a dead frame with no response" is never a correct response for a end user. It was right when you fired a console error log to inform the developer, that was the former behaviour, with the objection that the developer can't do anything with that message but change the code to return a frame instead.

Now we want to change the default behaviour, because we all agree that doing nothing but a console error message is not good for the end user, and there are many cases where you need to do something with it.

Here we have 2 options:

1. fire an event and let the developers do their work or
2. assume "their work" usually is to target _top frame, and let them interrupt that process if wasn't that.

In the second case, how do you know when to interrupt that process? how you can tell between two 200 responses, when in one case is right to target _top frame and not with the other?

My point of view is that, in the first case, you have more elements to decide what to do with the call (basically you can decide with the response code), so you can take decisions, but in the second case, you would have to deal with many if-else cases.

The thing here is that, if you chose number 1, then you can not make a console error for the develper, because you don't know if they are already handling it. Couldn't we fire a console info message if the environment is local/staging and supress it if environment is production?

Please, take this thoughts not as an objection, but my DX feelings after using it just for a day. I can see less development cases than you, so if you feel the other option is the right one, probably you are right.

[dhh]

Good to go over this in detail. It's an important change.

What strongly motivates me for option #2 is that developers rarely consider all the possible failure modes their app can trigger. Knowing that, and knowing that if a developer misses one, and does not write custom code to catch the event, then you have a dead UI, is in my book not the right default.

But I also think that if you're triggering requests from within a frame, and the response to that request does not have a frame, you actually do intend for that to work as though its targeting _top. If you didn't, you would have returned a response with the matching frame. So I don't see needing to catch the event and do custom work as common, and therefore do not see a big penalty to having some if/else go on in there. Nor a problem with logging a line, even if the developer is also doing the catching.

Appreciate you laying out the argument, though!

For now, let's proceed with the new default, and then do the work on making sure it captures all the cases, not just 200.

[davidjr82]

I am reading again my argument about option number 1, and now it makes no sense to me (haha!).

If I have enough elements to decide what to do with the event, then in the second case I have exactly the same arguments to decide if I should interrupt the process. So if I am able to decide where to go in the first case, then I must be able to decide if I want to avoid going somewhere in the second.

I endorse your opinion about option number 2, but for me there are a few things to solve before feeling completly good with it:

• If I want no frame in the responses (example, sending a job to a queue that does not need to give info to the user), then the most practical way to do it is to response with a dummy empty frame if I don't want to use custom javascript. Feels not clean to me.
• I like to keep the logging line, but I would like to not have it in production because I would not like to give more info that what is needed in prod. Would that be possible?

Anywy, good we are reaching a common opinion. And I also agree the event should be fired in all cases, not just 200.

[jon-sully]

What a great thread of conversation and thoughts 😁🤓

Trying to follow along here, #445 shipped in Beta.2 a couple weeks ago which exposed the frame-missing event for developers to optionally choose to hook into and preventDefault. If they did not do that, a top-level Turbo Drive navigation event would be executed to render the response content (the same content that didn't contain the matching Frame)... but only in the case of a 2xx or 3xx response status. Is that right?

So on one hand, I agree with the OP in that this new default (Turbo Navigating into the frame-missing response content) feels a little nonuniform to have only happen on 2xx and 3xx responses. It sounds like @dhh is there too:

For now, let's proceed with the new default, and then do the work on making sure it captures all the cases, not just 200.

But on the other hand, I find @seanpdoyle's cautionary thoughts compelling:

Imagine a partial or gradual production outage [..]. Prior to these changes, requests that respond with a 5xx would "break" the frame [..], but would leave the rest of the page unaltered. If the outage were momentary, users would be able continue to navigating their current pages without interruption elsewhere. If we continue with this change, the frame's 5xx would be promoted to a page-wide render, and would halt their experience.

Especially for apps that have a lot of interactivity across many frames on a single page (like HEY? 😋).

I feel like two vectors are pertinent to the discussion of which default is better (halt / redirect the whole page vs. just let the frame alone die) — whether the majority of Frame usage is for GETs vs. POST/PATCH's, and the weight of the danger of silent failures for POST/PATCHs. The former matters because IMO if most Frame usage is just for GETs then "just let the frame alone die" feels more reasonable, but vice versa for POST/PATCH's. Maybe more important and regardless of majority usage is the second vector.

Even if POST/PATCH's are the minority of requests generated from Frames, I think the danger of "just let the frame alone die" in the sense that a POST/PATCH failure might go somewhat undetected is weighty. Even if I have just a small frame in the middle of a largely-interactive page but the action was POSTing back a record / object, it feels more clear and deliberate to me as an end-user for my whole page experience to halt and render the 4xx or 5xx view rather than just have the little frame go white. Just having the frame go white doesn't give me a clear indication that my intended action failed. Especially if I'm not a dev and not savvy to opening dev tools, I'm left very unclear as to whether or not my action was received. I think halting the whole experience solves that problem.

So I guess all that to just add some additional justification / thought to stick behind:

For now, let's proceed with the new default, and then do the work on making sure it captures all the cases, not just 200.

At the end of the day the event is exposed to developers who want to implement their own process and that's great! And, outside of the transient server issues that Sean mentioned, I think this sentiment captures the rest of the circumstances (at least to me):

But I also think that if you're triggering requests from within a frame, and the response to that request does not have a frame, you actually do intend for that to work as though its targeting _top. If you didn't, you would have returned a response with the matching frame.

[jon-sully]

EDIT: I've since realized the change I describe below has already been made in #672 (🎉) and just isn't released into beta.3 yet! Sweet 😁. Leaving most of the rest of this comment for others who want to run beta.2 but need the event handler code below to avoid a second request.

---

One additional note I wanted to make after playing with this yesterday is that the behavior of "no matching frame was found, just render the response that was given as a whole page navigation" isn't quite true currently (beta.2). In 1043f0d (one of the latter commits of #445) the behavior was changed slightly from "render the exact content that was given" (don't make another request to the same path) to "request the same path again but as a full visit". I'd suggest that we may want to revert that change. (EDIT: This happened in #672 🎉 )

I can understand that re-requesting the same path would be valuable in the case where potentially only a partial was returned from the initial response, but I'll wager that that's very, very unlikely. If no matching frame was found I believe it likely that the response was already a full page of markup. E.g. if we're protecting against the case where the back-end only served just a frame and we don't want to render a layout-less page (therefore re-request the whole page as a full navigation), I think that problem is already guarded against by the notion that there is no matching frame. The odds that the back-end served back a response with no matching frame... but is still only a frame, just not the right one, seems... unlikely.

The downside of re-requesting the same path, as noted in several other threads over the last year, is that we lose any flash message/content that would've been present in the first response. I think that's a notable cost.

In the meantime (and for others), the frame-missing event is obviously exposed and we have control to do this ourselves, so here's the JS snippet I'm using to use the initial request's HTML rather than re-request the same path again. This is essentially a clone of the 'before' from 1043f0d. I'll dump this once beta.3 is out and we hop on it! 👍

document.addEventListener("turbo:frame-missing", async (event) => {
   event.preventDefault()

   const responseHTML = await event.detail.fetchResponse.responseHTML
   const { location, redirected, statusCode } = event.detail.fetchResponse

   Turbo.visit(location, { response: { redirected, statusCode, responseHTML } })
 })

[seanpdoyle]

I've opened #693 to resolve this issue.