events: `on` EventTarget support + once compliance #33659

benjamingr · 2020-05-30T14:04:39Z

There are two commits here:

One makes the code used in once more generic and fixes the behavior on EventTargets. We had tests for behavior that can't actually exist. Removed the test + made sure our behavior was correct. It also makes the test use our EventTarget implementation rather than a mock.
One makes .on work with EventTargets using the same generic functions.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

benjamingr · 2020-05-30T14:21:59Z

test/parallel/test-events-once.js

-  strictEqual(Reflect.has(et.events, 'myevent'), false);
-}
-
-async function onceWithEventTargetTwoArgs() {


cc @JeniaBR

I think Domenic was right on #29498 (comment) .

This PR still uses the same array wrapping (for compatibility with EventEmitter but no longer supports multiple arguments in event dispatch (as that has always been impossible anyway).

mcollina

lgtm

nodejs-github-bot · 2020-05-30T16:45:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31658/

lib/events.js

lundibundi · 2020-05-30T17:13:17Z

lib/events.js

+}
+
+function eventTargetAgnosticRemoveListener(emitter, name, listener, flags) {
+  if (typeof emitter.removeEventListener === 'function') {


Nit: should we perhaps guard this with something like [Symbol.toStringTag] instead to make sure this doesn't break when custom functions (i.e. removeEventListener) are mixed into EventEmitter by users which would result in very confusing errors/listener disappears?

I'm not sure I understand this part, are you concerned about implementations that implement an incorrect removeEventListener but a correct .removeListener?

Would it help if we checked for the Emitter variants prior to the EventTarget ones?

I was concerned with extensions/mixins of our EventEmitter that add methods like addEventListener/removeEventListener for their own use that would break this code since it would assume we are dealing with EventTarget when the method really is just a user-defined one that is most likely not the one we expect.

Basically my concern is that checking that addEventListener/removeEventListener are functions may not be sufficient to make sure we have an EventTarget and may break things as these names aren't that uncommon.

Would it help if we checked for the Emitter variants prior to the EventTarget ones?

This will probably be better since EventEmitter has been present for a long time and has all kinds of different adaptations/variations is user code. Though this won't make the code future-proof for EventTarget. Hence I suggested using the [Symbol.toStringTag] to differentiate them (by checking emitter[Symbol.toStringTag] === 'EventTarget' and then defaulting to the EventEmitter case if that checked false)

I will update the code to reflect that then.

This means we will prioritize EventEmitter over EventTarget here which makes sense.

@lundibundi can you please take a look after the update?

I'd rather not rely on toStringTag since EventEmitter is the base class for a lot of things and some of those things might override toStringTag (though we can recursively check the prototype chain or something if we want to)

Yeah, toStringTag of EventEmitter is not an option but if we go with

(by checking emitter[Symbol.toStringTag] === 'EventTarget' and then defaulting to the EventEmitter case if that checked false)

(so that EventEmitter is just the default and its toStringTag is never checked) that may work out better.

I'm not sure which of the options would be better since both just check the property anyway but at least toStringTag was designed with a similar purpose (to identify an object) in mind.

I'd be fine with both solutions but perhaps someone else may give their opinion.
/cc @mcollina @jasnell

lib/internal/event_target.js

test/parallel/test-event-on-async-iterator.js

test/parallel/test-events-once.js

test/parallel/test-eventtarget.js

lundibundi · 2020-05-31T13:25:08Z

lib/events.js

+    // EventTarget does not have `error` event semantics like Node
+    // EventEmitters, we do not listen to `error` events here.


I think this comment should go to the addErrorHandlerIfEventEmitter.

nodejs-github-bot · 2020-05-31T13:48:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/31679/

Remove support for multiple arguments (which don't actually work for EventTarget). Use our EventTarget implementation rather than a mock. Refactor `once` code in preparation of `on` using shared code for supporting `on` with `EventTarget`s.

Support EventTarget in the `events.on` static method

jasnell · 2020-06-22T15:40:38Z

Closing in favor of #34015 (which combines this and other eventtarget PRs into a single to make landing easier

Port of nodejs/node#33659

Event listeners pased to un/subscribe the `abort` event are mismatched. This removes the wrapper function in `eventTargetAgnosticAddListener()` and directly passes the given listener to the `EventTarget`. IMO, removing the wrapper seems harmless, and the `AbortSignal` is seemingly the only `EventTarget` passed to this function for now. Fixes: nodejs#43337 Refs: nodejs#33659 Refs: nodejs#34997 Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

Event listeners passed to un/subscribe the abort event are mismatched. This removes the wrapper function in `eventTargetAgnosticAddListener()` and directly passes the given listener to the `EventTarget`. IMO, removing the wrapper seems harmless, and the `AbortSignal` is seemingly the only `EventTarget` passed to this function for now. Fixes: nodejs#43337 Refs: nodejs#33659 Refs: nodejs#34997 Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

Event listeners passed to un/subscribe the abort event are mismatched. This removes the wrapper function in `eventTargetAgnosticAddListener()` and directly passes the given listener to the `EventTarget`. IMO, removing the wrapper seems harmless, and the `AbortSignal` is seemingly the only `EventTarget` passed to this function for now. Fixes: #43337 Refs: #33659 Refs: #34997 Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: #43373 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Event listeners passed to un/subscribe the abort event are mismatched. This removes the wrapper function in `eventTargetAgnosticAddListener()` and directly passes the given listener to the `EventTarget`. IMO, removing the wrapper seems harmless, and the `AbortSignal` is seemingly the only `EventTarget` passed to this function for now. Fixes: nodejs/node#43337 Refs: nodejs/node#33659 Refs: nodejs/node#34997 Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: nodejs/node#43373 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

benjamingr added the events Issues and PRs related to the events subsystem / EventEmitter. label May 30, 2020

benjamingr commented May 30, 2020

View reviewed changes

mcollina approved these changes May 30, 2020

View reviewed changes

jasnell approved these changes May 30, 2020

View reviewed changes

benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2020

benjamingr mentioned this pull request May 30, 2020

events: support EventTarget in once #29498

Closed

3 tasks

lundibundi reviewed May 30, 2020

View reviewed changes

benjamingr force-pushed the event-once-target-compliance branch from 9bc9846 to 3371271 Compare May 31, 2020 11:40

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18

lundibundi approved these changes May 31, 2020

View reviewed changes

BridgeAR force-pushed the event-once-target-compliance branch from 4e30431 to 22fc567 Compare May 31, 2020 13:47

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 31, 2020

benjamingr and others added 5 commits June 6, 2020 12:05

events: fix EventTarget support

62e24d9

Remove support for multiple arguments (which don't actually work for EventTarget). Use our EventTarget implementation rather than a mock. Refactor `once` code in preparation of `on` using shared code for supporting `on` with `EventTarget`s.

events: support EventTarget in on

22d784e

Support EventTarget in the `events.on` static method

fixup! acorn doesn't like ?.

0f2315c

fixup! fix error listener handler case

a62ff6d

fixup! change order for listeners

d783fc5

benjamingr force-pushed the event-once-target-compliance branch from 22fc567 to d783fc5 Compare June 6, 2020 09:06

jasnell added the eventtarget Issues and PRs related to the EventTarget implementation. label Jun 19, 2020

jasnell mentioned this pull request Jun 22, 2020

events: combined event target fixes #34015

Closed

3 tasks

jasnell closed this Jun 22, 2020

goto-bus-stop mentioned this pull request Feb 27, 2021

add support for EventTarget in once browserify/events#82

Merged

goto-bus-stop added a commit to browserify/events that referenced this pull request Feb 28, 2021

fix EventTarget support

3a0bcfe

Port of nodejs/node#33659

daeyeon mentioned this pull request Jun 11, 2022

events: fix adding abort listener in events.once() #43373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: `on` EventTarget support + once compliance #33659

events: `on` EventTarget support + once compliance #33659

benjamingr commented May 30, 2020

benjamingr May 30, 2020

mcollina left a comment

nodejs-github-bot commented May 30, 2020

lundibundi May 30, 2020

benjamingr May 30, 2020

lundibundi May 30, 2020

benjamingr May 31, 2020

benjamingr May 31, 2020

lundibundi May 31, 2020

lundibundi May 31, 2020

nodejs-github-bot commented May 31, 2020

jasnell commented Jun 22, 2020

		// EventTarget does not have `error` event semantics like Node
		// EventEmitters, we do not listen to `error` events here.

events: on EventTarget support + once compliance #33659

events: on EventTarget support + once compliance #33659

Conversation

benjamingr commented May 30, 2020

Checklist

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 31, 2020

jasnell commented Jun 22, 2020

events: `on` EventTarget support + once compliance #33659

events: `on` EventTarget support + once compliance #33659