breaking: allow handleError to influence status code - #16162
Conversation
...by returning `{ status: ..., ... }` from `handleError`.
Breaking change because theoretically someone could've used this as part of `App.Error` before, and now it's kind of outside that, a reserved property on `handleError`'s return object.
Closes #14442
🦋 Changeset detectedLatest commit: 196812d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
|
/autofix |
Co-authored-by: Tee Ming <chewteeming01@gmail.com>
| status: number; | ||
| message: string; | ||
| }) => MaybePromise<void | App.Error>; | ||
| }) => MaybePromise<void | (App.Error & { status?: number })>; |
There was a problem hiding this comment.
what if instead of doing it like this, App.Error always had a status property? we would update the error(...) helper like so:
// just a status, default message, works as today
error(418);
// status and message, works as today
error(418, `I'm a teapot`);
// pass an `App.Error` object — unlike today, omit the first argument
error({
id: crypto.randomUUID(),
status: 418,
message: `I'm a teapot`
});There was a problem hiding this comment.
I suppose another option for the custom App.Error case would be this, if it's possible type-wise
error(418, `I'm a teapot`, {
id: crypto.randomUUID()
});There was a problem hiding this comment.
Agree on adding the status property to our interface. Prevents people from overriding it in app.d.ts with a different type (ours takes precedence and causes the type in app.d.ts to be ignored). Not sure how to get it to error at the interface so that folks know it doesn't take effect. I guess that becomes obvious if they do try to assign a value to it.
There was a problem hiding this comment.
if it's possible type-wise
just took a run at this
type Custom = App.Error extends { status: number; message: string }
? { status: number; message: string } extends App.Error
? false
: true
: never;
declare function error(
...args: Custom extends true
? [status: number, message: string, body: Omit<App.Error, 'status' | 'message'>]
: [status: number, message: string]
): never;
Rich-Harris
left a comment
There was a problem hiding this comment.
blocking pending resolution of #16162 (review)
…rapper, so async `handleValidationError` hooks no longer type-check even though the runtime awaits the result.
This commit fixes the issue reported at packages/kit/src/exports/public.d.ts:972
## Bug
In commit `60cd975`, the `HandleValidationError` type in `packages/kit/src/exports/public.d.ts` was changed:
```ts
- (input: { issues: Issue[]; event: RequestEvent }) => MaybePromise<App.Error>;
+ (input: { issues: Issue[]; event: RequestEvent }) => AppErrorWithOptionalStatus;
```
The switch from `App.Error` to `AppErrorWithOptionalStatus` is intentional (status is now optional — the runtime does `error(body.status ?? 400, body)`). But dropping the `MaybePromise` wrapper is a regression.
### Why it's a bug / failure mode
The runtime explicitly awaits the hook result in `packages/kit/src/runtime/app/server/remote/shared.js`:
```js
const body = await state.handleValidationError({ issues: result.issues, event });
error(body.status ?? 400, body);
```
So returning a `Promise` works at runtime. With the type no longer permitting a `Promise`, a user with an async hook gets a TypeScript error:
```ts
export async function handleValidationError({ issues }) {
return { message: await translate(issues[0].message) };
}
```
This is inconsistent with the sibling hooks `HandleServerError` / `HandleClientError`, which use `MaybePromise<void | AppErrorWithOptionalStatus>`, and the type had been `MaybePromise<App.Error>` since its introduction.
## Fix
Restore the `MaybePromise` wrapper:
```ts
(input: { issues: Issue[]; event: RequestEvent }) => MaybePromise<AppErrorWithOptionalStatus>;
```
Applied to both `packages/kit/src/exports/public.d.ts` (line 972) and the generated `packages/kit/types/index.d.ts` (line 945) to keep them in sync. `MaybePromise` is already imported/in scope in both files (used by the sibling hooks).
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: dummdidumm <sholthausen@web.de>
There was a problem hiding this comment.
Additional Suggestions:
- Hydrating an SSR'd error page yields
page.status === 200instead of the real error status (e.g. 404/500) because_hydratedefaultsstatusto200, defeating theerror?.statusfallback.
- Statusless action errors (e.g. a network failure during a progressively-enhanced form submit) now render the nearest
+error.sveltewithpage.status === 200instead of the previous500, conflating a failed submission with a success status.
…ad of the real error status (e.g. 404/500) because `_hydrate` defaults `status` to `200`, defeating the `error?.status` fallback.
This commit fixes the issue reported at packages/kit/src/runtime/client/client.js:3013
## Bug
After commit `9b8da37`, `status` became derivable from `error.status`:
- `get_navigation_result_from_branch` (client.js:874) computes `status: status ?? error?.status ?? 200`.
- The server (render.js:508) only serializes `status` into the hydrate options when there is **no** error:
```js
if (status !== 200 && !error) {
hydrate.push(`status: ${status}`);
}
```
So for an SSR'd error page, `status` is omitted and only `error` (which carries `.status`) is serialized.
However, `_hydrate` (client.js:3013) destructured the options with a default:
```js
{ status = 200, error, ... }
```
When the server omits `status` (the error-page case), `status` defaults to `200`. That `200` is then passed to `get_navigation_result_from_branch` (client.js:3082), where `status ?? error?.status ?? 200` evaluates to `200` (since `200` is not nullish), so `error.status` is never consulted.
**Trigger:** SSR a 404 or 500 error page and hydrate it — `page.status` becomes `200` instead of the actual error status. The other four callers of `get_navigation_result_from_branch` (lines 1373, 1392, 1498, 2653) omit `status` entirely, so their `error?.status` fallback works; only `_hydrate` broke it via the `= 200` default.
## Fix
Removed the `= 200` default in `_hydrate`'s destructuring so `status` is `undefined` when not serialized, allowing the `error?.status` fallback to take effect.
All three paths remain correct:
- **Error page:** `status` omitted → `undefined ?? error.status ?? 200` = `error.status` ✅ (now fixed)
- **Normal page:** `status` omitted, no error → `undefined ?? null?.status ?? 200` = `200` ✅
- **Form `fail`:** `status` serialized, `error` null → serialized status is used ✅
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: Rich-Harris <hello@rich-harris.dev>
|
agree with the first vercel bot suggestion, not sure about the second |
|
The second point is correct, too, but not its fix. Pushing something up. |
…ted through handle_error; narrow the error type
Install latestInstall the latest version of With spi @sveltejs/kit --commit 196812d140b7765f0b9856073b7708f910971d5aWith pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/196812d140b7765f0b9856073b7708f910971d5aCommit URL: https://pkg.svelte.dev/@sveltejs/kit/c/196812d140b7765f0b9856073b7708f910971d5a |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to version-3, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `version-3` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `version-3`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @sveltejs/adapter-node@6.0.0-next.2 ### Major Changes - breaking: add `kit.paths.origin` config option, remove `kit.prerender.origin` and the `adapter-node` `ORIGIN` environment variable ([#16161](#16161)) ### Patch Changes - Updated dependencies [[`3c434fb`](3c434fb), [`a9284e8`](a9284e8), [`3726a7a`](3726a7a), [`f9d2240`](f9d2240), [`a9284e8`](a9284e8), [`7c040ba`](7c040ba), [`223eaad`](223eaad), [`223eaad`](223eaad), [`3b907d4`](3b907d4), [`fd628a5`](fd628a5), [`223eaad`](223eaad), [`6c1d035`](6c1d035), [`178eac0`](178eac0), [`c6562a9`](c6562a9), [`8eca2ab`](8eca2ab), [`61cf188`](61cf188)]: - @sveltejs/kit@3.0.0-next.6 ## @sveltejs/kit@3.0.0-next.6 ### Major Changes - breaking: return no content for 204 responses ([#16200](#16200)) - breaking: form action responses now use the HTTP status code returned from `fail` ([#16200](#16200)) - breaking: nested server-only directories ([#15685](#15685)) - breaking: add `kit.paths.origin` config option, remove `kit.prerender.origin` and the `adapter-node` `ORIGIN` environment variable ([#16161](#16161)) - breaking: don't abort navigation when calling `invalidate(All)` during navigation ([#16188](#16188)) - breaking: allow `handleError` to influence status code ([#16162](#16162)) - breaking: forbid external redirects by default ([#16198](#16198)) ### Minor Changes - feat: use `type: 'module'` for service worker registrations ([#16169](#16169)) - feat: add `dirty()` property to form fields ([#16208](#16208)) - feat: add `cookies.parse` method ([#16203](#16203)) ### Patch Changes - fix: drain unconsumed request bodies so keep-alive connections don't hang ([#16170](#16170)) - fix: properly handle Date objects in form.fields.set ([#16168](#16168)) - fix: skip clean fields when programmatically validating forms ([#16208](#16208)) - breaking: experimental remote form `validate({ includeUntouched })` option is now `all` ([#16208](#16208)) - fix: return `undefined` from `fields.branch.issues()` when only `fields.branch.leaf` has issues ([#16187](#16187)) - feat: add field.touched() helper to remote form fields ([#14692](#14692)) ## @sveltejs/adapter-cloudflare@8.0.0-next.1 ### Patch Changes - fix: avoid overriding user's existing `_headers` rules ([#16183](#16183)) - Updated dependencies [[`3c434fb`](3c434fb), [`a9284e8`](a9284e8), [`3726a7a`](3726a7a), [`f9d2240`](f9d2240), [`a9284e8`](a9284e8), [`7c040ba`](7c040ba), [`223eaad`](223eaad), [`223eaad`](223eaad), [`3b907d4`](3b907d4), [`fd628a5`](fd628a5), [`223eaad`](223eaad), [`6c1d035`](6c1d035), [`178eac0`](178eac0), [`c6562a9`](c6562a9), [`8eca2ab`](8eca2ab), [`61cf188`](61cf188)]: - @sveltejs/kit@3.0.0-next.6 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…, message, {...})` (#16540)
The TODO from
#16162 (comment) —
`error(status, {...})` is now deprecated in favour of passing any
additional properties as the third argument
---
### Please don't delete this checklist! Before submitting the PR, please
make sure you do the following:
- [x] 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
- [x] This message body should clearly illustrate what problems it
solves.
- [ ] Ideally, include a test that fails without this PR but passes with
it.
### Tests
- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint` and `pnpm check`
### Changesets
- [x] 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. Changesets that add features should be
`minor` and those that fix bugs should be `patch`. Please prefix
changeset messages with `feat:`, `fix:`, or `chore:`.
### Edits
- [x] Please ensure that 'Allow edits from maintainers' is checked. PRs
without this option may be closed.
---------
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
…ry.live` remote functions (#16529) #16162 removed `ServerErrorNode.status` (the status now lives on `App.Error`) and updated the readers in `shared.svelte.js`, leaving two behind in `query.live`'s iterator and the prerendered-response reader. This ports them to the same shape and makes the type assertion on the prerendered response actually apply (it was unparenthesized, so the stale read never failed `pnpm check`). The prerendered-error branch still falls into the surrounding catch and re-runs the function, which is a separate issue. --------- Co-authored-by: Tee Ming <chewteeming01@gmail.com>
… error page (#16528) #16162 made the status returned from `handle_error_and_jsonify` authoritative on two of `respond_with_error`'s three exits. #16374 later turned the `x-sveltekit-error` early return into a hook-calling exit but kept the caller's pre-hook status, so a `handleError` that returns `{ status }` is honored everywhere except there. Using `transformed.status` makes the `status` argument dead, so this also removes it and the three caller-side status arguments.
…non-ActionResult error response (#16308) closes #15737 Submitting a `use:enhance` form that trips the CSRF origin check does nothing visible. The 403 response is right there in the network tab: ```json { "message": "Cross-site POST form submissions are forbidden" } ``` but it has no `type`, so it isn't an ActionResult and every branch in the submit handler and `applyAction` skips it. Non-JSON responses already become `{ type: 'error' }` through the catch around `deserialize`, so JSON that isn't an ActionResult was the one shape that failed silently. Error responses without a recognized `type` now throw into that same catch and render the nearest `+error.svelte`. A body shaped like an `App.Error` becomes `page.error` as-is, the way an `error(403, { message })` body does. Anything else goes through `handleError`, which #16162 routed this catch through, so the hook keeps seeing these failures and `page.error` keeps its declared shape. 2xx responses are untouched. PatrickG suggested rendering the error page in the issue. teemingc flagged the same gap in #10464 with a server-side shape fix in mind; doing it on the client also covers proxy and middleware responses that kit's server never shaped. #10855 reports the same class of unhelpful failure for non-action endpoints; the non-2xx half of it is covered here. Responses that do parse as an ActionResult pass through regardless of status, which keeps the pattern that prompted the #13197 revert (#13397) working. The docs line that revert added says posting to a `+server.js` endpoint results in an error; with this change that error surfaces instead of failing silently. The test mimics the CSRF response with an endpoint, since the real check can't fire same-origin in Playwright. It fails on `version-3` and passes with this change, in dev and build. The hook suffix in two of the assertions is `handleError` running. --- ### Please don't delete this checklist! Before submitting the PR, please make sure you do the following: - [x] 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 - [x] This message body should clearly illustrate what problems it solves. - [x] Ideally, include a test that fails without this PR but passes with it. ### Tests - [x] Run the tests with `pnpm test` and lint the project with `pnpm lint` and `pnpm check` ### Changesets - [x] 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. Changesets that add features should be `minor` and those that fix bugs should be `patch`. Please prefix changeset messages with `feat:`, `fix:`, or `chore:`. ### Edits - [x] Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed. --------- Co-authored-by: Nic Polumeyv <nicolas.polum@gmail.com> Co-authored-by: Rich Harris <rich.harris@vercel.com>
...by returning
{ status: ..., ... }fromhandleError.Breaking change because theoretically someone could've used this as part of
App.Errorbefore, and now it's kind of outside that, a reserved property onhandleError's return object.Closes #14442