Conversation
Contributor
WalkthroughChangesMessagePort EventEmitter-style behavior now loads from a shared EventTarget-based emitter module, with bootstrap builtins and C++ prototype registration. MessagePort emitter integration
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
Collaborator
Author
|
Updated 4:44 PM PT - Jul 12th, 2026
❌ @robobun, your commit beb068b has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33856That installs a local version of the PR into your bun-33856 --bun |
Contributor
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
Collaborator
Author
|
Not a duplicate of #29024. That PR expands the emitter surface ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/js/builtins/MessagePort.ts`:
- Around line 6-32: The listener wrapper tracking in MessagePort.on and
MessagePort.once is overwritten when the same function is registered more than
once, which breaks MessagePort.off removal. Update the wrapper storage so it is
keyed by both event and listener (for example, a per-listener map/list of
wrapped handlers) instead of assigning a single $nodeEventTargetWrapped value on
the listener. Then adjust off() to look up the matching wrapped handler for the
specific event and remove that exact wrapper, preserving previous registrations.
- Around line 43-51: The MessagePort emit helper currently returns the port
instance, which masks whether the dispatch was handled. Update the emit function
in MessagePort so it returns the boolean result of dispatchEvent for both the
error/messageerror and other event branches, instead of returning this, and keep
the behavior aligned with the emit method name used in MessagePort.
In `@src/jsc/bindings/webcore/JSMessagePort.cpp`:
- Around line 140-147: MessagePort.prototype is exposing unsupported
EventEmitter-style methods via JSMessagePort’s property table, specifically
prependListener and prependOnceListener, and they are wired to the on/once
generators instead of any true prepend behavior. Remove these entries from the
JSMessagePort builtins list, or only retain them if the API divergence is
explicitly intended and documented, so the exported MessagePort surface matches
Node’s supported methods.
In `@test/js/web/workers/message-channel.test.ts`:
- Around line 330-437: Mark the two new subprocess-spawning tests in
message-channel.test.ts as concurrent, since they are independent and
self-contained and currently run sequentially. Update the test declarations for
the MessagePort node-style emitter methods check and the events.once(port, ...)
check to use test.concurrent (or wrap them in a concurrent describe) so they can
run in parallel without sharing state.
🪄 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: 7d6d8d9c-c5bf-4c9a-a298-5f3e4c5b3301
📒 Files selected for processing (5)
src/js/builtins/BunBuiltinNames.hsrc/js/builtins/MessagePort.tssrc/js/node/worker_threads.tssrc/jsc/bindings/webcore/JSMessagePort.cpptest/js/web/workers/message-channel.test.ts
Collaborator
Author
|
The diff is green: the new The remaining CI red is unrelated to this diff:
Ready for review. |
…ut loading node:worker_threads
The emitter methods (on/once/off/emit/addListener/removeListener/listenerCount/
eventNames/removeAllListeners/setMaxListeners/getMaxListeners) were installed on
MessagePort's prototype chain by injectFakeEmitter() as a top-level side effect
of evaluating node:worker_threads. A port from the global new MessageChannel()
therefore had none of them until some dependency happened to load that module,
so `port.on('message', cb)` threw TypeError depending on import order. Node's
MessagePort inherits these from NodeEventTarget unconditionally.
Move injectFakeEmitter into internal/worker/messageport_emitter and put
self-replacing bootstrap stubs for each method name on JSMessagePortPrototype.
The first call to any stub requires the internal module, which inserts the real
methods on an intermediate prototype and deletes the stubs so subsequent lookups
hit the inherited implementations directly. worker_threads.ts now just requires
the same module (idempotent via the module cache).
robobun
force-pushed
the
claude/farm/75f53c78/messageport-emitter-without-worker-threads
branch
from
July 12, 2026 22:46
02a2b90 to
a7accad
Compare
…m the emitter prototype fakeParentPort defined own addListener/removeListener as raw self.addEventListener/removeEventListener, which shadowed the intermediate prototype's wrapper-aware aliases. Drop the own properties so the inherited on/off apply; they route through the own self-bound addEventListener/ removeEventListener, so listeners still land on the worker global scope.
Collaborator
Author
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-12, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
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
1 participant
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.
Repro
Importing
node:worker_threadsanywhere (even transitively, even after the port is constructed) retroactively makes the same port work:Node has these methods on the class from the start.
Cause
src/js/node/worker_threads.tscallsinjectFakeEmitter(globalThis.MessagePort)at module top level, so the NodeEventTarget emitter surface (on/once/off/emit/addListener/removeListener/listenerCount/eventNames/removeAllListeners/setMaxListeners/getMaxListeners) only appears onMessagePort's prototype chain oncenode:worker_threadsis first evaluated. A port from the globalnew MessageChannel()has none of them until something else in the dependency graph loads that module. In node,MessagePort's prototype chain runs throughNodeEventTarget, so the methods are always present.Fix
Move
injectFakeEmitterintointernal/worker/messageport_emitterand put self-replacing bootstrap stubs for each emitter method name onJSMessagePortPrototype(viasrc/js/builtins/MessagePort.ts). The first call to any stub requires the internal module, which inserts the real methods on the intermediate prototype (same as before) and deletes the stubs so subsequent lookups hit the inherited implementations directly.worker_threads.tsnow just requires the same module; the module cache makes that idempotent.The emitter implementation itself is unchanged from
main; only where and when it is installed changes.Verification
New tests in
test/js/web/workers/message-channel.test.tsspawn subprocesses that assertnode:worker_threadsis not inrequire.cache, then check the prototype shape and exerciseon/once/off/addListener/removeListener/listenerCountend to end, plusevents.once(port, 'message')(which now resolves with the payload, matching Node).Existing coverage stays green:
test/js/web/workers/,test/js/node/worker_threads/worker_threads.test.ts(all MessagePort/NodeEventTarget/listener-registry tests), and thetest-worker-message-port*/test-messagechannel*node-compat tests.[review] gate passed · iteration 3 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 6 passed · 0 rejected · iteration 3
evidence per changed file