MessagePort: implement NodeEventTarget on the native listener list - #35811
Open
robobun wants to merge 13 commits into
Open
MessagePort: implement NodeEventTarget on the native listener list#35811robobun wants to merge 13 commits into
robobun wants to merge 13 commits into
Claude / Claude Code Review
completed
Jul 26, 2026 in 29m 7s
Code review found 2 potential issues
Found 3 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/js/node/worker_threads.ts:637-642 |
fakeParentPort removeEventListener shadow: drops options arg and doesn't look up once-wrapper |
| 🟡 Nit | src/jsc/bindings/webcore/JSMessagePort.cpp:576-588 |
removeAllListeners(undefined)/(null) clears type "undefined"/"null" instead of all listeners |
Annotations
Check warning on line 642 in src/js/node/worker_threads.ts
claude / Claude Code Review
fakeParentPort removeEventListener shadow: drops options arg and doesn't look up once-wrapper
The 46834ad `removeEventListener` shadow has two gaps vs the pre-PR `.bind(self)` and vs `off()`: (1) it drops the third `options`/useCapture argument, so a capture-phase listener added via `fake.addEventListener` (still `.bind(self)`, forwards all args) can no longer be removed; (2) it passes the raw `listener` to `self.removeEventListener` instead of `set?.get(listener) ?? listener` like `off()` does, so after `parentPort.once('message', fn)` a `removeEventListener('message', fn)` no-ops on `s
Check warning on line 588 in src/jsc/bindings/webcore/JSMessagePort.cpp
claude / Claude Code Review
removeAllListeners(undefined)/(null) clears type "undefined"/"null" instead of all listeners
The branch is on `callFrame->argumentCount() == 0`, so `port.removeAllListeners(undefined)` (or a computed `removeAllListeners(maybeType)` where `maybeType` is undefined/null) has `argumentCount() == 1`, takes the else path, stringifies to `"undefined"`/`"null"` via IDL DOMString conversion, and calls `removeAllEventListenersForType("undefined")` — leaving every real listener in place. Node's `NodeEventTarget.removeAllListeners` explicitly checks `type !== undefined && type !== null` and clears
Loading