Skip to content
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

events: getEventListeners static #35991

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

This adds a static getEventListeners to events. The major difference between it and .listeners is that it works with EventTargets and not just EventEmitters.

This is useful after talking to users of EventTarget and AbortController for two main reasons:

  • For diagnostics and digging into why EventTargets are behaving and trying to isolate possible memory leaks.
  • For creating linked AbortSignals potentially.

cc @bterlson

This has to be a static because we are not allowed to add this method on EventTarget.prototype. I checked with WHATWG and the only way I could come up with that is both spec compliant and addresses the use case.

The web platform itself does not have a way to do this (due to encapsulation) but I double checked we are allowed to expose this functionality as long as it's not on EventTarget itself (from the public #whatwg IRC channel):

Screen Shot 2020-11-05 at 21 02 17

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 benjamingr added events Issues and PRs related to the events subsystem / EventEmitter. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 6, 2020
For `EventEmitter`s this behaves exactly the same as calling `.listeners` on
the emitter.

For `EventTargegt`s this is the only way to get the event listeners for the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For `EventTargegt`s this is the only way to get the event listeners for the
For `EventTarget`s this is the only way to get the event listeners for the

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

It seems unlikely this will be backported to v12 (going to maintenance this month), so we may as well use optional chaining

lib/events.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
lib/events.js Outdated Show resolved Hide resolved
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
lib/events.js Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 6, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@@ -0,0 +1,37 @@
// Flags: --expose-internals --no-warnings
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems unnecessary:

Suggested change
// Flags: --expose-internals --no-warnings

Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed for NodeEventTarget no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I ended up not needing it, fun

@benjamingr benjamingr force-pushed the events-get-listeners branch from 4a25a04 to b104cad Compare November 8, 2020 11:40
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@nodejs-github-bot
Copy link
Collaborator

@benjamingr benjamingr force-pushed the events-get-listeners branch from b104cad to 6ac986b Compare November 8, 2020 12:37
@@ -829,6 +829,39 @@ class MyClass extends EventEmitter {
}
```

### `events.getEventListeners(emitterOrTarget, eventName)`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### `events.getEventListeners(emitterOrTarget, eventName)`
## `events.getEventListeners(emitterOrTarget, eventName)`

@benjamingr benjamingr force-pushed the events-get-listeners branch from 6ac986b to 7bf4ac3 Compare November 8, 2020 13:46
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@nodejs-github-bot
Copy link
Collaborator

@benjamingr
Copy link
Member Author

@addaleax @Trott @mcollina any chance this can get a second LGTM to not wait a week :]?

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2020

Landed in ad98cf0...1a6d4dc

@github-actions github-actions bot closed this Nov 9, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #35991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
PR-URL: #35991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
danielleadams added a commit that referenced this pull request Nov 9, 2020
Notable changes:

* events: getEventListeners static (Benjamin Gruenbaum) (#35991)
* fs: add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911)

PR URL: #36055
danielleadams added a commit that referenced this pull request Nov 10, 2020
Notable changes:

* events: getEventListeners static (Benjamin Gruenbaum) (#35991)
* fs: add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911)

PR URL: #36055
danielleadams added a commit that referenced this pull request Nov 10, 2020
Notable changes:

* events:
  * getEventListeners static (Benjamin Gruenbaum) (#35991)
* fs:
  * support abortsignal in writeFile (Benjamin Gruenbaum) (#35993)
  * add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911)
* stream:
  * fix thrown object reference (Gil Pedersen) (#36065)

PR URL: #36055
danielleadams added a commit that referenced this pull request Nov 10, 2020
Notable changes:

* events:
  * getEventListeners static (Benjamin Gruenbaum) (#35991)
* fs:
  * support abortsignal in writeFile (Benjamin Gruenbaum) (#35993)
  * add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911)
* stream:
  * fix thrown object reference (Gil Pedersen) (#36065)

PR URL: #36055
danielleadams added a commit that referenced this pull request Nov 10, 2020
Notable changes:

* events:
  * getEventListeners static (Benjamin Gruenbaum) (#35991)
* fs:
  * support abortsignal in writeFile (Benjamin Gruenbaum) (#35993)
  * add support for AbortSignal in readFile (Benjamin Gruenbaum) (#35911)
* stream:
  * fix thrown object reference (Gil Pedersen) (#36065)

PR URL: #36055
Copy link

@sperez841 sperez841 left a comment

Choose a reason for hiding this comment

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

targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
PR-URL: nodejs#35991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
PR-URL: nodejs#35991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
PR-URL: nodejs#35991
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
PR-URL: #35991
Backport-PR-URL: #38386
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
deokjinkim added a commit to deokjinkim/node that referenced this pull request Apr 28, 2023
Use `getEventListeners` instead of `listenerCount` because nodejs#35991
was landed.

Refs: nodejs#35991
Refs: nodejs#36006
nodejs-github-bot pushed a commit that referenced this pull request May 3, 2023
Use `getEventListeners` instead of `listenerCount` because #35991
was landed.

Refs: #35991
Refs: #36006
PR-URL: #47759
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit that referenced this pull request May 3, 2023
Use `getEventListeners` instead of `listenerCount` because #35991
was landed.

Refs: #35991
Refs: #36006
PR-URL: #47759
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
targos pushed a commit that referenced this pull request May 3, 2023
Use `getEventListeners` instead of `listenerCount` because #35991
was landed.

Refs: #35991
Refs: #36006
PR-URL: #47759
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
Use `getEventListeners` instead of `listenerCount` because #35991
was landed.

Refs: #35991
Refs: #36006
PR-URL: #47759
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
Use `getEventListeners` instead of `listenerCount` because nodejs#35991
was landed.

Refs: nodejs#35991
Refs: nodejs#36006
PR-URL: nodejs#47759
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants