Fix undici usage on node 26 (alternate to #53) - #54
Open
applesnort wants to merge 15 commits into
Open
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
- Coverage 94.06% 94.05% -0.01%
==========================================
Files 4 4
Lines 236 303 +67
==========================================
+ Hits 222 285 +63
- Misses 14 18 +4
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
dlongley
requested changes
Jul 26, 2026
dlongley
left a comment
dlongley
left a comment
Member
There was a problem hiding this comment.
Closer to what we want (I think), see comments.
davidlehn
reviewed
Jul 28, 2026
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.
davidlehn
force-pushed
the
fix/undici-consistent-fetch
branch
from
July 28, 2026 21:27
cfc6c03 to
949ad09
Compare
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
The code used "bundled" for the npm-installed undici and "runtime"/"native" for the one built into node, which reads backwards and made `bundledMajor === runtimeMajor` hard to follow. Standardize on "installed" and "built-in" throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment above `builtinFetchCompatible` had grown to explain the background, the constant, both branches, and the guarded version reads all at once. Hoist the background to a module-level note, leave the constant with only its own meaning, and move the note about `ky` forwarding `dispatcher` to the branch that relies on it. Drop the `Request` rebuild explanation, which `createFetch` already documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename `builtinFetchCompatible` to `platformFetchCompatible` and `builtinMajor` to `platformMajor`, and update the comments to match. Also update the 4.3.1 changelog entry, which still used the older "bundled" and "runtime" wording for the same two things. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Using a minor bump due to nature of the undici fix.
dlongley
requested changes
Jul 31, 2026
dlongley
left a comment
dlongley
left a comment
Member
There was a problem hiding this comment.
One more nit here w/the possible cache confusion we should address. We should have an DISPATCHER_CACHE that has agent => dispatcher, and a FETCH_CACHE that has dispatcher => fetch.
| @@ -1,9 +1,21 @@ | |||
| /*! | |||
| * Copyright (c) 2022 Digital Bazaar, Inc. All rights reserved. | |||
Member
There was a problem hiding this comment.
Suggested change
| * Copyright (c) 2022 Digital Bazaar, Inc. All rights reserved. | |
| * Copyright (c) 2022-2026 Digital Bazaar, Inc. |
Comment on lines
+8
to
+17
| // Background: node ships its own copy of undici in the platform but does | ||
| // not expose it (there is no `node:undici`), so this package installs its | ||
| // own. A dispatcher only works with the undici that created it -- the | ||
| // handler contract changed across majors, so handing an installed v6 | ||
| // dispatcher to a platform v7 or v8 `fetch` fails with "invalid onError | ||
| // method". Which major the platform provides varies by release line (node | ||
| // 22 has 6, node 24 has 7, node 26 has 8), so no single installed version | ||
| // matches every supported runtime -- with undici 6 installed, both node 24 | ||
| // and node 26 take the fallback path below. See | ||
| // digitalbazaar/http-client#43. |
Member
There was a problem hiding this comment.
Note that if comments exceed more than 3-4 lines, we normally use multiline comment syntax instead (/* ... */).
- Update years. - Removed "All rights reserved.".
Convert the three multi-line explanatory `//` blocks in `lib/agentCompatibility.js` to `/* ... */`, matching the leading-asterisk style already used by the copyright and JSDoc headers. Short blocks, single-line comments, and commented-out code are left as `//`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`AGENT_CACHE` had come to hold two kinds of entry: agents keyed to their dispatchers, and dispatchers keyed to their `fetch` overrides. Split it into `DISPATCHER_CACHE` and `FETCH_CACHE` so each map is homogeneous and its contents are clear when inspected. Lifetimes are unchanged: a dispatcher is strongly held by `DISPATCHER_CACHE` while its agent is reachable, so its `FETCH_CACHE` entry lives exactly as long as the agent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dlongley
approved these changes
Jul 31, 2026
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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