Skip to content

Turn error and redirect into commands, rather than functions returning a value we have to throw #11144

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

Closed
elliott-with-the-longest-name-on-github opened this issue Nov 29, 2023 · 4 comments · Fixed by #11165
Milestone

Comments

@elliott-with-the-longest-name-on-github
Copy link
Contributor

Describe the problem

Throwing error and redirect has always been a bit weird, and people have complained about it before, but it is necessary -- both of them are meant to stop the normal control flow and start something new, which is kinda what throw does. Remembering that you have to throw them, though, is strange, and it doesn't feel like a great developer API. Thankfully, TypeScript gives us what we need to have the best of both worlds:

function redirect(): never {}

A function with a return type of never, well, never returns. TypeScript treats it like a thrown error. In the following code:

const foo = 'bar';
redirect('/hello-world');
console.log('hello, world');

TypeScript would grey out the last line, knowing that it could never be reached. This is ultra-simple to implement, and would basically just mean changing the implementation of the current redirect and error functions like so:

// old
function redirect() {
  return new Redirect();
}

// new
function redirect(): never {
  throw new Redirect();
}

Describe the proposed solution

Both combined above.

Alternatives considered

Leaving it as-is. The main downside here is that users can't do something like:

const e = error(401);
throw e;

but the only reason they can do this today is because we didn't think of the above before this feature was released -- the actual redirect and error classes are meant to be implementation details not exposed to the user.

Importance

nice to have

Additional Information

No response

@Conduitry
Copy link
Member

I like the throw being there to remind you that this does indeed work by throwing an exception. I've at least once wondered why my code wasn't working, and it was because my throw redirect() was inside a try block. That's going to be even more likely to cause confusion when the user doesn't actually see the throw anymore.

@elliott-with-the-longest-name-on-github
Copy link
Contributor Author

I'm not sure that'll really be a problem in practice; this is how Next's similar directives work (at least, from a typing perspective) and I haven't seen any similar feedback from their users.

Additionally -- is there any reason this feature has to work via error throwing other than that it was convenient?

@bever1337
Copy link

bever1337 commented Nov 30, 2023

Additionally -- is there any reason this feature has to work via error throwing other than that it was convenient?

I think this is the core issue. My two cents -- not throwing is preferable to misdirecting the throw. Returning the value of redirect should suffice.

Having to allow some throws makes it difficult to enhance and compose server actions, ex: logging middleware. JS provides limited pattern matching, so it's non-trivial for the developer to assert "Oh ok, I should re-throw this error and not log it."

@Rich-Harris
Copy link
Member

is there any reason this feature has to work via error throwing other than that it was convenient?

it's because of this:

function load(event) {
  do_some_auth_stuff_or_whatever(event);

  // ...
}

if we had to return a redirect or error then you'd have to do this:

function load(event) {
  const result = do_some_auth_stuff_or_whatever(event);

  if (result) {
    return result;
  }

  // ...
}

but then what if it can return something other than a redirect/error, like some details about the user? you'd have to determine that that was the case... and before you know it your simple reusable helper is just a PITA to use.

JS provides limited pattern matching, so it's non-trivial for the developer to assert "Oh ok, I should re-throw this error and not log it."

@tcc-sejohnson and I were just talking about exposing helpers like this...

function isHttpError(error: any, status?: number): error is HttpError {
  return error instanceof HttpError && (status ? error.status === status : true);
}

...so that you can do this sort of thing:

try {
  whatever();
} catch (e) {
  if (isHttpError(e, 404)) {
    // ...
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants