Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Throwing Special Exceptions in loaders/actions #204

Closed
ryanflorence opened this issue Jun 19, 2021 · 19 comments
Closed

Throwing Special Exceptions in loaders/actions #204

ryanflorence opened this issue Jun 19, 2021 · 19 comments

Comments

@ryanflorence
Copy link
Member

ryanflorence commented Jun 19, 2021

When uncaught exceptions are thrown in loaders, actions, or during render, Remix emulates componentDidCatch and renders the ErrorBoundary 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:

  • Not Found
  • Redirects

The API would be very simple: you just throw from your loader/action.

Not Found

import { notFound } from "remix";

export function loader({ params }) {
  let project = await db.projects.find(params.projectId);

  if (!project) {
    throw notFound();
  }

  return json(project);
}

// this would render
export function NotFound() {
  let params = useParams();
  return <h1>No project for {params.projectId}.</h1>
}

Like ErrorBoundaries, if this route doesn't export a NotFound then the nearest parent's NotFound will render instead, all the way to the root.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:

import { getUserSession } from "../sessions";

export function loader({ request }) {
  let userSession = await getUserSession(request);

  if (!userSession) {
    // return redirect("/login");
    throw redirect("/login");
  }

  return null;
}

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:

// instead of creating abstractions with a "push" API
export function loader({ request }) {
  return removeTrailingSlash(request, () => {
    return withUserSession(request, session => {
      let projects = await db.projects.findMany({ where: { userId: session.uid } });
      return json(projects);
    }); 
  })
}

// we could create abstractions with a "pull" API which are typically easier to work with
// (like render props -> hooks)
export function loader({ request }) {
  removeTrailingSlash(request);
  let session = await getUserSession(request);
  let projects = await db.projects.findMany({ where: { userId: session.uid } });
  return json(projects);
}

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.

export function loader({ request }) {
  try {
    removeTrailingSlash(request);
    let session = await getUserSession(request);
    let projects = await db.projects.findMany({ where: { userId: session.uid } });
    return json(projects);
  } catch (e) {
    // app has to rethrow if it's a special remix exception.
    if (isRemixException(e)) {
      throw e;
    }
    return json(e.message, { status: 500 });
  }
}

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 or removeTrailingSlash. 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:

export function loader({ request, params }) {
  // these could throw a special remix exception and everything is fine
  // because the app didn't catch these
  removeTrailingSlash(request);
  let session = await getUserSession(request);

  // after that though, it's unlikely these APIs will be throwing special
  // remix exceptions, so we're good.
  try {
    return json(await db.projects.find(params.id));
  } catch (e) {
    // some DB exception API, not Remix
    if (e.code === "RECORD_NOT_FOUND") {
      // can use the new Remix notFound exception
      throw notFound();
    }

    // otherwise use the ErrorBoundary
    throw e;
  }
}

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:

export function links({ data, notFound }) {
  return notFound ? notFoundLinks : foundLinks;
}

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:

// probably what we'll do
throw notFound();
throw notFound(401):

// but if we wanted to be more pedantic about it:
throw clientError(); // default is 404
throw clientError(401);

Benefits

  • Cleaner loader/action code for any abstractions that redirect
  • Can remove the weird /404 route (especially nice since people are tempted to redirect to 404 which is not at all the point!)
  • Can handle not found cases in context, or at the root level of the app, by simply exporting a NotFound at any level of the route tree
  • Don't need a bunch of branching code in components/loaders and sending a { notFound: true} to the component

Related: #203

@alexblack
Copy link

alexblack commented Jun 19, 2021

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

import { notFound } from "remix";

export function loader({ params }) {
  let project = await db.projects.find(params.projectId);

  if (!project) {
    throw notFound();
  }

  return json(project);
}

export const meta = (data): MetaFunction => {
  return { title: `You found me here too: ${data.projectName}` };
}

export const links: LinksFunction = () => YouFoundMeLinks;

export default function ProjectView() => {
  const data = useRouteData();
  return (<span>You found me: {data.projectName}</span>);
}

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 NotFound, then you could use throw and/or return semantics.

@ryanflorence
Copy link
Member Author

would it make it possible for a route not to handle 404s

Yes, that's the bubbling, just like ErrorBoundary.

In your root.tsx export a NotFound component and you'll never need another one if you don't want contextual not found screens.

(There would no longer be a routes/404.jsx file, the "global not found handler" would just be the export on root)

@alexblack
Copy link

alexblack commented Jun 19, 2021

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 meta, links and a renderer (component).

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?

@ryanflorence
Copy link
Member Author

ryanflorence commented Jun 19, 2021

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? https://bbq.info/404 should be a valid URL (that's the area code in atlanta).

Additionally, a status code of 404 means the current URL is not found. If you switch the URL to /404 that's kinda misunderstanding HTTP.

You can branch in links as proposed for different assets.

@alexblack
Copy link

alexblack commented Jun 19, 2021

I think you're misunderstanding me... yes I agree the url should never be /404. I was just pointing out what I thought were the advantages of a global 404 handler, eg it gets a render component, its own styles, its own meta, and doesn't clutter up other routes.

(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...???)

@alexblack
Copy link

alexblack commented Jun 19, 2021

So i'm curious then, why does remix respond on the path /404? eg If I request the path /no-such-path I get my 404 page, which is great, and a 404 status code. But are you saying if I request /404 I get the 404 page too? why?

@ryanflorence
Copy link
Member Author

Yeah, that stuff is kinda nice, but then we'd have two ways to do it. We already have ErrorBoundary (which replaced routes/500.js many months ago). By handling both of these exceptions the same way, once you understand NotFound or ErrorBoundary, you immediately understand the other.

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).

@ryanflorence
Copy link
Member Author

But are you saying if I request /404 I get the 404 page too? why?

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 ErrorBoundary and replaced routes/500.js with it, it's been great.

Now we also know how to get rid of routes/404.js.

Remix responds to /404.js because we knew we were going to nix it eventually, so we didn't worry about the unintended behavior.

@alexblack
Copy link

alexblack commented Jun 19, 2021

By handling both of these exceptions the same way, once you understand NotFound or ErrorBoundary, you immediately understand the other.

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)

@alexblack
Copy link

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.

@ryanflorence
Copy link
Member Author

ryanflorence commented Jun 19, 2021

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 exceptions/{404,401,418,500}.js routes that rendered for any of those status codes.

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.

@kentcdodds
Copy link
Member

kentcdodds commented Jun 19, 2021

this handles more than 404

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 👍

@sergiodxa
Copy link
Member

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.

@kentcdodds
Copy link
Member

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.

@sergiodxa
Copy link
Member

🤔 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:

  • ClientError exported component catch any clientError(status: number) throw in the loader or action
  • ErrorBoundary exported component catch any non handled error in loaders, actions or the view, both server and client side
  • Custom error boundaries you may add in the view to catch client-side errors in the render or events or effects
  • Any try/catch for loader/action errors you want to handle

@kentcdodds
Copy link
Member

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.

@sergiodxa
Copy link
Member

sergiodxa commented Jun 22, 2021

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.

@sergiodxa
Copy link
Member

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.

@kentcdodds
Copy link
Member

Quick question. What happens in the links and meta exports for these kinds of errors? Will the data be undefined or something? Or are they just not called?

@remix-run remix-run locked and limited conversation to collaborators Mar 18, 2022
@chaance chaance converted this issue into discussion #2389 Mar 18, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants