Fix undici usage on node 26 (alternate to #53) - #54
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
- Coverage 94.06% 93.89% -0.17%
==========================================
Files 4 4
Lines 236 295 +59
==========================================
+ Hits 222 277 +55
- Misses 14 18 +4
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
dlongley
left a comment
There was a problem hiding this comment.
Closer to what we want (I think), see comments.
| // 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}; |
There was a problem hiding this comment.
This wasn't done previously (fetch had to be passed) ... but maybe this works now?
There was a problem hiding this comment.
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."
| const bundledMajor = parseInt(undiciPkg.version, 10); | ||
| const runtimeMajor = parseInt(versions.undici, 10); |
There was a problem hiding this comment.
This naming is a bit confusing. Maybe "installed" and "builtin" would be better?
| 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 |
There was a problem hiding this comment.
Can this text block be simplified? I find it hard to follow. Not sure the "today" wording makes sense.
convertAgent builds a dispatcher from the bundled undici's Agent. When the runtime fetch and the bundled undici share a major version their handler contracts match, so the dispatcher can be handed to ky, which forwards it to the native fetch (ky keeps dispatcher out of its request-option registry so it reaches fetch); this preserves the native fetch and its performance. Only when the majors differ -- e.g. node 26's built-in undici 8 rejecting the bundled undici 6 dispatcher with "invalid onError method" -- fall back to the bundled undici's own fetch, decomposing ky's runtime Request to url + init because that fetch cannot consume a foreign Request class. Guard the version read so a future undici that hides package.json behind an exports map (or a missing process.versions.undici) defaults to the bundled fetch rather than throwing at module load and breaking import for every consumer. Strip the converted agent/httpsAgent options so they are not forwarded on to fetch. The previous approach always routed through the bundled undici's fetch, regressing runtimes whose native fetch was already compatible. This skew only exists because node does not expose its built-in undici. Add a POST-with-body agent test so the request body and headers are exercised across both the native and the bundled-fetch paths. Related to #43. Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
The existing agent tests already exercise the fallback path (bundled undici's own fetch) on Node 24, since its built-in undici (7.x) does not match this package's installed undici (6.x) either -- only Node 22's built-in undici happens to match today. Adding 26.x to the test matrix is what actually proves the fix on the one runtime it was written for, rather than relying on the local repro scripts alone.
Replaces the createRequire(...) require('undici/package.json') dance
with a native ESM JSON import attribute -- same value, no CJS interop
needed now that this is read at module scope with top-level await
unnecessary either way.
Also corrects the comment above it, which stated node 24 bundles
undici 6 (matching this package's installed undici) when it actually
bundles undici 7 -- only node 22 matches today. The runtime check
itself was never affected (it compares versions dynamically rather
than hardcoding them), but the comment was actively misleading.
Dave Longley flagged the previous field-by-field RequestInit copy (and a denylist/reflection-based version of the same idea, tried in between) as overly complex and not future-proofed -- any manual list of fields drifts out of sync with the Fetch spec as it gains new RequestInit members over time. `new UndiciRequest(input.url, input)` sidesteps the whole problem: undici's own `Request` constructor performs its own RequestInit dictionary conversion, reading exactly the fields its own implementation understands directly off the object it's given -- duck-typed, not `instanceof`-checked -- so passing the runtime's native `Request` as that init "just works," and stays correct automatically as undici's own supported fields evolve. No allow-list, deny-list, or prototype reflection needed or maintained here. Verified via the existing test suite (test-node: 22/22, test-node-cjs: 18/18, both including the HTTPS-agent POST test that exercises this exact path) plus manual checks of GET/HEAD/POST, a streaming body with a non-empty init override, and field-by-field confirmation that method/headers/mode/credentials/cache/redirect/referrer/ referrerPolicy/integrity/keepalive/signal/url all survive the conversion without the source request's body being consumed early.
cfc6c03 to
949ad09
Compare
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
Summary
fetchuses its own private undici, which can differ in major version from this package's installed undici (6.x); on a mismatch, handing it a dispatcher built from the installed undici'sAgentfails withUND_ERR_INVALID_ARG: invalid onError method.fetchwhen there's an actual version mismatch (process.versions.undicivs the installed undici's own version) — nativefetch/Responseis preserved wherever the majors already match, rather than unconditionally on every node version.main(post-4.3.0), not stacked on the CJS-removal branch, so this can ship as a 4.x patch independent of that larger breaking change.What's included
ad6ede8— the fix itself:convertAgentchecks whether the runtime's bundled undici major matches the installed undici major; on match, hands the dispatcher straight toky/native fetch; on mismatch, routes through the installed undici's ownfetch, decomposingky'sRequestinto(url, init)since the bundled undici's fetch can't consume the runtime'sRequestclass.https.Agent, JSON body over POST, redirect following) on node 26.x, not just 22/24 — this is what actually proves the fix on the runtime it targets, rather than relying on local repro scripts.undici/package.json's version via an ESM import attribute instead ofcreateRequire, and fixes a comment that incorrectly stated node 24 bundles undici 6 (it bundles undici 7 — only node 22 matches the installed undici 6 today, so node 24 already exercises the same fallback path as node 26).Test plan
test-nodepasses on 18.x/20.x/22.x/24.x/26.x in CItest-karma(browser path, unaffected by this node-only fix) passes