Error handling fix

[cdcarson]

This fixes #4802, which was previously attempted in #4032.

Currently, when an error is thrown from an endpoint, the server ignores whether the request was the result of a fetch call from the client side. There are two cases where this matters: (1) a get request from the client router and (2) a fetch request from userland client code.

• In all cases, the server returns HTML from __error.svelte rather than the error as JSON.
• In the case of client-side navigation, __error.svelte ends up getting hydrated with a default error rather than the thrown error.
• In the case of a userland fetch, the HTML response is useless.

Two changes, as light-handed as possible:

1. In packages/kit/src/runtime/server/index.js we check for the two cases above, and if applicable, return the error as JSON. NB: Rather than worrying about serializing potentially complex or sensitive errors, it just returns an Error-like object {message: string}. IMO, this will take care of most userland use cases (where you just want to show the user what went wrong.) It doesn't preclude folks from throwing more complex error structures as JSON.

2. Additionally, a minor change was necessary in packages/kit/src/runtime/client/client.js.

There are three addition tests that cover off on the scenarios mentioned above. All the existing relevant tests seem to be passing, but I'm not sure how much coverage there was to begin with.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 83c6252

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@sveltejs/kit	Patch
test-basics	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Rich-Harris]

Alright, I got completely carried away with this PR, but I think it's done now. Thank you @cdcarson for getting the ball rolling, and thank you @georgecrawford for #4032 — apologies, I totally dropped the ball on that PR due to other things getting in the way.

In addition to turning thrown errors into JSON responses, this PR now does the same thing for explicitly returned errors. If the body of a request handler from a page endpoint is an Error, the error page will be shown (rather than validation errors being mixed in with get properties), as in this example adapted from #4032:

// post handler can return validation errors
export function post({ request, locals }) {
  if (!locals.user) {
    // fatal error — results in error page
    return {
      status: 401,
      body: new Error('show yourself, coward')
    };
  }

  if (!locals.user.mojo < 50) {
    // fatal error — results in error page
    return {
      status: 403,
      body: new Error('insufficient mojo')
    };
  }

  const [errors, item] = await create_item(await request.formData());

  if (errors) {
    // validation errors — mixed with GET props and rendered
    return {
      status: 400,
      body: { errors }
    };
  }

  // great success
  return {
    body: { item }
  };
}

Previously, the only way to trigger the error page in the first two cases (the 401 and 403) would have been to throw the error, which has two problems:

• the status will always be 500
• thrown errors go through handleError (i.e. they show up in logs etc) which probably isn't what you want

Turning errors into JSON involves being slightly opinionated. The name, message and stack properties are preserved (stack is neutered in production), unknown-but-enumerable properties (things like code or frame) are preserved, and if there's a cause property the process recurses. I didn't copy over the cyclical structure handling from #4032 as I think it's probably overkill — we don't do that for normal JSON responses, and I think it's a niche enough situation not to bother with — developers know how to debug stack overflows.

Since all this requires documentation, and since it was a bit awkward to fit into the existing docs, I overhauled the relevant section.

[cdcarson]

Thanks @Rich-Harris. (I especially enjoyed some of your commit comments.) This looks good.

[benmccann]

could refactor this into a reusable function since the other new code added here does the same thing

[Rich-Harris]

feels like unnecessary indirection to me — you add function call overhead, a new utility somewhere in the codebase, an import declaration per consuming module... you shave off a few characters at the callsite but make the codebase larger and arguably less easy to read

[Rich-Harris]

ah, you're talking about the whole block, not just the highlighted line. thought you mean is_error(body)

[Rich-Harris]

this is one of those cases where you basically can't DRY it out, because of the control flow (i.e. mutating an existing object and conditionally returning it). Or rather you can, but the resulting function is larger than the code you saved by deduplicating. i reckon it's probably not worth it

[benmccann]

it could use a comment explaining why we're doing a clone

[Rich-Harris]

added

[benmccann]

might not hurt to add descriptions for these two. I guess it's the accept header and types that we're okay using or something?

[Rich-Harris]

added

[georgecrawford]

Great stuff @Rich-Harris - thanks so much for getting to this, and no worries for the delay. I was going for longest-open-PR 🥇 !