feat: add Undici adapter (follow-up)#10893
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 8 files
Confidence score: 2/5
- There is a high-confidence runtime risk in
package.json: declaringundicias optional conflicts with static imports in the adapter path, which can trigger module resolution failures in Node.js at runtime. lib/adapters/undici.jshas concrete behavior regressions: basic auth URLs can fail with undiciRequest, and the mixed static/dynamic loading plus per-requestnew Agent()adds both correctness and performance risk.- Test reliability is also shaky in
tests/unit/adapters/undici.test.jsbecause undefined helper references can causeReferenceErrorbefore adapter logic is exercised, reducing confidence in coverage of these changes. - Pay close attention to
package.json,lib/adapters/undici.js,tests/unit/adapters/undici.test.js- runtime dependency resolution, auth handling, and test validity need fixes before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/adapters/undici.js">
<violation number="1" location="lib/adapters/undici.js:2">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
Contradictory static and dynamic `undici` loading creates impossible fallback logic. The file static-imports `{ Agent, interceptors } from 'undici'` at module load time, so the module will already fail to load if `undici` is unavailable. Despite this, a runtime `try/catch` around `await import('undici')` assigns `undici = null` on failure, but the code then unconditionally accesses `undici.fetch`, `undici.request`, `undici.Request`, and `undici.Response` — making the null fallback non-functional and unreachable. This is unnecessary defensive handling for an impossible edge case.</violation>
<violation number="2" location="lib/adapters/undici.js:33">
P2: Per-request `new Agent()` causes avoidable connection pool churn and resource overhead</violation>
<violation number="3" location="lib/adapters/undici.js:105">
P1: Basic auth fails because undici `Request` rejects URLs containing credentials. The adapter should sanitize URL credentials before request construction.</violation>
</file>
<file name="tests/unit/adapters/undici.test.js">
<violation number="1" location="tests/unit/adapters/undici.test.js:589">
P2: Abort-normalization test references helpers that are not defined or imported in this file, so it will fail with ReferenceError before exercising the adapter.</violation>
<violation number="2" location="tests/unit/adapters/undici.test.js:686">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The test claims to validate AxiosError/ERR_NETWORK handling, but it only checks the nested cause code and never verifies the Axios error shape it names.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:53">
P2: Missing `exports` entry for new undici adapter subpath</violation>
<violation number="2" location="package.json:170">
P0: Optional peer dependency `undici` will cause runtime module resolution failures in Node.js because `lib/adapters/undici.js` statically imports `undici` at the top level, and `lib/adapters/adapters.js` statically imports the undici adapter module. This means any Node.js user importing axios without `undici` installed will get `Cannot find module 'undici'` at startup.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@niksy I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 8 files
Confidence score: 3/5
- There is a concrete regression risk in
lib/adapters/adapters.js: registeringundicias a direct adapter function instead of a lazy resolver object can change adapter fallback behavior at runtime. - Given the medium severity (6/10) and solid confidence (7/10), this is more than a cosmetic issue and could affect users depending on fallback adapter selection.
- Pay close attention to
lib/adapters/adapters.js- ensureundicifollows the same lazy resolver pattern asfetchso fallback semantics remain intact.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/adapters/adapters.js">
<violation number="1" location="lib/adapters/adapters.js:23">
P2: `undici` is registered as a direct adapter function instead of a lazy resolver object like `fetch`, breaking adapter fallback semantics</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fetch: { | ||
| get: fetchAdapter.getFetch, | ||
| }, | ||
| undici: undiciAdapter |
There was a problem hiding this comment.
P2: undici is registered as a direct adapter function instead of a lazy resolver object like fetch, breaking adapter fallback semantics
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/adapters/adapters.js, line 23:
<comment>`undici` is registered as a direct adapter function instead of a lazy resolver object like `fetch`, breaking adapter fallback semantics</comment>
<file context>
@@ -19,6 +20,7 @@ const knownAdapters = {
fetch: {
get: fetchAdapter.getFetch,
},
+ undici: undiciAdapter
};
</file context>
| undici: undiciAdapter | |
| undici: { | |
| get: (config) => undiciAdapter.get?.(config) ?? undiciAdapter | |
| } |
There was a problem hiding this comment.
Is this relevant? Undici adapter uses Fetch adapter getFetch method underneath.
| i = 1; | ||
| } | ||
|
|
||
| // option is ignored in undici.fetch |
There was a problem hiding this comment.
I’m not happy about this. It basically means we have different behavior based on requested response type. How should we handle this?
|
@niksy can you please look at resolving the comments if they have been fixed or leave a comment as to why you disagree |
|
@jasonsaayman will do! I’m running same benchmark as in original PR (https://github.com/nodejs/undici/tree/main/benchmarks), but unfortunately I’m not seeing any of the large gains from Should we also take a look at this before merging this PR? Does it make sense to contact someone from Undici to see if there is something we could do to improve our implementation? |
|
@niksy I will see if I can get some eyes on this from a Node maintainer. |
| let response = await (isRequestSupported | ||
| ? _fetch(request, fetchOptions) | ||
| : _fetch(url, resolvedOptions)); | ||
| ? _fetch(request, fetchOptions, resolvedConfig) |
There was a problem hiding this comment.
Since Undici adapter has different behavior based on passed config options (fast undici.request for most use cases, undici.fetch for stream/FormData, …) we pass currently resolved config to custom fetch adapter getter.
We have that config available in adapter, but that also means we create additional fetch adapter seed cache map entries for each Axios request which seems unecessary and uses more memory just because we have different usage semantics which could be inferred in run time of fetch function instead of Fetch adapter getter.
There was a problem hiding this comment.
@jasonsaayman I’ve reverted this change since I’m not sure if this is the correct approach.
Can you provide some pointers here? Am I using getFetch correctly?
It does seem like a waste to create new seed entry for each Axios request if we can manage to resolve everything when running env.fetch function and third argument which is final resolved config.
There was a problem hiding this comment.
Maybe we should see if we really need to use Undici globals. Are there any drawbacks if we use globals exposed via current Node fetch implementation (other than newer and possible more optimized implementations)? Maybe someone from Node can chime in with pros/cons of this approach.
jasonsaayman
added
commit::feat
2 participants
Summary
This PR is a follow-up to #6915, with up-to-date changes and minimal implementation using custom Fetch adapter.
Test suite is adapted from original PR and adjusted with latest fetch test suite changes, but there are 3 tests that are currently failing. I’ve marked them with comments so we can easily see and discuss potential changes.
This is the content diff between two test suites: https://www.diffchecker.com/hXpATVrd/
I’ve tried to omit what isn’t necessary for Undici test, but I don’t know if I missed something important.
Things currently not fleshed out:
undici.fetchandundici.requestimplementation have different options so maybe we should pick subset of common options (currentlyfetchOptionsis used)undici.fetchandundici.requesthave different errors for same requests (fetch needs to follow Fetch specification), so I’ve tried to remove differences by discarding Fetch-specific logic regarding network errorLinked issue
Issue #10885 tracks all the necessary changes.
Checklist
index.d.tsandindex.d.cts)Summary by cubic
Adds a Node-only
undiciadapter that preserves Axios behavior for redirects, timeouts, progress, streaming, size limits, and FormData. It lazy-loadsundici, usesundici.requestby default, and switches toundici.fetchonly when needed.Description
lib/adapters/undici.jsand registered it as'undici'; updated README and types.undiciwith a clear resolver error; adds optional peer depundici@^8(Node >= 22.19) and dev dep.undici.request; usesundici.fetchonly for progress,maxContentLength, orresponseTypestream/response/formdata/blob.FormDatato environment/undici.FormData; bridgesresponse.formData()to nativeFormData; also normalizes requestFormDatain thefetchadapter.maxRedirectsvia Undici’s global dispatcher + redirect interceptor; normalizes toAxiosError.ERR_FR_TOO_MANY_REDIRECTSin both code paths.ETIMEDOUT; mapsENOTFOUNDtoAxiosError: Network Errorwithcause.maxBodyLength/maxContentLength(incl. chunked anddata:URLs); sanitizes headers; sets defaultUser-Agenttoaxios/<version>unless provided.undiciefficiently in Node while preserving Axios semantics.Docs
Add a new
/docs/page for theundiciadapter: installation (npm i undici), usage (adapter: 'undici'), Node-only scope and minimum Node version, lazy import and resolver error, whenrequestvsfetchis used, redirect/timeout behavior, progress/streaming and size limits, default/overrideableUser-Agent, and request/responseFormDatahandling.Testing
Added
tests/unit/adapters/undici.test.jscovering:Content-Type, removing manual header without boundary, preserving custom boundary, and request conversion to env/undici.FormData.ETIMEDOUT); cancel classification.maxBodyLength/maxContentLength(incl. chunked anddata:URLs).ERR_FR_TOO_MANY_REDIRECTSacrossrequestandfetch.Semantic version impact
Minor: adds a new adapter and type literal (
'undici') without breaking existing APIs.Written for commit f60469b. Summary will update on new commits.