Throwing Special Exceptions in loaders/actions #2389
Replies: 19 comments
-
This sounds promising - would it make it possible for a route not to handle 404s? Instead it'd throw notFound(), and let the default 404 handler handle it? eg
If so this seems like a huge improvement, meaning we don't have to keep handling 404 cases repeatedly in each route. Another option would be to have a sentinel return value to indicate |
Beta Was this translation helpful? Give feedback.
-
Yes, that's the bubbling, just like ErrorBoundary. In your (There would no longer be a routes/404.jsx file, the "global not found handler" would just be the export on root) |
Beta Was this translation helpful? Give feedback.
-
That sounds pretty good. Though are there not some advantages to having routes/404.jsx? What I like about routes/404.jsx is that it is a route in its own right, same as the others, it gets to implement Would this work the same in your new proposal? Or would the root route now need to handle the 404 case in each of its meta and links functions? |
Beta Was this translation helpful? Give feedback.
-
The url /404 shouldn't really exist, we've never liked it and it's been on the chopping block since it was first created. For example, what if you want to build a bbq finder and want to search for the best bbq in atlanta? Additionally, a status code of 404 means the current URL is not found. If you switch the URL to You can branch in |
Beta Was this translation helpful? Give feedback.
-
I think you're misunderstanding me... yes I agree the url should never be (Redirecting to /404 is the advice i got on the remix discord, which seems bizarre to me, eg return a 302 status code to go to another path that then returns a 404 status code...???) |
Beta Was this translation helpful? Give feedback.
-
So i'm curious then, why does remix respond on the path |
Beta Was this translation helpful? Give feedback.
-
Yeah, that stuff is kinda nice, but then we'd have two ways to do it. We already have Additionally, this handles more than 404, it handles all 29 of the 4xx codes. Instead scenario solving, we're generically handling all http client errors (and again, handling them in the same way as 5xx http server errors). |
Beta Was this translation helpful? Give feedback.
-
We've known since the beginning that our routes/404.js and routes/500.js were wrong, but we didn't know what to do about it. Then we figured out Now we also know how to get rid of Remix responds to /404.js because we knew we were going to nix it eventually, so we didn't worry about the unintended behavior. |
Beta Was this translation helpful? Give feedback.
-
Makes sense. Definitely sounds like an overall improvement, to stop responding on /404 and to not require dynamic routes (eg /path/$id.tsx) to have to handle links, meta, render for the not found case (unless they wanted to) |
Beta Was this translation helpful? Give feedback.
-
It still seems like there might be a best-of-both-worlds solution, where errors (4xx, 5xx) could be handled by their own routes (with rendering, links, meta), and handled at different levels, and to have remix not respond on /404 or /500 etc. I think express does this well for the not-found case, probably for other error cases too, though I am not 100% sure. |
Beta Was this translation helpful? Give feedback.
-
Best of both worlds or two worlds? 😜 I agree having their own routes would be nice. At one point we considered like a routes/ and an exceptions/ folder. So you could have global But we have a strong preference to only have one API for the same use-case. Using route exports lets us have global handling (in root.tsx) as well as contextual (inside the routes). The only real trade off is that you have to branch in root.tsx. Not a big deal. |
Beta Was this translation helpful? Give feedback.
-
That makes sense, bit that being the case maybe "Not Found" is the incorrect terminology here if I'm going to use that to handle "unauthorized" as well. I'm that case, the "pedantic" solution your proposed probably makes more sense semantically and rationally. "ClientError" as an export would be best I think. I'll also add that so far I've been making use of throwing errors to send error messages to my client and that's worked well. Adding a little check for whether it's a remix error would not be a problem for me. So I think this would be a great change 👍 |
Beta Was this translation helpful? Give feedback.
-
The pedantic solution looks better, a single ClientError exported component, also maybe change ErrorBoundary to ServerError? Since it will be used to represent that, ClientError are always thrown intentionally from the loader/action, while a ServerError is an uncaught error and ErrorBoundary will be used only for that. |
Beta Was this translation helpful? Give feedback.
-
I think keeping ErrorBoundary as-is would be a good idea because it handles more than just server errors and if you had both then people would confuse "client errors" as errors that happen in the client when actually they're instigated by the server sending a 4xx error. |
Beta Was this translation helpful? Give feedback.
-
🤔 that makes sense, so ErrorBoundary is for uncaught errors in general, because you could also catch any client-side error and put your own error boundary for some specific component or change a state to represent it, so with this ClientError component you will have:
|
Beta Was this translation helpful? Give feedback.
-
When you spell it all out like that it feels like a lot 😅 but the alternative is handling different kinds of errors in a single mechanism and that would be even worse. I guess this is just the number of different categories of errors an application can have and there's one way to handle each. |
Beta Was this translation helpful? Give feedback.
-
I think for most apps you only need the first two, specially in Remix where most logic is moved to the server, effects would be really simple so maybe you don't need a try/catch, you will not have a lot of event handlers either so less errors to catch and with TypeScript a lot of possible render errors are removed so custom error boundaries may not be needed either, I used to use error boundaries with RQ to catch errors but I don't need that now. Even the ClientError will be most likely used for routes with params in the URL so you can handle a not found, a bad request is most likely to happen only in actions and you can return the errors directly and call useActionData to retrieve it. |
Beta Was this translation helpful? Give feedback.
-
Another possible name for the exported component could be UserError so it's not confused with client-side error, ClientError is still nice tho because it's how 4xx status codes are called so it's more similar to the standard. |
Beta Was this translation helpful? Give feedback.
-
Quick question. What happens in the |
Beta Was this translation helpful? Give feedback.
-
When uncaught exceptions are thrown in loaders, actions, or during render, Remix emulates
componentDidCatch
and renders theErrorBoundary
for the route where the exception was thrown. If there is no boundary there, it "bubbles" up to the nearest parent error boundary.This behavior could also be useful for:
The API would be very simple: you just throw from your loader/action.
Not Found
Like ErrorBoundaries, if this route doesn't export a
NotFound
then the nearest parent'sNotFound
will render instead, all the way to theroot.tsx
.Redirect
Redirects would work the same way, instead of returning them, you can throw them. For example, you could protect a route from visitors without a user session like this:
Of course, this isn't really different than today except that you could "throw" instead of return. It's not obvious at first but this makes abstractions that may redirect far nicer! Instead of the callback flying-v, your code could be much cleaner:
The Gotcha! Apps have to rethrow
There's one big gotcha here. I experimented with throwing redirects in @reach/router and it was pretty nice, but the problem is that applications need to be mindful to rethrow any exceptions they caught.
If the app doesn't rethrow a special Remix exception then the whole API breaks.
This was particularly problematic in @reach/router because all of this was happening in the render tree so every application-level
componentDidCatch
had to deal with it. We've thought about this API for Remix for a while but my memory of the problems in @reach/router made me shy away from it.However, I don't think it's as big a risk in Remix, since we aren't throwing in the render tree, we're always just one level away from where the exception needs to go. I think this drastically reduces the risk that apps will accidentally swallow these special exceptions. Additionally, since we already have error boundaries for uncaught exceptions, apps aren't very inclined to wrap the code in their loaders with try/catch, it's automatic in Remix.
I think the most common try/catch in a loader is unlikely to be a problem because it's usually not wrapping things that will throw special Remix exceptions like
getUserSession
orremoveTrailingSlash
. It's usually some other API after those things have been dealt with. For example, some database APIs throw when the record isn't found, that would look like this:Links export
The links export will need to know about this in case apps want to load different resources for the not found branch. Simplest thing would be a boolean to the links function:
Not Found Status Codes
Not found should probably take any 4xx status code.
Technically these are "client errors" (and 5xx are server errors). So instead of "Not Found" we might want to be more pedantic:
Benefits
NotFound
at any level of the route tree{ notFound: true}
to the componentRelated: #203
Beta Was this translation helpful? Give feedback.
All reactions