Conversation
The browser node:events polyfill only attached the captureRejections rejection handler when the listener returned a native same-realm Promise (result.then === Promise.prototype.then). A plain thenable, a cross-realm promise, or a promise-like from a userland library was dropped silently: no 'error' event, no nodejs.rejection hook, no unhandled rejection. Duck-type .then on any non-nullish return value, read it once, and call it with (undefined, onRejected), routing a throw from the getter or the call itself to the 'error' event. This matches Node.
WalkthroughRejection-capture logic in the events fallback module was modified to treat handler return values as generic thenables rather than only native promises, adding synchronous error handling for faulty ChangesCapture-rejections thenable support
Sequence Diagram(s)sequenceDiagram
participant emit as emitWithRejectionCapture
participant listener as Listener/onceWrapper
participant addCatch as addCatch
participant then as value.then
emit->>listener: invoke listener
listener-->>emit: return value (via onceWrapper)
emit->>addCatch: pass return value if non-null/undefined
addCatch->>then: check typeof value.then
alt then is function
addCatch->>then: then.call(value, undefined, onRejected)
then-->>addCatch: rejection triggers queueMicrotask
addCatch->>emit: emitUnhandledRejectionOrErr
else then access/call throws
addCatch->>emit: emit "error" synchronously
end
Estimated code review effort: High — changes rejection-capture semantics with subtle synchronous/asynchronous error paths. Related issues: None found in provided context. Related PRs: None found in provided context. Suggested labels: node:events, bundler, tests Suggested reviewers: None specified in provided context. 🐰 A promise or a plain old thing, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:16 AM PT - Jul 5th, 2026
❌ @robobun, your commit f0592e2 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33369That installs a local version of the PR into your bun-33369 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, correct alignment of the browser polyfill's addCatch with Node's duck-typed thenable handling; the .once() sibling noted inline is pre-existing and non-blocking.
Extended reasoning...
Overview
Two files touched. src/node-fallbacks/events.js rewrites addCatch to read .then once, check it's callable, invoke via then.call(value, undefined, onRejected), and wrap the whole thing in try/catch so a throwing .then getter routes to 'error' instead of escaping emit(). The call site in emitWithRejectionCapture drops the result.then === Promise.prototype.then identity check and now gates only on result !== undefined && result !== null. This mirrors Node's own addCatch in lib/events.js essentially line-for-line. test/bundler/bundler_browser.test.ts adds two itBundled cases exercising a plain thenable and a throwing then getter.
Security risks
None. This is the --target=browser polyfill for EventEmitter — bundled userland JS, no native code, no auth/crypto/permissions surface. The change strictly widens what gets routed to the emitter's own 'error' handler; it doesn't expose or leak anything.
Level of scrutiny
Low-to-medium. The blast radius is limited to browser bundles that opt into captureRejections, and the diff is ~15 lines of production JS plus tests. The new shape is a near-verbatim port of the reference implementation, and the PR description documents the four observable behaviors were cross-checked against Node v26.3.0. The extra cost of calling addCatch on every non-nullish return (vs. only native promises before) is negligible — the function immediately returns when .then isn't a function.
Other factors
The one finding from the bug hunt is a pre-existing gap: onceWrapper discards the listener's return value, so .once() listeners still bypass the capture. That's real and worth folding in, but it's not introduced or worsened by this diff — .once() + captureRejections was already broken on this path before the PR. Left as an inline nit rather than a blocker. Tests are placed alongside the existing browser/NodeUrl* polyfill test using the same itBundled pattern, and the PR body confirms the suite is green. No prior reviewer comments to address.
…lyfill The browser polyfill's onceWrapper discarded its listener's return value, so the result never reached the captureRejections gate in emit and a rejected promise from a .once() or .prependOnceListener() listener was dropped. Node's onceWrapper and the runtime's src/js/node/events.ts both return it.
|
Pushed f0592e2 for the
ee.once("boom", async () => { throw new Error("kaboom"); });dies with an unhandled rejection where Node prints Added On the earlier red build (68531): the single failed job was |
There was a problem hiding this comment.
LGTM — thanks for folding in the onceWrapper return and the .once()/.prependOnceListener() test.
Extended reasoning...
Overview
This PR fixes the captureRejections path in the browser node:events polyfill (src/node-fallbacks/events.js). Previously addCatch was gated on result.then === Promise.prototype.then, which excluded plain thenables, cross-realm promises, and userland promise-likes, and also read .then twice while letting a throwing getter escape emit(). The new addCatch reads .then once, checks it's callable, invokes it via then.call(value, undefined, onRejected), and routes any synchronous throw to the 'error' event — matching Node's lib/events.js implementation. The call site now gates on result !== undefined && result !== null.
Since my earlier review, commit f0592e2 addressed the onceWrapper sibling gap: it now returns listener.apply(this, args) so .once()/.prependOnceListener() results reach addCatch, and a third test (browser/NodeEventsCaptureRejectionsOnceListener) covers both entry points.
Security risks
None. This is Node-compat behavior in a browser bundler polyfill; no auth, crypto, filesystem, or network surface is touched. Reading .then on a listener's return value is exactly what Node and the Promises/A+ resolution procedure already do.
Level of scrutiny
Low-to-moderate. The file is a browser-target polyfill (not the runtime's own src/js/node/events.ts), the diff is ~20 lines of straightforward JS that mirrors Node's reference implementation, and it only affects the captureRejections: true opt-in path. The onceWrapper change adds a return whose only consumer is the rejection-capture gate — harmless in the non-capture path.
Other factors
Three new itBundled tests in test/bundler/bundler_browser.test.ts exercise the thenable case, the throwing-getter case (verifying emit() still returns true), and the .once()/.prependOnceListener() case. The PR description confirms behavior was cross-checked against Node v26.3.0 and that the runtime copy is being fixed separately in #32814. The bug hunter found no issues on the current revision, and my one prior comment has been fully addressed.
|
CI status for whoever merges this: both red jobs in build 68537 are infrastructure, and the diff is green. Not re-rolling, because neither failure is the kind a re-run fixes.
The one test on the lane that allocates multi-hundred-MB V8 heap snapshots got squeezed while the MySQL containers came up. Unrelated to this diff, which is a
Same failure on build 68531 (03:45Z) and build 68537 (11:33Z), eight hours apart on the same agent pool, and the same one #32814 hit on two different macOS 26 aarch64 agents. A re-roll will almost certainly reproduce it. Positive evidence the diff is fine, rather than just an absence of evidence:
Happy to re-roll if a maintainer wants to see it go green, but the macOS artifact-download timeout looks like it needs an agent-side fix first. |
|
Stale PR review: keep open, rework. The fix is still needed. On main ( The current diff is an incomplete port of Node's Wanted shape:
|
This branch has not been deployed
1 participant
The
node:eventspolyfill used for--target=browseronly captured rejections from a native, same-realmPromise. Any other thenable returned by a listener was dropped silently: no'error'event, noSymbol.for("nodejs.rejection")hook, no unhandled rejection.captureRejectionsis the safety net for async listeners, so a rejection that disappears is worse than no net at all.Separately, a rejection from a
.once()listener was dropped for the same reason, even for a native promise.Repro
Cause
Two independent gaps in
src/node-fallbacks/events.js, both on the path from a listener's return value toaddCatch.emitWithRejectionCapturegated onThe identity check against
Promise.prototype.thenexcludes plain thenables, cross-realm promises, and promise-likes from userland libraries. Node duck-types.thenon any non-nullish listener return value instead. A second divergence came out of the same line: the.thenread already happened during the check, so a throwingthengetter propagated out ofemit()rather than reaching the'error'event, and it could be read twice.onceWrappernever returned its listener's result, soresultwas alwaysundefinedfor a.once()or.prependOnceListener()listener and the gate was never entered at all. Node'sonceWrapperand the runtime's own copy (src/js/node/events.ts:314) both return it.Fix
addCatchnow reads.thenonce, checks it is callable, and calls it asthen.call(value, undefined, onRejected). A throw from the getter or from the call itself is routed to the'error'event. The call site gates onresult !== undefined && result !== null.onceWrapperreturns the listener's result.All five behaviors were checked against Node v26.3.0: the
thengetter is read exactly once (Promises/A+ compat),thenreceivesundefinedasonFulfilled, a throwing getter emits'error'synchronously,emit()still returnstrue, and a.once()listener's rejection reaches'error'.The runtime's own copy of the
addCatchbug,src/js/node/events.ts, is already fixed in #32814 (reviewed, CI green, awaiting a maintainer), which does not touch the browser polyfill, so that file is left alone here.Verification
Three tests in
test/bundler/bundler_browser.test.ts, all of which fail against main'ssrc/:browser/NodeEventsCaptureRejectionsThenable: a rejecting thenable reaches the'error'listener, andonFulfilledisundefined. Before: no output.browser/NodeEventsCaptureRejectionsThrowingThenGetter: a throwingthengetter reaches the'error'listener andemit()returnstrue. Before: the throw escapesemit()and kills the script.browser/NodeEventsCaptureRejectionsOnceListener:.once()and.prependOnceListener()rejections reach the'error'listener. Before: unhandled rejection, the script dies.bun bd test test/bundler/bundler_browser.test.tsis green (16 pass, 1 skip, 1 todo).