Skip to content

Ignore null and undefined EventTarget listeners - #2343

Closed
sergioperezcheco wants to merge 1 commit into
websockets:masterfrom
sergioperezcheco:fix/nullable-event-listeners
Closed

sergioperezcheco wants to merge 1 commit into
websockets:masterfrom
sergioperezcheco:fix/nullable-event-listeners

Conversation

@sergioperezcheco

Copy link
Copy Markdown

addEventListener() currently registers a wrapper for a null or omitted callback. When the event fires, that wrapper tries to invoke the missing listener and throws instead of behaving as a no-op.

Ignore null and undefined callbacks before registering a wrapper. This keeps the existing EventEmitter dispatch and event classes intact; function listeners and objects with handleEvent retain their current behavior. Regression tests cover all four supported events, once listeners, and removal.

Verified on Node.js 22/macOS with the manifest-compatible Mocha 8.4.0: the regression tests fail on the original implementation and pass with the fix; the complete local unit/coverage suite, lint/format checks, ws/wss integration tests, and an independent loopback WebSocket exchange pass. Cross-platform/Node-version CI, optional native addons, and Autobahn were not run locally.

Prepared and independently reviewed/tested by autonomous Hermes agents; no human review is claimed.

Assisted-by: Hermes Agent
Signed-off-by: sergioperezcheco <checo520@outlook.com>
@lpinca

lpinca commented Oct 10, 2026

Copy link
Copy Markdown
Member

* @param {(Function|Object)} handler The listener to add

So why are you passing null and undefined? It does not make sense to call addEventListener() with a null or undefined listener.

@sergioperezcheco

Copy link
Copy Markdown
Author

The motivation was EventTarget compatibility: the DOM listener parameter is nullable, and passing null is specified to do nothing. A wrapper forwarding an optional handler could therefore work with a browser WebSocket but register a throwing wrapper with ws. That is an illustrative use case, not a production report I have reproduced.

You're right that ws currently documents only Function/Object listeners. I should have framed this as a compatibility change rather than a violation of the documented ws contract; the omitted-argument tests also go beyond the DOM's required second argument. If this compatibility behavior is not wanted, I can withdraw the PR rather than broaden that contract.

@lpinca

lpinca commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Compatibility is good but I don't think it makes much sense here. The EventTarget-like interface is deliberately not fully WHATWG-compliant. If this is added, proper arguments validation should also be included for both addEventListener() and removeEventListener(). E.g.

  • Check the number of arguments
  • Convert the type argument to a string
  • Throw an error if the listener argument is not an object or a function

I'm not very keen on adding all of that. There is little to no added value.

@sergioperezcheco

Copy link
Copy Markdown
Author

Understood — ws deliberately exposes an EventTarget-like interface, not full WHATWG compliance. This was a compatibility proposal rather than a reproduced violation of the documented Function/Object listener contract, and I don't have a concrete production use case that justifies adding argument-count checks, type coercion and listener validation to both methods. I'll withdraw this PR rather than broaden that contract. Thanks for clarifying the intended scope.

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