-
Notifications
You must be signed in to change notification settings - Fork 8
Fix undici usage on node 26 (alternate to #53) #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c7b0e77
44223a7
03f2b26
949ad09
62cd8e9
f465606
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| /*! | ||
| * Copyright (c) 2022 Digital Bazaar, Inc. All rights reserved. | ||
| */ | ||
| import {Agent} from 'undici'; | ||
| import {Agent, fetch as undiciFetch, Request as UndiciRequest} from 'undici'; | ||
| import undiciPkg from 'undici/package.json' with {type: 'json'}; | ||
| import {versions} from 'node:process'; | ||
|
|
||
| // as long as an agent has a reference to it, its associated dispatcher will | ||
|
|
@@ -12,6 +13,37 @@ const AGENT_CACHE = new WeakMap(); | |
| const [major, minor] = versions.node.split('.').map(v => parseInt(v, 10)); | ||
| const canConvert = (major > 18) || (major === 18 && minor >= 2); | ||
|
|
||
| // A dispatcher built from the bundled undici's `Agent` shares a handler | ||
| // contract with the runtime's `fetch` only when their undici majors match. | ||
| // This package installs undici 6; node's own bundled undici major varies by | ||
| // release line and does not necessarily match that -- today node 22 bundles | ||
| // undici 6 (matches), while node 24 bundles undici 7 and node 26 bundles | ||
| // undici 8 (both mismatch, so both already take the fallback path below, | ||
| // not just node 26). The contract that breaks (the dispatcher handler's | ||
| // `onError`) changed across those majors, so a mismatched pairing rejects | ||
| // the bundled v6 dispatcher with "invalid onError method". When they match | ||
| // we hand the dispatcher to `ky`, which forwards it to the runtime fetch (ky | ||
| // deliberately keeps `dispatcher` out of its request-option registry so it | ||
| // reaches fetch). When they differ we call the bundled undici's own fetch, | ||
| // which cannot consume the runtime's `Request` class directly, so it is | ||
| // rebuilt as the bundled undici's own `Request` first (see `createFetch` | ||
| // below). This skew only exists because node does not | ||
| // expose its built-in undici (`node:undici`); see | ||
| // digitalbazaar/http-client#43. | ||
| // The version read is guarded: if a future undici hides `package.json` behind | ||
| // an `exports` map, or `process.versions.undici` is absent, default to the | ||
| // bundled undici's own fetch (the always-safe path) rather than throwing at | ||
| // module load and breaking `import` for every consumer. | ||
| const nativeFetchCompatible = (() => { | ||
| try { | ||
| const bundledMajor = parseInt(undiciPkg.version, 10); | ||
| const runtimeMajor = parseInt(versions.undici, 10); | ||
|
Comment on lines
+39
to
+40
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This naming is a bit confusing. Maybe "installed" and "builtin" would be better? |
||
| return runtimeMajor === bundledMajor; | ||
| } catch{ | ||
| return false; | ||
| } | ||
| })(); | ||
|
|
||
| // converts `agent`/`httpsAgent` option to a dispatcher option | ||
| export function convertAgent(options) { | ||
| if(!canConvert) { | ||
|
|
@@ -29,24 +61,51 @@ export function convertAgent(options) { | |
| return options; | ||
| } | ||
|
|
||
| // use custom fetch if agent has already been converted | ||
| let fetch = AGENT_CACHE.get(agent); | ||
| // reuse the dispatcher built for this agent | ||
| let dispatcher = AGENT_CACHE.get(agent); | ||
| if(!dispatcher) { | ||
| dispatcher = new Agent({connect: agent.options}); | ||
| AGENT_CACHE.set(agent, dispatcher); | ||
| } | ||
|
|
||
| // drop the converted legacy options so they are not forwarded to `fetch` | ||
| const rest = {...options}; | ||
| delete rest.agent; | ||
| delete rest.httpsAgent; | ||
|
|
||
| // compatible runtime: let `ky` forward the dispatcher to the native `fetch`, | ||
| // which consumes the runtime `Request` natively — no wrapper, native perf | ||
| if(nativeFetchCompatible) { | ||
| return {...rest, dispatcher}; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wasn't done previously (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. claude had a long winded run of tests and explanation summarized as "Confirmed — works now. On node 22 convertAgent returns only {dispatcher} and the self-signed HTTPS tests pass; without the dispatcher the same request fails with DEPTH_ZERO_SELF_SIGNED_CERT, so the option is definitely reaching fetch. The custom fetch is only needed on the major-mismatch path." |
||
| } | ||
|
|
||
| // incompatible runtime `fetch` that rejects this dispatcher, so route | ||
| // through the bundled undici's own fetch via an override | ||
| let fetch = AGENT_CACHE.get(dispatcher); | ||
| if(!fetch) { | ||
| const dispatcher = new Agent({connect: agent.options}); | ||
| fetch = createFetch(dispatcher); | ||
| fetch._httpClientCustomFetch = true; | ||
| AGENT_CACHE.set(agent, fetch); | ||
| AGENT_CACHE.set(dispatcher, fetch); | ||
| } | ||
|
|
||
| return {...options, fetch}; | ||
| return {...rest, fetch}; | ||
| } | ||
|
|
||
| // create fetch override uses custom `dispatcher`; since `ky` does not pass | ||
| // the dispatcher option through to `fetch`, we must use this override | ||
| function createFetch(dispatcher) { | ||
| return function fetch(...args) { | ||
| dispatcher = (args[1] && args[1].dispatcher) || dispatcher; | ||
| args[1] = {...args[1], dispatcher}; | ||
| return globalThis.fetch(...args); | ||
| // create fetch override uses custom `dispatcher`; on an incompatible runtime | ||
| // `ky`'s runtime `Request` cannot be consumed by the bundled undici's fetch | ||
| // directly, so it is rebuilt as the bundled undici's own `Request` here. | ||
| // Passing the runtime `Request` as undici's `Request` *init* (its second | ||
| // constructor argument) works because undici's own `Request` constructor | ||
| // performs its own `RequestInit` dictionary conversion -- it reads exactly | ||
| // the fields its own implementation understands directly off the object it's | ||
| // given, duck-typed rather than `instanceof`-checked, so it stays correct | ||
| // automatically as undici's own supported fields evolve. No manual | ||
| // allow/deny-list of `RequestInit` fields is needed or maintained here. | ||
| function createFetch(defaultDispatcher) { | ||
| return function fetch(input, init) { | ||
| const dispatcher = init?.dispatcher || defaultDispatcher; | ||
| if(input && typeof input === 'object' && typeof input.url === 'string') { | ||
| input = new UndiciRequest(input.url, input); | ||
| } | ||
| return undiciFetch(input, {...init, dispatcher}); | ||
| }; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this text block be simplified? I find it hard to follow. Not sure the "today" wording makes sense.