breaking: change form.error type from any to unknown - #16245
Conversation
Went through the `any` types on `public.d.ts` and these are the only changes that feel necessary. All the other are either ok (because really "any", or part of generics", or internal. Closes #15674
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/f3344e9db9b3f8e77a7bf0c8bf8b85313605e65eOpen in |
🦋 Changeset detectedLatest commit: f3344e9 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 |
| export type RemoteResource<T> = Promise<T> & { | ||
| /** The error in case the query fails. Most often this is a [`HttpError`](https://svelte.dev/docs/kit/@sveltejs-kit#HttpError) but it isn't guaranteed to be. */ | ||
| get error(): any; | ||
| get error(): unknown; |
There was a problem hiding this comment.
as a follow-up (not for this PR) I think we could probably tighten this up — if it's not an App.Error then surely it can only be a NetworkError?
There was a problem hiding this comment.
actually... maybe it should be part of this PR. feels like we could simplify usage a fair bit, including in the q.error test samples below. Do network errors go through handleError? If not, should they? If so, is there a sensible way to represent a network error as an App.Error?
There was a problem hiding this comment.
good call - this makes more sense. The promise will stay as-is largely though, mainly so that our transformError will not transform it again.
…n the query rejection handler lets a superseded request clobber a newer successful result's state with a stale error.
This commit fixes the issue reported at packages/kit/src/runtime/client/remote-functions/query/instance.svelte.js:130
## Bug
In `Query.#run()`'s `.catch` handler, the supersede guard was changed from synchronous to `async`:
```js
.catch(async (e) => {
const idx = this.#latest.indexOf(resolve);
if (idx === -1) return;
const error = await handle_error(e, { ... }); // <-- async gap
untrack(() => {
this.#latest.splice(0, idx).forEach((r) => r(undefined));
this.#error = error;
this.#loading = false;
});
reject(new HttpError(error.status, error));
});
```
`#latest` is a shared array across concurrent `#run()` invocations. `refresh()` spawns new `#run()` calls that each push a resolver onto `#latest`, so multiple in-flight requests are expected and normal.
### Concrete trigger
1. Request **A** starts → `#latest = [resolveA]`.
2. `refresh()` starts request **B** → `#latest = [resolveA, resolveB]`.
3. A's underlying `#fn()` rejects → enters `.catch`, captures `idx = 0`, passes the `idx === -1` guard, then suspends on `await handle_error(...)`.
4. During that await, B's `#fn()` resolves (synchronous success path): it splices A out of `#latest`, sets `#ready = true`, `#raw = B's value`, `#error = undefined`. Now `#latest = [resolveB]`.
5. A resumes with the **stale** `idx = 0`. `splice(0, 0)` removes nothing, but it then sets `this.#error = error` (A's stale error) and `#loading = false`.
Result: `#error` is set even though the latest refresh (B) succeeded, while `#raw` still holds B's value. The UI shows an error for a query that actually succeeded.
The synchronous success path never had this problem because there is no await between its `indexOf` guard and the state mutation — the invariant "this request is still current" held atomically. The async rejection path broke that invariant.
## Fix
Re-check `this.#latest.indexOf(resolve)` **after** the `await handle_error(...)` and bail out (`return`) if it's now `-1`. This restores the synchronous path's invariant: a request superseded during error handling correctly abandons its state mutation instead of clobbering a newer result. An early pre-await check is kept to avoid the (potentially costly) `handle_error` call for an already-superseded request.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: Rich-Harris <hello@rich-harris.dev>
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/kit@3.0.0-next.7 ### Major Changes - breaking: change `form.error` type from `any` to `App.Error | undefined` ([#16245](#16245)) - feat: allow adapters to provide additional Vite plugins ([#16206](#16206)) - breaking: move tracing out of the experimental namespace and remove the instrumentation flag ([#16260](#16260)) - breaking: remove `$app/stores` ([#15499](#15499)) - breaking: disallow `*.remote.ts/js` files unless `experimental.remoteFunctions` is enabled ([#16247](#16247)) - breaking: remove param files in folder in favor of `params.js/ts` file ([#16189](#16189)) ### Patch Changes - fix: defer `query.refresh()` in server commands until after the command body completes ([#16225](#16225)) - fix: resolve service worker and `tsconfig.json` based on Vite `root` setting ([#16229](#16229)) - fix: populate `$app/env/*` dynamic variables in contexts that don't run the dev server, such as `vite-node` ([#16223](#16223)) - fix: no longer throw "An impossible situation occurred" when a server-only module is imported by both server and client code ([#16257](#16257)) - fix: set `define` values on `globalThis` when running Vitest ([#16246](#16246)) - fix: serve `.ico` files with `image/x-icon` Content-Type ([#16234](#16234)) - fix: make `paths.origin` type looser ([#16215](#16215)) - fix: avoid client build warning about externalising `node:async_hooks` ([#16244](#16244)) - fix: allow reserved words (e.g. `delete`, `class`) as remote function export names ([#16264](#16264)) ## @sveltejs/adapter-netlify@7.0.0-next.2 ### Patch Changes - fix: ensure types for `platform.context` work ([#16255](#16255)) - Updated dependencies [[`ba78a0b`](ba78a0b), [`a248c9b`](a248c9b), [`7596981`](7596981), [`be72ed9`](be72ed9), [`a5bd7e2`](a5bd7e2), [`dec671f`](dec671f), [`82f3867`](82f3867), [`97eb324`](97eb324), [`e2a9ac0`](e2a9ac0), [`309bfb4`](309bfb4), [`522a86b`](522a86b), [`0ff547f`](0ff547f), [`3d4ff91`](3d4ff91), [`c925f2a`](c925f2a), [`7daf445`](7daf445)]: - @sveltejs/kit@3.0.0-next.7 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
The
errorproperty onRemoteResource(and by extensionRemoteQuery,RemoteLiveQuery, andRemotePrerenderFunctionresults) is now typed asApp.Error | undefinedinstead ofany. All errors are now transformed throughhandleErrorbefore being surfaced on the.errorproperty, matching how the server already serializes errors. This means.erroris always anApp.Error-shaped object ({ message: string, status: number, ... }) rather than potentially being anHttpErrorinstance, a nativeError, or aTypeErrorfrom network failures.If you were using
isHttpError(resource.error)to check the error shape, replace it with a simple truthiness check and access.status/.messagedirectly:Went through the
anytypes onpublic.d.tsand these are the only changes that feel necessary. All the other are either ok (because really "any", or part of generics", or internal.Closes #15674