router icon indicating copy to clipboard operation
router copied to clipboard

refactor(react-router): `never` return type for `notFound`

Open RobinDaugherty opened this issue 1 year ago β€’ 5 comments

Currently, when you call notFound({ throw: true }) Typescript does not identify it as never returning.

A discussion of this issue: https://github.com/TanStack/router/discussions/2168

Here is an example:

export const Route = createFileRoute('/$tenantId')({
  loader: async ({ context, params: { tenantId } }) => {
    const result = await context.getTenantInfo()
    if (!result) {
      throw new Error('Got no result')
    }
    if (result.error) {
      throw result.error
    }
    if (!result.data.tenant) {
      notFound({ throw: true })
    }
    return result.data.tenant
  },
  component: TenantLayout,
})

function TenantLayout() {
  const tenant = Route.useLoaderData() // tenant is possibly null or undefined
}

Typescript infers the return type of the loader function in this case to be either a data structure or undefined or null. Since the type being returned is a subset of another object, defining an explicit type is not very efficient.

Fortunately Typescript can be given a hint that the notFound function will never return, and in that case will infer a non-nullable return type.

Because the notFound function only raises an exception when the throws option is true, this is expressed as an overload of the function. The overloads tell Typescript the concrete return types depending on options passed, returning never when throws is true, and returning a NotFoundError in other cases.

With this in place, the result of Route.useLoaderData() is a non-nullable data structure.

(The throws: true implementation is desired when using eslint, since it will complain about throwing a NotFoundError because it does not inherit from Error. By asking the function to throw instead, we can bypass the eslint complaint. The complaint is otherwise desirable, but the internal implementation of the router throws and catches these specific NotFoundError objects, and our application should not catch one, so the eslint complaint is not applicable.)

RobinDaugherty avatar Sep 02 '24 10:09 RobinDaugherty

returning notFound is deprecated, throwing is the way to use it. same goes for throw: true, this was just addded because of at that time the loader type inference could not handle functions that would only throw and never return any "real" value. we need to mark this as deprecated in code and documentation.

cc @chorobin

schiller-manuel avatar Sep 02 '24 17:09 schiller-manuel

returning notFound is deprecated, throwing is the way to use it.

😒 oh no that is really sad. Throwing non-exceptions is IMHO a big code smell. There is not much difference to a goto and it impossible to type the thrown value properly, so it always requires runtime validation when catching those errors. I know a lot of other frameworks do it in the same way, but I don’t think that makes it better.

lo1tuma avatar Sep 03 '24 06:09 lo1tuma

it impossible to type the thrown value properly, so it always requires runtime validation when catching those errors.

you as a library user do not need to handle those exceptions, it's handled by router. no need to add any "runtime validation" on your side.

schiller-manuel avatar Sep 03 '24 18:09 schiller-manuel

you as a library user do not need to handle those exceptions

Well, that is not quite true. As a user of the library I’m in charge of the code that throws this weird value. Since I want to unit test the code that I’m in control of I need to somehow catch this value and verify that it is the expected value.

lo1tuma avatar Sep 04 '24 06:09 lo1tuma

Note that auto-throwing can allow for more elegant code in some cases:

const post = await getPost() ?? notFound({ throw: true })

I would be sad if I was no longer able to do that.

rijk avatar Oct 09 '24 07:10 rijk

Is there a reason not to accept this change? I understand you want to mark one of these deprecated, but I don't think that conflicts with this change. Even if you deprecate it, the types I'm adding are correct for the extant behavior.

RobinDaugherty avatar Mar 26 '25 13:03 RobinDaugherty

sorry this got lost in the PR stack

schiller-manuel avatar Mar 27 '25 00:03 schiller-manuel

View your CI Pipeline Execution β†— for commit 1818104eae240f75990f0e80d88b7dea163248be.

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ❌ Failed 1m 28s View β†—
nx run-many --target=build --exclude=examples/*... ❌ Failed 5s View β†—

☁️ Nx Cloud last updated this comment at 2025-03-27 00:38:42 UTC

nx-cloud[bot] avatar Mar 27 '25 00:03 nx-cloud[bot]

@schiller-manuel can you please clarify? You wrote

returning notFound is deprecated, throwing is the way to use it.

I'm a bit confused by the phrasing of this. Are you saying that the notFound variant that throws (and therefore would have a never return type) is deprecated? Or the variant that returns is deprecated?

It seems from your other statement

you as a library user do not need to handle those exceptions

that you intend that the library does the throwing and the catching, and application code should not involved in that process. Which sounds to me like you mean to deprecate the variant that returns. But the rest of this discussion seems to be based on the opposite interpretation.

RobinDaugherty avatar Jul 07 '25 09:07 RobinDaugherty