ws
ws copied to clipboard
Use Node.js 15 native `EventTarget` object
- [x] I've searched for any related issues and avoided creating a duplicate issue.
Description
Node.js 15 already provides a native implementation of EventTarget
, so there's no need to use our own implementation. In fact, using both of them at the same time leads to errors.
In my use case, I've created a Client
class that extends from Node.js native EventTarget
class, that internally it's using an ws
instance (and doing some other project specific things), and setting as listeners methods from this Client
class, with the idea of propagate these errors to the user:
export class Client extends EventTarget
{
constructor(ws)
{
super()
if(!(ws instanceof WebSocket)) ws = new WebSocket(ws)
ws.binaryType = 'arraybuffer'
ws.addEventListener('close', this.#onClose, {once: true})
ws.addEventListener('error', this.#onError)
ws.addEventListener('message', this.#onMessage)
ws.addEventListener('open', this.#onOpen, {once: true})
this.#ws = ws
}
#onClose = this.dispatchEvent.bind(this)
#onError = this.dispatchEvent.bind(this)
#onOpen = this.dispatchEvent.bind(this)
}
Problem is, since ws
is using its own implementation of both EventTarget
and Event
classes, when these events gets propagated to the native one, I get the next error:
TypeError [ERR_INVALID_ARG_TYPE]: The "event" argument must be an instance of Event. Received an instance of ErrorEvent
at new NodeError (node:internal/errors:277:15)
at EventTarget.dispatchEvent (node:internal/event_target:326:13)
at WebSocket.onError (/home/piranna/Trabajo/Atos/awrtc_signaling/node_modules/ws/lib/event-target.js:141:16)
at WebSocket.emit (node:events:329:20)
at WebSocket.EventEmitter.emit (node:domain:467:12)
at ClientRequest.<anonymous> (/home/piranna/Trabajo/Atos/awrtc_signaling/node_modules/ws/lib/websocket.js:579:15)
at ClientRequest.emit (node:events:329:20)
at ClientRequest.EventEmitter.emit (node:domain:467:12)
at TLSSocket.socketErrorListener (node:_http_client:478:9)
at TLSSocket.emit (node:events:329:20) {
code: 'ERR_INVALID_ARG_TYPE'
}
This is due because Node.js native EventTarget
class is expecting a native Event
class instance, instead of the one provided by ws
. According to Node.js docs it should be accepting any object with a type
field, but for some reason is not accepting it.
Reproducible in:
- version: 7.4.0
- Node.js version(s): 15.2.0
- OS version(s): Ubuntu 20.10
Steps to reproduce:
- use Node.js 15
- create an object instance with Node.js 15 native
EventTarget
class in its prototype chain - create a
ws
instance and call toaddEventListener
setting one function that propagate the event to the nativeEventTarget
- emit the event
- BOOM
Expected result:
ws
should check for actual support of both Event
and EventTarget
classes in the native platform (in this case, Node.js 15) and use them. In case they are not available, then use its own implementation as a polyfill.
Actual result:
ws
is using always its own implementation of Event
and EventTarget
classes, since there was none before, so now it conflicts with the new Node.js native available ones.
I think it is not possible to use the Node.js EventTarget
implementation without introducing breaking changes and performance regressions.
- We would need to call
ws.dispatchEvent()
every time we callws.emit()
. - I'm not sure what to do with non standard events like
'ping'
,'pong'
,'unexpected-response'
, and'upgrade'
. Currently the listeners of these events do not receive anEvent
even if they are added viaws.addEventListener()
The ws
implementation relies on the EventEmitter
interface.
If I could go back in time I would have never made the WebSocket
an EventTarget
. Anyway the feature was added before I started contributing to ws
and I think the reason was to have a browser compatibile interface.
Can't you use the EventEmitter
interface in your code? Make Client
inherits from EventTarget
and use ws.{on,once}()
instead of ws.addEventListener()
.
I think it is not possible to use the Node.js
EventTarget
implementation without introducing breaking changes and performance regressions.
Or maybe yes :-) NodeEventTarget extends from EventTarget
and implements the EventEmitter
API on top of it. I read in the development discussion that it was specifically designed for compatibility and migration issues, so if you are concerned about that, probably this would be the correct aproach to begin with.
Can't you use the
EventEmitter
interface in your code? MakeClient
inherits fromEventTarget
and usews.{on,once}()
instead ofws.addEventListener()
.
I didn't consider that, since by inertia I always use the W3C API for compatibility between browser and Node.js, but yes, it's something I can do since in this case it would be a Node.js only code :-)
Or maybe yes :-) NodeEventTarget extends from
EventTarget
and implements theEventEmitter
API on top of it. I read in the development discussion that it was specifically designed for compatibility and migration issues, so if you are concerned about that, probably this would be the correct aproach to begin with.
Ok, but NodeEventTarget
currently only implements a subset of the EventEmitter
API. Some important missing API are emitter.prependListener()
and emitter.listeners()
. It would still be a breaking change and I'm still worried about performance regressions. I didn't follow the EventTarget
implementation closely in Node.js and I'm not sure if it was about NodeEventTarget
but I remember some benchmarks where EventTarget
was an order of magnitude slower than the EventEmitter
.
One important missing API is
emitter.prependListener()
andemitter.listeners()
. It would still be a breaking change
Is it actually being used? We could take a look for what NodeEventTarget
missing features are being used and identify if they can be fixed someway.
I'm still worried about performance regressions. I didn't follow the
EventTarget
implementation closely in Node.js and I'm not sure if it was aboutNodeEventTarget
but I remember some benchmarks whereEventTarget
was an order of magnitude slower than theEventEmitter
.
I somewhat remember something about that, but an order of magnitude is too much... How are ws
benchmarks being done? Maybe we can work in a separate branch and check for them...
Reading Node.js EventTarget
source code, it shows events list is implemented with a private SafeMap
, and that's what NodeEventTarget
uses for the listenerCount()
count method. We would need to change its implementation, but if both emitter.prependListener()
and emitter.listeners()
are needed and can be used an alternative, seems it's possible to implement them there... just not something immediate.
I prefer to keep making WebSocket
inherit from EventEmitter
for now.
Advantages:
- No breaking changes.
- No feature detection and polyfill.
- The
WebSocket
class is always anEventEmitter
and the documentation is the same in all supported Node.js versions. - No performance regressions.
Disadvantages:
- Some compatibility issues between the very simple
ws
EventTarget
implementation and the Node.jsEventTarget
implementation.
That said, I think making WebSocket
inherit from NodeEventTarget
is something worth exploring.
Also there are some node packages with binary blobs that aren't ready, and don't yet work under Node v15. I use one of those closely tied to ws (wrtc)
After taking a more carefully look, probably this can be splitted in two tasks:
- use native
Event
class - use native
EventTarget
orNodeEventTarget
class
First one is just a data container, so it's easy to replace, and that would fix the current error since it's doing a validation that the provided object inherit from the Event
class, so it would work and probably second one would not be needed (or would get a lower priority).
target
field of Event
class is read-only, and we are assigning it. I've reviewed Node.js source code and it's only set by private [kHybridDispatch]()
method, so it's not possible to only do first step, we'll need to go directly to use Node.js native EventTarget
or NodeEventTarget
class, not just only the native Event
class :-/
Would very much also want to see EventTarget being used also
I just had this dumb error too:
TypeError [ERR_INVALID_ARG_TYPE]: The "event" argument must be an instance of Event. Received an instance of OpenEvent
at new NodeError (node:internal/errors:363:5)
at EventTarget.dispatchEvent (node:internal/event_target:401:13)
[...]
Can you make a fork or something that uses proper EventTargets?
With the introduction of e173423c180dc1e4e6ee8938d9e4376a7a8b9757 subclassing EventTarget
would be even harder.
Also the pattern used in the issue description would not work even if WebSocket
was a subclass of EventTarget
.
const event = new Event('foo');
const target1 = new EventTarget();
const target2 = new EventTarget();
target1.addEventListener('foo', target2.dispatchEvent.bind(target2));
target2.addEventListener('foo', function (event) {
console.log(event);
});
target1.dispatchEvent(event);
node:internal/event_target:635
process.nextTick(() => { throw err; });
^
Error [ERR_EVENT_RECURSION]: The event "foo" is already being dispatched
at new NodeError (node:internal/errors:370:5)
at EventTarget.dispatchEvent (node:internal/event_target:401:13)
at EventTarget.[nodejs.internal.kHybridDispatch] (node:internal/event_target:455:20)
at EventTarget.dispatchEvent (node:internal/event_target:403:26)
at Object.<anonymous> (/home/luigi/event-target.js:11:9)
at Module._compile (node:internal/modules/cjs/loader:1095:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
at Module.load (node:internal/modules/cjs/loader:975:32)
at Function.Module._load (node:internal/modules/cjs/loader:816:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12) {
code: 'ERR_EVENT_RECURSION'
}
The event "foo" is already being dispatched
Does this means, once an event is dispatched, it can't be dispatched again in other object, and a new one needs to be created? It sorts of make sense, since target
should be different...
It can but you need to wait for the previous dispatch to complete.
We could do something like this
diff
diff --git a/lib/event-target.js b/lib/event-target.js
index cc4f3ba..1da1f1a 100644
--- a/lib/event-target.js
+++ b/lib/event-target.js
@@ -1,21 +1,32 @@
'use strict';
+const event = new Event('foo');
+let kTarget;
+
+for (const symbol of Object.getOwnPropertySymbols(event)) {
+ if (String(symbol) === 'Symbol(kTarget)') {
+ kTarget = symbol;
+ break;
+ }
+}
+
/**
* Class representing an event.
*
* @private
*/
-class Event {
+class WsEvent extends Event {
/**
- * Create a new `Event`.
+ * Create a new `WsEvent`.
*
* @param {String} type The name of the event
* @param {Object} target A reference to the target to which the event was
* dispatched
*/
constructor(type, target) {
- this.target = target;
- this.type = type;
+ super(type);
+
+ this[kTarget] = target;
}
}
@@ -25,7 +36,7 @@ class Event {
* @extends Event
* @private
*/
-class MessageEvent extends Event {
+class MessageEvent extends WsEvent {
/**
* Create a new `MessageEvent`.
*
@@ -46,7 +57,7 @@ class MessageEvent extends Event {
* @extends Event
* @private
*/
-class CloseEvent extends Event {
+class CloseEvent extends WsEvent {
/**
* Create a new `CloseEvent`.
*
@@ -72,7 +83,7 @@ class CloseEvent extends Event {
* @extends Event
* @private
*/
-class OpenEvent extends Event {
+class OpenEvent extends WsEvent {
/**
* Create a new `OpenEvent`.
*
@@ -90,7 +101,7 @@ class OpenEvent extends Event {
* @extends Event
* @private
*/
-class ErrorEvent extends Event {
+class ErrorEvent extends WsEvent {
/**
* Create a new `ErrorEvent`.
*
@@ -129,22 +140,27 @@ const EventTarget = {
if (typeof listener !== 'function') return;
function onMessage(data, isBinary) {
- listener.call(
- this,
- new MessageEvent(isBinary ? data : data.toString(), this)
- );
+ const event = new MessageEvent(isBinary ? data : data.toString(), this);
+ listener.call(this, event);
+ event[kTarget] = null;
}
function onClose(code, message) {
- listener.call(this, new CloseEvent(code, message.toString(), this));
+ const event = new CloseEvent(code, message.toString(), this);
+ listener.call(this, event);
+ event[kTarget] = null;
}
function onError(error) {
- listener.call(this, new ErrorEvent(error, this));
+ const event = new ErrorEvent(error, this);
+ listener.call(this, event);
+ event[kTarget] = null;
}
function onOpen() {
- listener.call(this, new OpenEvent(this));
+ const event = new OpenEvent(this);
+ listener.call(this, event);
+ event[kTarget] = null;
}
const method = options && options.once ? 'once' : 'on';
but it is very fragile and event.target
would not be a real EventTarget
.
Yeah I actually figured out that I can't redispatch events anyways, not even in the browser. So nvm for my part. But it would still be better if you used something that's instanceof EventTarget and then dispatched things that are instances of native Event because then .addEventListener would be native and have options like once.
The once
option is already supported. See https://github.com/websockets/ws/commit/2e5c01f5b550ae4171d127b0b707ebcec5925cc3.
-class Event { +class WsEvent extends Event {
👍🏻 to this :-)
I would be open to that if Symbol('kTarget')
was exposed.
The once option is already supported. See 2e5c01f.
Ah ok, but if I use .onmessage at the same time it doesn't seem to work IIRC
Ah ok, but if I use .onmessage at the same time it doesn't seem to work IIRC
It works if websocket.addEventListener()
is used after websocket.onmessage
. It doesn't if the order is reversed but
- It is fixable.
- It is an edge case. Why using both?
- AFAIK it is not possible to create
on<eventName>
attributes with the Node.jsEventTarget
implementation. There is an helper function to do that but it is not public.
Ah ok, but if I use .onmessage at the same time it doesn't seem to work IIRC
It works if
websocket.addEventListener()
is used afterwebsocket.onmessage
. It doesn't if the order is reversed but1. It is fixable. 2. It is an edge case. Why using both? 3. AFAIK it is not possible to create `on<eventName>` attributes with the Node.js `EventTarget` implementation. There is an [helper function](https://github.com/nodejs/node/blob/e2a6399be742f53103474d9fc1e56fadf7f90ccc/lib/internal/event_target.js#L651-L683) to do that but it is not public.
That's exactly my point. It's a big strange buggy mess because you tried making your own events. By using the browser/node provided events everything is compatible and you don't get weird issues. There is a, maybe limited, spec, but it's followed well in all aspects.
2. It is an edge case. Why using both?
Made a "mothersocket" that reconnects if anything goes wrong, "queues" messages and sends them so often until the server acknowledges them - and that still can be used like it's "one websocket". It runs both on browser and node and because the browser has .onmessage, etc I had to implement on-something support. I tried to just put it onto the websocket and debugged the hell out of it because I ran into the weird something added first other ones don't fire bug. But I ended up just adding one event listener and dispatching to the class itself and calling the .on functions from there.
It works if
websocket.addEventListener()
is used afterwebsocket.onmessage
. It doesn't if the order is reversed but
If I understood correctly the spec, using on...
remove all the previous handlers, both on...
and addEventListener
ones. That's why on...
ones are totally discouraged and unrecommended in W3C specs.
It's a big strange buggy mess because you tried making your own events.
I respect your opinion, but I didn't do it. I've just tried to improve it. From https://github.com/websockets/ws/issues/1818#issuecomment-731575731
If I could go back in time I would have never made the
WebSocket
anEventTarget
. Anyway the feature was added before I started contributing tows
and I think the reason was to have a browser compatible interface.
The primary, EventEmitter
based interface is way better for a server-side library in my opinion. I don't think it is possible to inherit from both EventEmitter
and EventTarget
as that means emitting the events twice as written above. So what is in place is a "best-effort" approach to have a browser compatible API. I wasn't there but I'm pretty sure that making it spec-compliant was not even considered. If you have better ideas please share them, I'm all ears.
Also, again in my opinion, the fact that there is no way to create on<eventName>
attributes when using the Node.js EventTarget
implementation proves that it is not quite there yet. We would need to use a copy of defineEventHandler()
or something like that.
If I understood correctly the spec, using
on...
remove all the previous handlers, bothon...
andaddEventListener
ones. That's whyon...
ones are totally discouraged and unrecommended in W3C specs.
If that is the case, then scratch ~~1. It is fixable
~~ and ~~2. It is an edge case. Why using both?
~~, as it works as intended.
If I understood correctly the spec, using on... remove all the previous handlers, both on... and addEventListener ones. That's why on... ones are totally discouraged and unrecommended in W3C specs.
What spec? :eyes:
This is how it works in the browser and how I'm used to it:
![Screenshot 2021-07-15 at 21 54 06](https://user-images.githubusercontent.com/4047180/125849313-1f0ac52f-00fa-4875-be68-708be83dcff6.png)
I respect your opinion, but I didn't do it. I've just tried to improve it. From #1818 (comment)
Ah shoot, np. Thanks.
Then node is the weird thing and you're just an unfortunate target of my frustration as I'm coming from the frontend.
Why the fuck do they have an EventEmitter when there's also EventTarget? Also they don't even have CustomEvent.
Also, again in my opinion, the fact that there is no way to create on<eventName> attributes when using the Node.js EventTarget implementation proves that it is not quite there yet. We would need to use a copy of defineEventHandler() or something like that.
I'd just not support it. But if you want, why not just make it a normal property and every time you dispatch an event you check if the property is a function and then call it? It's like 4 lines and as far as I understand it would cover the functionality
Why the fuck do they have an
EventEmitter
when there's alsoEventTarget
?
The EventEmitter
came first and is more efficient in terms of both performance and memory usage.
I'd just not support it. But if you want, why not just make it a normal property and every time you dispatch an event you check if the property is a function and then call it? It's like 4 lines and as far as I understand it would cover the functionality.
Yes, I guess that is a not spec-compliant way of doing it :)
The EventEmitter came first and is more efficient in terms of both performance and memory usage.
Weird. Why would they do that?
Yes, I guess that is a not spec-compliant way of doing it :)
Just using it as a property? Why is that not spec compliant? But yeah, coupled with
on... ones are totally discouraged and unrecommended in W3C specs.
I'd call for not supporting it.
FWIW https://github.com/websockets/ws/commit/0b21c03a6e69f8e37b2dfe55c4e753575fc09ac7 fixes the ws.on<eventName>
after websocket.addEventListener()
issue discussed above.
Damn, that's awesome. Props to you! One frustration less for users.
I just had a weird bug migrating to Nuxt's v3-bridge: Class extends value [object Module] is not a constructor or null
coming from class WebSocket extends EventEmitter while on Node 16. My current fix is to specify the following in nuxt.config.ts
:
alias: {
ws: 'ws/browser.js',
}
I see using <on~> will remove all of the previous handlers. ws should be worked as well.
@stackdev37 it is already like that.
function foo() {}
function bar() {}
websocket.onmessage = foo;
websocket.onmessage = bar;
In the above example, the foo
handler is removed when the bar
handler is set.