breaking: review async APIs - #16352
Conversation
|
Install the latest version of pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/72b507e36c4ed6e107f8caace5690d46a818eca3Open in |
🦋 Changeset detectedLatest commit: 72b507e 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 |
| resolve: (event: RequestEvent, opts?: ResolveOptions) => MaybePromise<Response>; | ||
| }) => MaybePromise<Response>; | ||
| resolve: (event: RequestEvent, opts?: ResolveOptions) => Promise<Response>; | ||
| }) => Promise<Response>; |
There was a problem hiding this comment.
More often than not you'd call await resolve(event) inside the hook anyway. If you did do return resolve(event) it'd still return a Promise
EDIT: you could end up with:
export const handle = ({ event, resolve }) => {
if (...) {
return new Response(...);
}
return resolve(event);
}And then you'd needlessly have to add async to the function, so maybe we need to reconsider this
There was a problem hiding this comment.
yeah I think this does need to be MaybePromise for exactly that reason
| event: RequestEvent; | ||
| resolve: (event: RequestEvent, opts?: ResolveOptions) => MaybePromise<Response>; | ||
| }) => MaybePromise<Response>; | ||
| resolve: (event: RequestEvent, opts?: ResolveOptions) => Promise<Response>; |
There was a problem hiding this comment.
This is always await resolve() in apps so it makes sense to be a Promise
| // is running because subsequent invalidations may make earlier ones outdated, | ||
| // but batch multiple synchronous invalidations. | ||
| await (pending_invalidate ||= Promise.resolve()); | ||
| if (!pending_invalidate) return; |
There was a problem hiding this comment.
eslint thinks this is evaluating a promise which will always be true
…Response`, which is not assignable to the `Handle` type's `resolve` param that is now typed `(event, opts?) => Promise<Response>`, causing `tsc`/`pnpm check` to fail.
This commit fixes the issue reported at packages/kit/src/exports/hooks/sequence.spec.js:58
## Bug
`packages/kit/src/exports/public.d.ts` (line ~965) types the `Handle` hook's `resolve` parameter as:
```ts
resolve: (event: RequestEvent, opts?: ResolveOptions) => Promise<Response>;
```
`packages/kit/tsconfig.json` has `checkJs: true` and includes `src/**/*`, so `sequence.spec.js` is type-checked, and the `check` script runs `tsc`.
Three `resolve` mocks in the spec return a **synchronous** `Response`:
- Line 58: `resolve: () => response`
- The "uses first defined preload option" test: `resolve: (_event, opts = {}) => { ...; return new Response(html); }`
- The "uses first defined filterSerializedResponseHeaders option" test: same synchronous shape
A function returning `Response` is **not** assignable to a parameter typed `(...) => Promise<Response>` — `Response` has no `.then`/thenable structure — so `tsc` reports an assignability error at each of these mock sites. This matches commit `dc67c02` reverting the earlier fix (`f7b69b9`) while keeping the `Promise<Response>` typing.
The two `async` resolve mocks (in the `transformPageChunk` tests) are unaffected because an `async` function already returns `Promise<Response>`.
## Fix
Re-apply the `f7b69b9` change: wrap the three synchronous returns in `Promise.resolve(...)`:
- `resolve: () => Promise.resolve(response)`
- `return Promise.resolve(new Response(html));` (preload test)
- `return Promise.resolve(new Response(html));` (filterSerializedResponseHeaders test)
This makes each mock return `Promise<Response>`, satisfying the `Handle` type and restoring the type-check.
Note: I could not execute `tsc` here because TypeScript/deps are not installed in the environment, but the mismatch is structural and deterministic under `checkJs`.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: teemingc <chewteeming01@gmail.com>
|
/autofix |
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.9 ### Major Changes - breaking: `handle`'s `resolve` is now typed to always return a `Promise` ([#16352](#16352)) - breaking: replace the `$lib` alias with `#lib` and remove `files.lib` config. ([#16360](#16360)) - breaking: disallow cross-origin form submissions without a `Content-Type` header ([#16347](#16347)) - breaking: Server-only directories (`/server/` in the path) are now treated as server-only everywhere inside the project (except `src/routes` and the assets directory) ([#16360](#16360)) - breaking: delegate CORS handling to Vite for static directory requests during development ([#16357](#16357)) ### Minor Changes - feat: reinstate `$env/static/private`, `$env/dynamic/private`, `$env/static/public`, `$env/dynamic/public` and `$app/environment` as deprecated aliases for `$app/env/private` `$app/env/public` and `$app/env` ([#16334](#16334)) ### Patch Changes - perf: cache the default cookie header parse and avoid allocations in `cookies.get` ([#16341](#16341)) - fix: avoid client-side code being bundled by Cloudflare Wrangler ([#16364](#16364)) - fix: handle rejected streamed server data after delayed loads ([#16268](#16268)) - fix: enable CSRF protection in builds with a non-production `NODE_ENV` value ([#16313](#16313)) ## @sveltejs/package@3.0.0-next.3 ### Minor Changes - feat: transform import aliases into relative imports in files ([#16360](#16360)) ## @sveltejs/adapter-node@6.0.0-next.4 ### Patch Changes - fix: correctly bundle entrypoints on Windows ([#16367](#16367)) - Updated dependencies [[`c1ee782`](c1ee782), [`1a1b3ea`](1a1b3ea), [`6423d98`](6423d98), [`6d1f4f0`](6d1f4f0), [`5ca9906`](5ca9906), [`b148d31`](b148d31), [`5ca9906`](5ca9906), [`9f3d9bb`](9f3d9bb), [`7bfd922`](7bfd922), [`ffa0e3b`](ffa0e3b)]: - @sveltejs/kit@3.0.0-next.9 ## @sveltejs/enhanced-img@1.0.0-next.2 ### Patch Changes - chore: replace the `$lib` alias with `#lib` in docs ([#16360](#16360)) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
closes #12293
This PR turns on the no-misused-promises rule partially and attempts to standardise functions that are usually asynchronous more often than not (and therefore don't really need to use
MaybePromise)Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits