Tighten up error handling

[dummdidumm]

Describe the problem

#11278 sent me down a rabbit hole of error handling once more. It occured to me that us throwing errors is not correct, strictly speaking. Someone could've enhanced the App.Error object which means that more properties than just message are required, and as such calling error with just a string is wrong.

Describe the proposed solution

Instead of using error, we distinguish between unforseen fatal errors and not-so-bad errors by expanding on what was introduced in #11131:

• We create a NonFatalError with status and message (and NotFound introduced in that PR would go away)
• handleError is called with those, too, but our fallback uses the message verbatim in that case (because we know it doesn't contain sensitive information)
• We expose that class through @sveltejs/kit so people can do instanceof NonFatalError in handleError and inspect the status/message (would also make More information about error in HandleServerError #6774 more clear)/not send this to an error reporting service in that case
• If NonFatalError is discovered, the status is not 500 but instead the one defined in the instance

Alternatives considered

Leave this as is and document this somewhere in the code base

Importance

would make my life easier

Additional Information

Putting this on the 2.0 milestone because if we change this it could be seen as a breaking change (things appearing in handleError differently)

[alexbjorlig]

• so people can do instanceof NonFatalError in handleError and inspect the status/message

For me, this is key. In the future, HandleServerError should "always" be called and support something like this (in pseudo-code)

 export function handleError(...) {
   if (httpErrorCode is 500) {
     reportToSentry();
     return maskedError; // To hide vulnerable issue
   }
   
   return errorThatImplements App.Error
}

Benefits of this is:

• Developer can control (and expect) App.Error interface to always be true
• All errors go trough handleErrorso developers can choose and control what/if/when to report.

[dummdidumm]

So you're saying that throw error(..) should also go through handleError? That would contradict how the API is currently designed. throw error(..) is when you know that something is wrong but you can handle it explicitly / don't bother with going through handleError (and error is typed such that you are required to adhere to the App.Error interface). If you want this to be the case you could create your own error class and throw that, and instanceof check that in handleError.
As for the rest, seems we're in agreement.

[alexbjorlig]

That would contradict how the API is currently designed.

But in all modesty, the API today is not really that well designed. Ask 10 Sveltekit developers how it works today, and I think you get 5 different answers. Example:

• I throw an error 404 in my resolver: it will NOT go through handleError
• Sveltekit throws an error 404 for a route not found: it goes through handleError
• There is no HTTP status code concept in handleError, so we have to "infer" a 404 by doing !event.route.id - that's pretty weird. Sveltekit is about making web development streamlined 🤔

If developers are not bothered with their errors being reported, or implementing a custom shape in App.Error, they can just leave an empty implementation.

What is really the downside to always going through handleError - it would streamline error handling in Sveltekit so that both experienced and new developers can understand what's going on. It would also make it simple for Typescript to detect if errors are being "re-written" to match App.Error.

Imagine a future where the docs about error handling simply is:

All errors in Sveltekit go through the handleError hook. Here we provide you with a httpStatus code so you can decide to report to Sentry if needed - typically on 500. Use this function to mask the error, so you don't expose vulnerable information in production. The handleError hook is expected to return the error as an App.Error, so you can enforce a custom type if needed.

But I could be missing something here?

[Rich-Harris]

I've opened #11295 to revert the recent changes (sorry @alexbjorlig).

I agree that the current design isn't quite right, and we should take the opportunity of 2.0 to revisit it. Here's what I think should happen: handleError continues to be for unexpected errors only (if you call error(...), it's not unexpected), but we add a status code that defaults to 500 (but could also be e.g. 404 or 415).

If you want to log expected errors too, that's easy enough — just replace error(...) with your own logAndError(...) helper. That gives you more control than funneling everything through handleError and expecting developers to differentiate it themselves. An alternative would be to introduce a new hook (handleExpectedError? handleUserError?) but I'm not sure what value that would bring.

[alexbjorlig]

@Rich-Harris ok, I understand. It makes sense to think hard about the right design. With the impressive work done by the Sveltekit team, I'm sure we will get a good solution in 2.0. I'm here to offer my help writing tests, to cover these cases that we often find in our production Sveltekit app (mostly related to form actions).

Comments/questions about the 2.0 error design

I agree it's a good mental model to think about errors as being "unexpected" or "expected". But it is equally important to identify the HTTP error code; to help determine if this error should be reported to Sentry or not.

That was also my motivation for my 2 PRs - to solve the problem that "expected" errors were reported as "unexpected" errors.

Just one more comment:

funneling everything through handleError and expecting developers to differentiate it themselves.

This is exactly what Sveltekit developers are already doing today - we are "forced" to identify the severity by identifying event.route.id or regex the .message of the error. There is no other solution, to my knowledge, to identify a 404 not found or vulnerability scanner attempting to POST to a form action that does not exist.

[Rich-Harris]

see #11289, which addresses all these points

[alexbjorlig]

see #11289, which addresses all these points

The PR looks amazing - and it even includes the 2 tests 🤩 Can't wait for 2.0 now