-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Node.js 15 native EventTarget
object
#1818
Comments
I think it is not possible to use the Node.js
The If I could go back in time I would have never made the Can't you use the |
Or maybe yes :-) NodeEventTarget extends from
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 :-) |
Ok, but |
Is it actually being used? We could take a look for what
I somewhat remember something about that, but an order of magnitude is too much... How are |
Reading Node.js |
I prefer to keep making Advantages:
Disadvantages:
That said, I think making |
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:
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 |
|
Would very much also want to see EventTarget being used also |
I just had this dumb error too:
Can you make a fork or something that uses proper EventTargets? |
With the introduction of e173423 subclassing 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);
|
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 |
It can but you need to wait for the previous dispatch to complete. |
We could do something like this diffdiff --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 |
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 |
👍🏻 to this :-) |
I would be open to that if |
Ah ok, but if I use .onmessage at the same time it doesn't seem to work IIRC |
It works if
|
That's exactly my point.
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. |
If I understood correctly the spec, using |
I respect your opinion, but I didn't do it. I've just tried to improve it. From #1818 (comment)
The primary, Also, again in my opinion, the fact that there is no way to create |
If that is the case, then scratch |
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.
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 |
The
Yes, I guess that is a not spec-compliant way of doing it :) |
Weird. Why would they do that?
Just using it as a property? Why is that not spec compliant? But yeah, coupled with
I'd call for not supporting it. |
Prevent the `onclose`, `onerror`, `onmessage`, and `onopen` getters and setters from returning or removing event listeners added with `WebSocket.prototype.addEventListener()`. Also prevent `WebSocket.prototype.removeEventListener()` from removing event listeners added with the `onclose`, `onerror`, `onmessage`, and `onopen` setters. Refs: #1818
FWIW 0b21c03 fixes the |
Damn, that's awesome. Props to you! One frustration less for users. |
Prevent the `onclose`, `onerror`, `onmessage`, and `onopen` getters and setters from returning or removing event listeners added with `WebSocket.prototype.addEventListener()`. Also prevent `WebSocket.prototype.removeEventListener()` from removing event listeners added with the `onclose`, `onerror`, `onmessage`, and `onopen` setters. Refs: #1818
I just had a weird bug migrating to Nuxt's v3-bridge: alias: {
ws: 'ws/browser.js',
} |
@stackdev37 it is already like that. function foo() {}
function bar() {}
websocket.onmessage = foo;
websocket.onmessage = bar; In the above example, the |
This is still an issue. In fact,
Can someone please share the status of this issue? If I find time to open a pull request, will somebody support me with the review and see this change merged? |
I think it's time to reconsider this, all Node.js versions that don't support |
@piranna, replacing |
Removing the |
@lpinca any particular reason the It can benefit greatly from extending
There are likely more, I haven't looked much. This is a fundamental issue since I'd even argue the convenience of the |
I propose the following:
Release this. Then, discuss anything else. @lpinca, what do you think about this proposal? |
Performance, and the ability to pass multiple arguments to the listeners. This allows for example to do websocket.on('message', function (buffer, isBinary) {
// Let the user decide what to do here.
}); without calling websocket.on('message', function (buffer, isBinary) {
websocket.send(buffer, { binary: isBinary });
});
What is the difference? We can't extend the native |
Isn't the transferred data either text or buffer-like? I know this is a naive suggestion but when does
I set the export function bindEvent<E extends Event, T>(
target: T,
event: E
): EventWithTarget<E, T> {
Object.defineProperty(event, 'target', {
enumerable: true,
writable: false,
value: target,
})
return event as EventWithTarget<E, T>
} I have the luxury of not caring about the Node internal that much but for |
That is how it worked and was changed to not call
It is indeed an internal |
We could override the getters in the subclass but that would prevent |
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 nativeEventTarget
class, that internally it's using anws
instance (and doing some other project specific things), and setting as listeners methods from thisClient
class, with the idea of propagate these errors to the user:Problem is, since
ws
is using its own implementation of bothEventTarget
andEvent
classes, when these events gets propagated to the native one, I get the next error:This is due because Node.js native
EventTarget
class is expecting a nativeEvent
class instance, instead of the one provided byws
. According to Node.js docs it should be accepting any object with atype
field, but for some reason is not accepting it.Reproducible in:
Steps to reproduce:
EventTarget
class in its prototype chainws
instance and call toaddEventListener
setting one function that propagate the event to the nativeEventTarget
Expected result:
ws
should check for actual support of bothEvent
andEventTarget
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 ofEvent
andEventTarget
classes, since there was none before, so now it conflicts with the new Node.js native available ones.The text was updated successfully, but these errors were encountered: