Repository navigation
Conversation
Bun's undici polyfill intercepts all `require("undici")` calls and
returns an incomplete stub implementation. This causes real-world
breakage because the polyfill lacks critical features like
`Agent.compose()`, real interceptors, and proper Dispatcher methods.
Remove the hardcoded module interception so that `require("undici")`
resolves to the actual npm-installed undici package, matching Node.js
behavior.
Fixes oven-sh#17799
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughThis PR removes Bun's bundled undici implementation: the JS compatibility layer, native JSC bindings, builtin/alias registrations, and related tests; test fixtures and the undici dev dependency version were updated accordingly. ChangesUndici polyfill removal
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
prob should replace it with at very least a very extensive integration test to prove undici works well in bun (and doesn't get regressed)
also a good way to judge the compatibility
There was a problem hiding this comment.
Good point, thanks! Will replace with comprehensive integration tests for the real undici package.
Replace deleted polyfill tests with integration tests for the real undici npm package. Upgrade undici from 5.20.0 to ^6.21.0 to cover Dispatcher.compose(), interceptors, MockAgent, and other APIs that the old polyfill was missing.
|
Updated undici from 5.20.0 to 6.21.3 in test/package.json — v5 doesn't have Dispatcher.compose() or the interceptors API, which are the exact features that motivated removing the polyfill (#17799). Testing against v5 would miss the whole point. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/first_party/undici/undici.test.ts`:
- Around line 332-340: The test should be converted to an async test and the
cleanup calls awaited to avoid races: change the spec to be async and replace
the synchronous Agent.close() and Client.close() calls with await agent.close()
and await client.close(); keep the same assertions for Agent.compose and
Client.compose to locate the code (Agent, Client, compose, close).
- Line 1: Replace the runtime require("module") usage with a module-scope import
for isBuiltin: add an import { isBuiltin } from "module" at the top of the file
and remove the dynamic require call where isBuiltin is obtained, ensuring all
references in the test (e.g., any uses of isBuiltin in the undici.test.ts suite)
use the statically imported symbol instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7ef0368-880f-4422-836b-64c1f67d646b
⛔ Files ignored due to path filters (1)
test/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
test/js/first_party/undici/undici.test.tstest/package.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/first_party/undici/undici.test.ts`:
- Around line 332-340: The test calls the async undici close() on Agent and
Client without awaiting, which can leave cleanup unresolved; make the test async
and await both agent.close() and client.close() (or return/await their promises)
so Agent.close() and Client.close() are properly awaited after asserting
compose() exists.
- Around line 95-97: The test uses a dynamic import inside the spec: `const
undici = await import("undici");`—replace that with a module-scope import so the
test validates API presence rather than dynamic loading. Add a top-of-file
module import (e.g., import * as undici from "undici";) and remove the in-test
await import; keep the rest of the "exports all expected top-level APIs" test
using the `undici` symbol unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ebc0f905-80a4-43ab-b40a-78134e5ece20
⛔ Files ignored due to path filters (1)
test/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
test/js/first_party/undici/undici.test.tstest/package.json
- Replace dynamic import/require with module-scope imports
- Import isBuiltin from node:module instead of require("module")
- Await agent.close() and client.close() in compose test
The Rust rewrite ported the module registry from Zig to Rust but
kept undici registered as a hardcoded module. Remove the enum
variant, phf map entry, alias table entry, and Next.js alias so
that require("undici") resolves to the real npm package.
|
Will this be merged soon? This creates problem on our side for using cyclonedx with bun. Thanks a lot! |
|
Thank you for this work, @galenzo17, and for the integration suite you added after the first review. Sorry that it sat without a clear answer for so long. The team is taking the approach in #36102 for this problem. Bare This branch also predates the Rust port of the resolver tables ( Your tests were useful. I ran the suite from this PR against the real undici 6.21.3 under Bun 1.4.3: 32 of 33 tests pass ( Closing this in favor of #36102. You are welcome to review it or build on it. If you think full removal of the shim is still the better call, say so there or on #17799 and this can be reopened. |


What does this PR do?
Removes Bun's built-in undici polyfill so that
require("undici")/import from "undici"resolves to the actual npm-installed package instead of Bun's incomplete stub.The polyfill lacks critical features (
Agent.compose(), real interceptors, properDispatchermethods) causing breakage in packages likepayload,@distube/ytdl-core, andhttp-cookie-agent. See also #20022.Changes:
undicifromHardcodedModuleenum, module map, and alias entries (includingnext/dist/compiled/undici)"undici"fromisBuiltinModuleandbuiltinModuleNameslistssrc/js/thirdparty/undici.js,src/jsc/bindings/Undici.{cpp,h}test/js/first_party/undici/fuzzy-wuzzy.test.tsbuiltin module listAfter this change,
require("undici")behaves like Node.js: it resolves to the npm package if installed, or throws "Cannot find module" otherwise.Fixes #17799
How did you verify your code works?
createUndiciInternalBinding,Undici.h,undici.js) acrosssrc/andtest/require("undici")/import from "undici"in test filesdetect-libcpolyfill removal (PR js: no longer provide our own 'detect-libc' #18138)