Bump undici & TS #9847
Bump undici & TS #9847
Conversation
🦋 Changeset detectedLatest commit: 0d4bd70 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
penalosa
force-pushed
the
penalosa/bump-undici
branch
2 times, most recently
from
3349d58 to
4b37b39
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
penalosa
force-pushed
the
penalosa/bump-undici
branch
from
859a957 to
d5593a9
Compare
packages/miniflare/src/http/fetch.ts
Outdated
| let protocols: string[] | undefined; | ||
| for (const [key, value] of request.headers.entries()) { | ||
| if (key.toLowerCase() === "sec-websocket-protocol") { | ||
| protocols = value.split(",").map((protocol) => protocol.trim()); | ||
| } else { | ||
| headers[key] = value; | ||
| headers.set(key, value); |
There was a problem hiding this comment.
Nit: Would it be better to make it headers.append(key, value)? As there could be multiple entries with the same key.
| @@ -59,13 +52,13 @@ export async function fetch( | |||
| // Establish web socket connection | |||
| const ws = new NodeWebSocket(url, protocols, { | |||
| followRedirects: request.redirect === "follow", | |||
| headers, | |||
| headers: Object.fromEntries(headers.entries()), | |||
There was a problem hiding this comment.
Nit: Object.fromEntries will ignores repeated keys, how about new undici.Headers(headers)?
There was a problem hiding this comment.
It will, but this is being passed in to WebSocket from ws, which only accepts an object here.
| const requests = new Map<string, BufferedRequest>(); | ||
| const responses = new Map<string, Response>(); |
There was a problem hiding this comment.
Previously, we relied on stability of the DispatchOptions variable, but that is no longer guaranteed in Undici v7 (see https://github.com/nodejs/undici/blob/c6a8b23ee55b34fedd7b90d3c7f4b51a35d3ee25/lib/mock/mock-agent.js#L74). As such, we serialise the options for lookup, and make sure we remove them manually from the Map after access so as not to cause a memory leak.
| onConnect(abort) { | ||
| if (abortSignalAborted) { | ||
| abort(); | ||
| } | ||
| }, |
There was a problem hiding this comment.
This doesn't appear to work in Undici v7. The abort() parameter isn't documented so I'm not 100% sure, but in manual testing calling abort() here (rather than in onComplete) causes fetch() calls to abort with undefined
Bump Undici & TS to the latest versions. I had a quick read of the Undici v6 & v7 breaking changes, and nothing seems like it'll affect us, but I'm mostly relying on E2E test coverage here, and the fact that we use the
fetch()API which shouldn't have changed.