Skip to content

fix(worker): inbound message event.target is self, not null#264

Merged
colinhacks merged 1 commit into
mainfrom
worker-event-target
Jul 1, 2026
Merged

fix(worker): inbound message event.target is self, not null#264
colinhacks merged 1 commit into
mainfrom
worker-event-target

Conversation

@colinhacks

Copy link
Copy Markdown
Contributor

Inside a nub Worker, an inbound message event exposed the wrong target identity:

before:  event.target === null    event.currentTarget === null
after:   event.target === self    event.currentTarget === self

WHATWG DedicatedWorkerGlobalScope dispatches inbound events on the worker global, so both are self during the handler (browsers and Deno agree). The polyfill hand-invoked each listener with a constructed MessageEvent that never went through EventTarget dispatch, so the target slots stayed null.

Fix

The worker-side message/messageerror path dispatches the one shared event through a private EventTarget, with target/currentTarget shadowed to self as own properties (globalThis can't be a real EventTarget without prototype re-parenting, which the polyfill avoids). parentPort ref-counting — a worker with no message listener must still exit naturally — is preserved by mediating add/remove and implementing {once}/{signal} in-band, attaching one parentPort forwarder per type on the 0-to-1 listener boundary.

Verification

Both addEventListener('message') and self.onmessage now see target === currentTarget === self on the fast tier (Node 24, 26) and compat tier (Node 20). New test worker_inbound_message_target_is_self; the 22 existing worker tests stay green; clippy and fmt clean. The main-thread Worker.onerror/onmessage target (the Worker instance) is spec-correct and untouched.

Refs the worker-event-target-null thread. Held for maintainer review -- behavior change on a shipped worker surface.

…arget is self

Inside a worker, an inbound `message`/`messageerror` event had `event.target`
and `event.currentTarget === null` instead of `self`, violating WHATWG
DedicatedWorkerGlobalScope (browsers and Deno set both to self). The worker-side
scope hand-invoked each listener with a constructed MessageEvent that never went
through EventTarget dispatch, so the target slots were never populated.

Route inbound message/messageerror through a private EventTarget and shadow
`target`/`currentTarget` = self as own data properties on the event (globalThis
cannot be a genuine EventTarget without prototype re-parenting). parentPort
ref-counting for worker lifetime is preserved by mediating every add/remove and
implementing once/signal in-band, attaching one parentPort forwarder per type on
the 0<->1 listener boundary.

Verified on the fast tier (Node 24, 26) and compat tier (Node 20): both
addEventListener('message') and self.onmessage observers now see target ===
currentTarget === self over one shared event.
Copilot AI review requested due to automatic review settings July 1, 2026 03:43
@vercel

vercel Bot commented Jul 1, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nub Ready Ready Preview, Comment Jul 1, 2026 3:43am

Request Review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — a worker-polyfill fix so an inbound message/messageerror event's target and currentTarget are self during the handler (WHATWG DedicatedWorkerGlobalScope), instead of the pre-fix null.

  • Dispatch inbound events through a real EventTarget — the worker-side scope now dispatches the one shared MessageEvent through a private inbound EventTarget rather than hand-invoking cb.call(scope, new MessageEvent(...)), which never went through dispatch and left the target slots null.
  • Shadow target/currentTarget to self — set as own data properties on the event since globalThis can't be a genuine EventTarget without prototype re-parenting; native dispatch still drives listener ordering and stopImmediatePropagation over the single event object.
  • Preserve parentPort ref-countingattachPort/detachPort mediate a single per-type forwarder on the 0↔1 listener boundary, with {once}/{signal} implemented in-band so every removal stays observable to the count; a worker with no message listener still unrefs and exits naturally.
  • handleEvent binding + map rename — object listeners now bind via handleEvent.bind(listener) (spec-correct this); the wrappers map is renamed to registered with all references updated.
  • Test + fixtures — new worker_inbound_message_target_is_self asserts both addEventListener('message') and self.onmessage see target === currentTarget === self in registration order; file-worker fixtures work on all tiers.

The ref-counting, dedup, exact-wrapper removal, and dispatch-ordering invariants all hold under trace, and the {once} self-removal mid-dispatch is spec-legal (removing the currently-invoked listener skips no later one). The two divergences I looked at are inconsequential and either documented or pre-existing: currentTarget persists as self post-dispatch rather than resetting to null (worker code only reads it during the handler), and a user-synthesized self.dispatchEvent(new MessageEvent("message")) still routes to the other target rather than inbound — unchanged from before this PR. Clean to merge.

Pullfrog  | View workflow run | Using Claude Opus𝕏

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes worker-side inbound message / messageerror event dispatch so handlers observe spec-correct identity (event.target === self and event.currentTarget === self) by routing inbound events through an internal EventTarget while preserving Node’s parentPort ref-counting semantics.

Changes:

  • Reworks worker-side message / messageerror listener plumbing to dispatch via a private EventTarget and attach/detach a single parentPort forwarder per event type at the 0↔1 listener boundary.
  • Adds a new worker fixture pair to assert target/currentTarget identity across both addEventListener('message') and self.onmessage.
  • Adds an integration test covering the new behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
runtime/worker-polyfill.mjs Switches inbound message delivery to real EventTarget dispatch with listener-count mediated parentPort forwarding.
tests/fixtures/worker/event-target-self-worker.ts Worker-side fixture asserting inbound event target/currentTarget identity and handler ordering.
tests/fixtures/worker/event-target-self-main.ts Main-side driver fixture that posts to the worker and prints observed identities for harness assertions.
crates/nub-cli/tests/integration.rs Adds integration coverage for inbound message event target/currentTarget being self.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +603 to +610
function forward(evt, data) {
const ev = new MessageEvent(evt, { data });
// Shadow the inherited accessors so worker code observes the spec-required
// DedicatedWorkerGlobalScope as target/currentTarget instead of `inbound`.
Object.defineProperty(ev, "target", { value: scope, configurable: true });
Object.defineProperty(ev, "currentTarget", { value: scope, configurable: true });
inbound.dispatchEvent(ev);
}
@colinhacks colinhacks merged commit b023818 into main Jul 1, 2026
60 checks passed
@colinhacks

Copy link
Copy Markdown
Contributor Author

Shipped in v0.3.0: https://github.com/nubjs/nub/releases/tag/v0.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants