Don't automatically throw from redirect and error

[cdcarson]

Describe the problem

I appreciate the thought & work that went into #11144 and #11165 but I think it's the wrong approach.

The confusion around return and throw arises from SvelteKit's original decision to stray from the common sense meanings of those words. I get there was a reason for this -- deriving action function return types, which can't be done if validation errors are thrown. But it led to a less than intuitive API...

• return fail(400, {bah: 'humbug'}) ...ack
• throw redirect(302, '/success') ...ack (it's a success, you should be able to return it)
• throw redirect(302, '/login') ...fair enough
• throw error(500) ...fair enough

Hiding the throw in the redirect and error cases doesn't really fix the problem. It just kicks the can down the road.

• It replaces a well-understood keyword with a SvelteKit abstraction that does the same exact thing. The fact that it does the same thing has to be remembered or looked up.
• The never works in an IDE. But you won't see the "unreachable code" lint if you're looking at something in a GitHub issue.

There's also a decent case to be made for...

const err = error(4xx, /** something complicated **/);
// throw it various places later...

Describe the proposed solution

Consider rolling it back.

Alternatives considered

Ideally, the error handling API should be re-written to be simpler and more intuitive. Stick to to the easy-to-understand paradigm of throwing Error and returning success, like so...

// I think these already exist, but making up the names...
import type {InvalidFormError, Redirect, ExpectedError} from '@svelte/kit';

// always thrown...
throw new ExpectedError(403, {message: 'You cannot do that.'});

// validation errors always thrown...
throw new InvalidFormError(400, {...});

// redirects are thrown or returned...
return new Redirect(302, '/success'); // as the return value of an action
throw new Redirect(302, '/login'); // thrown from an auth check in an action or load function

Granted, this leaves out how to type Action return values, given that validation errors are thrown. I get that this typing is a feature, but I'm not convinced that it's super valuable in practice. The shape of form (and the enhance result) on the client usually needs to narrowed anyway, between success and failure states, and between actions if a route has more than one. The generated types become cumbersome. It's just simpler to cheat...

// auth/+page.svelte
import type { ActionData } from './$types.js';
export let form: ActionData;
$: actionData = form as any; 
// passed to form components which have narrowed types...

// SignInForm.svelte
export let actionData: {data: SignInData, errors: {[k in keyof SignInData]?: string}};

// SignUpForm.svelte
export let actionData: {data: SignUpData, errors: {[k in keyof SignUpData]?: string}};

I suppose it's a matter of opinion whether generating action types is worth increasing the eccentricity and surface of the framework API. I'm fine either way. It's easy enough to wrap away the eccentricities.

Importance

nice to have

Additional Information

No response

[f15u]

If possible, I would completely remove the chance or need to throw something for control flow. There is a lot of literature that explains why exceptions are not ideal for control flow

Throwing something makes it very difficult to use other libraries in our backend logic. You have to add boilerplate to throw at the edge of the logic, resulting in more code, a non-linear flow, and patterns that are not widely used (like throwing for control flow itself).

My proposed solution is to have internal classes (like Response, Redirect, Error, and so on) and exported "constructors" (like response, redirect, error and so on) that hide the implementation detail of the classes that can be returned from loaders, actions and so on. Svelte can then pattern match the result, even with a simple instanceof, and decide what to do.

This would remove a lot of confusion about this pattern, use a well known approach by other frameworks, create reusable errors/redirects and simplify the interoperability with other libraries.

EDIT: seems that months ago this was the implementation and got changed.
Is it possible to have an issue, PR or anything where the rationale behind this change is explained? I'm struggling to find resources about this topic.

[cdcarson]

exceptions are not ideal for control flow

@FedericoBiccheddu As I understand it the anti-pattern is to abuse throw as a goto to jump around rather than using a more appropriate control structure.

In this case (both in the current API and in my proposed fix) throw is the appropriate control structure. When you do throw redirect(302, '/xyz) you're saying "Hey, Kit, I'm done, go do your thing."

[einarpersson]

I have to agree with this being confusing. If anything "fail" sounds like a command but "error" is a value. But I would really not loose the types (I don't know how it would affect awesome superforms which we are using)

I would prefer if we just used return for everything, and let throw be for unexpected server errors (500). But I don't see it likely to happen. But that is what I would like as it imo would be consistent and preserve type information. Errors as values. 👍

[f15u]

exceptions are not ideal for control flow

@FedericoBiccheddu As I understand it the anti-pattern is to abuse throw as a goto to jump around rather than using a more appropriate control structure.

In this case (both in the current API and in my proposed fix) throw is the appropriate control structure. When you do throw redirect(302, '/xyz) you're saying "Hey, Kit, I'm done, go do your thing."

You can achieve the same result with an early return. Using throw is literally using exception for control flow.

Exceptions are something you don't expect in your program/logic. Saying "I'm done, now framework will do the rest" is not an exception, is exactly what do you expect from it.

[cdcarson]

@FedericoBiccheddu Let's agree to disagree. I think it's fine, as a practical matter, to throw the ball over the API boundary between my code (I'm not catching it) and SvelteKit (who is.) You don't.

But for the sake of mutual understanding, let me ask a question. If we agree...

1. That you are right and the API should only account for unexpectedly thrown errors resulting from, say, a bad database query. Everything else, expected business logic errors (authorization, existence, etc), form validation errors, http redirects and plain old data, should be returned.
2. That a primary goal of the API is to generate accurate return types (in ./$types) for consumption in +page.svelte
3. That the current API relies on throw vs return to do (2)

Then how would you structure the API for load and action functions, sticking to (1) and getting rid of (3)?

(I realize after writing the above that it looks like a trick question. It's not. If you have an answer, I'd be genuinely interested. If not, that's fine too.)

[elliott-with-the-longest-name-on-github]

I don't have time to address everything here, but I did want to chime in on the "throw vs. redirect" thing -- I used to be in the "Wow, I really wish we could just return these" camp. Unfortunately, the ergonomics of that solution in reality are just untenable. It seems "obvious" that this should work:

export function load() {
  return redirect('/foo');
}

But what happens if you have this?:

export const load = withAuthentication(() => {
  // get stuff from the database, realize something failed
  return error(400);
})

Now what? withAuthentication has to check the return value of your route-level load function and decide whether or not it should return early (if load returned an error or redirect) or if it should continue on. In complex apps, you could have several layers of this. Not to mention, it's quite possible you want some code in /lib to be able to redirect users if, say, the user doesn't have access to a resource. Without throw, all of their return types would have to be Whatever | error, which would then have to be passed allllll the way back up the stack until you could "give it back" to SvelteKit. Gross. throw is the control-flow paradigm you want for redirects and errors.

[ITenthusiasm]

To answer the authentication scenario, another alternative is to bump authentication to hooks.server.ts. That's where I do my auth, and I prefer it there so that no unauthenticated actions accidentally slip through to a route inside. (This also saves me from having to add boiler plate to every route that I want protected.) I guess it depends on whether you want to be "safe first" or "open/public first". If this risks making the handle function too complex, the logic related to auth within hooks.server.ts could be split into sub functions that are responsible for different sets of routes within the app.

I think the "'Wow, I really wish we could just return these' camp" is valid. Developers should be allowed to accomplish whatever goals they need. I believe Remix (which seems to have inspired SvelteKit, SolidStart, etc.) actually does this. You can return or throw regular Response objects depending on your needs, or you can use their helper functions. This is a better DX than having a set of options stripped from you completely, as in the case of SvelteKit and Next.js. (I've found the latter even less convenient.)

It's really hard to predict every use case the developer needs to satisfy. @FedericoBiccheddu couldn't predict @tcc-sejohnson's use case/preference, who in turn likely can't predict another set of use cases for returning redirect (because we haven't each built the apps that have locked us down those paths). I actually have use cases for why I need to be able to return a regular Response object (which will be posted soon in a separate GH Issue).

Personally, I love Svelte. I think it's much better than React. But I like Remix more than SvelteKit because I find myself butting heads with SvelteKit too often to accomplish what I want when it comes to headers, response objects, and cookies. In Remix, I just say, "Use this Response please" and everything works.

That said, regardless of whether we're allowed to return Responses or not, I agree with @cdcarson's reasons for not throwing implicitly. I also agree that there are situations where it would make sense to throw (if there's a legitimate error).

[cdcarson]

This issue has turned somewhat philosophical. Totally my fault. I should have stuck to the facts in the original post, rather than succumbing to the temptation to air radical Jacobin opinions (or maybe retrograde ancien régime opinions, I dunno) about how the SvelteKit API should be.

Reset: A function that always throws is bad DX, anyway you cut it. Roll this change back.

[JonathonRP]

I've run into an issue today with redirect form actions and setting headers. I thought form actions would work like post server endpoint and returning a response with redirect settings would redirect but no get an error because form actions only expects data to return and using set headers redirect doesn't keep headers.

[ITenthusiasm]

@JonathonRP It seems that the problem you're describing is related to a different issue. Is that correct? If so, do you still want to leave your comment here? Or migrate it?

[JonathonRP]

I wanted to share my example of the API of redirect being inconsistent and confusing, and how that can lead to problems. In endpoints return redirect can be achieved multiple ways but I was not expecting form actions on post to work different then post even when using use: enhance
@ITenthusiasm

[DePasqualeOrg]

After migrating to SvelteKit 2, I get consistent-return errors from eslint in my SvelteKit projects, since redirect and error now implicitly throw. I've had to disable that rule until this is resolved. (I previously mentioned this in this discussion: #11467)

[cdcarson]

I can't replicate @DePasqualeOrg's eslint errors, but the example provided is instructive. Too often SvelteKit assumes that it is the be all and end all of the application, rather than a useful piece of an application with other important parts (for example, some database with a Prisma interface.) It bakes in decisions that would be better left to the developer.

In this particular instance, SvelteKit has hidden the throw keyword behind functions that always throw, without regard for whether other, perhaps less sophisticated, pieces of code can make sense of the nifty typescript that make it work in SvelteKit.

[DePasqualeOrg]

@cdcarson, to replicate the issue, just add consistent-return to the rules in your .eslintrc.cjs file if it's not already part of the rules you use. I use the Airbnb eslint rules, which include consistent-return.

[cdcarson]

@DePasqualeOrg Got it. I was using the SvelteKit default eslint, which I assumed had that rule, but maybe that's because I nearly always explicitly type function returns. This has more or less the same effect as the rule, complaining if there's a logical branch inside the function that doesn't return the sanctified type. More to the point, SvelteKit shouldn't add conventions that break cases like yours, especially on such apparently dubious grounds as those which led to this change.

[AlexRMU]

You can make it possible to return and throw.
Or make a wrapper for catching.
And write in the documents that it is recommended to use a return.