-
Notifications
You must be signed in to change notification settings - Fork 418
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
turbo:frame-missing
event not dispatched when code is not 200
#670
Comments
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. |
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. |
cc @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 Imagine a partial or gradual production outage, or a flicker in service. Prior to these changes, requests that respond with a The same is true for a These circumstances could be mitigated by a |
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. |
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 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. |
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 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! |
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. |
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. |
@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. |
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 I'll investigate dispatching a |
@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. |
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:
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. |
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. |
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:
Anywy, good we are reaching a common opinion. And I also agree the event should be fired in all cases, not just 200. |
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 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:
But on the other hand, I find @seanpdoyle's cautionary thoughts compelling:
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:
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):
|
EDIT: I've since realized the change I describe below has already been made in #672 (🎉) and just isn't released into 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 ( 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 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 } })
}) |
Closes hotwired#670 Re-use the existing `2xx` and `3xx` behavior for a response to handle error responses. When the frame is missing from the page (likely for error pages like `404`), dispatch a `turbo:frame-missing` event. Alongside that behavior, yield the [Response][] instance and a `Turbo.visit`-like callback that can transform the `Response` instance into a `Visit`. It also accepts all the arguments that the `Turbo.visit` call normally does. When the event is not canceled, treat the `Response` as a Visit. When the event **is** canceled, let the listener handle it. [Response]: https://developer.mozilla.org/en-US/docs/Web/API/Response
I've opened #693 to resolve this issue. |
Closes hotwired#670 Re-use the existing `2xx` and `3xx` behavior for a response to handle error responses. When the frame is missing from the page (likely for error pages like `404`), dispatch a `turbo:frame-missing` event. Alongside that behavior, yield the [Response][] instance and a `Turbo.visit`-like callback that can transform the `Response` instance into a `Visit`. It also accepts all the arguments that the `Turbo.visit` call normally does. When the event is not canceled, treat the `Response` as a Visit. When the event **is** canceled, let the listener handle it. [Response]: https://developer.mozilla.org/en-US/docs/Web/API/Response
Closes #670 Re-use the existing `2xx` and `3xx` behavior for a response to handle error responses. When the frame is missing from the page (likely for error pages like `404`), dispatch a `turbo:frame-missing` event. Alongside that behavior, yield the [Response][] instance and a `Turbo.visit`-like callback that can transform the `Response` instance into a `Visit`. It also accepts all the arguments that the `Turbo.visit` call normally does. When the event is not canceled, treat the `Response` as a Visit. When the event **is** canceled, let the listener handle it. [Response]: https://developer.mozilla.org/en-US/docs/Web/API/Response
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?The text was updated successfully, but these errors were encountered: