events: add eventNames() method by jasnell · Pull Request #5617 · nodejs/node · GitHub

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

events

Description of change

Per #1817, there are many modules
that currently abuse the private _events property on EventEmitter.
One of the ways it is used is to determine if a particular event is
being listened for. This adds a simple eventNames() method that
returns an array of the events with currently registered listeners.

/cc @ChALkeR

@ChALkeR ... currently this returns an Array... given the various use cases that I've seen, however, the listenerCount API is typically sufficient. I implemented this quickly given your discussion in #1817. Do you still feel this is necessary?

-1 for the reason you stated: listenerCount() can already handle most needs for a method like this.

@mscdex listenerCount() is not enough in some cases, see, for example, the ultron module from #1817. I will prepare a list of other modules that do something like that.

@jasnell

One of the ways it is used is to determine if a particular event is being listened for.

That usecase is covered by the listenerCount(type) method, iterating over all present event listeners isn't.

FWIW if such a mechanism were to be added, I'd prefer something like just events() to match our existing listeners(), but the problem would be naming clashes if (many) modules use that same property name.

@mscdex .. possible name clashes is exactly why I used listEvents instead of just events... it's a less obvious choice.

@ChALkeR ... couldn't the iterating over all present listeners be handled by listening for the 'newListener' and 'removeListener' events? Sure, it requires a bit more work but it's not too difficult.

@jasnell Not if something receives an eventemitter that already has some listeners after those listeners were attached.

@ChALkeR ... good point. Can you do a scan of existing modules to see if using events() would cause any conflicts?

or even something like eventNames(). IMHO listEvents() sounds like a ("action") function that will print the event names to the console or something.

+1 to eventNames()

Per nodejs#1817, there are many modules that currently abuse the private `_events` property on EventEmitter. One of the ways it is used is to determine if a particular event is being listened for. This adds a simple `listEvents()` method that returns an array of the events with currently registered listeners.

@ChALkeR @mscdex ... updated to use eventNames(). PTAL

Having the return [] on the next line would be more readable IMHO

@mscdex ... updated!

minor nit: this can go inside the if block now.

@mscdex ... fixed!

LGTM if CI is ok: https://ci.nodejs.org/job/node-test-pull-request/1910/

Should this be Names? Why not just events?

Discussed above. Two reasons:

This is less likely to cause conflicts with modules extending EventEmitter.
This is more reasonable name because it correctly describes what this method actually does — it returns the list of event names for the registered event listeners. _events, for an example, is an object that stores all listeners (grouped by event name).

It would be easier to understand what the code that uses this method does without navigating to the docs this way.

@ChALkeR I am really sorry. I straight away started with the code. It makes sense. Thanks for explaining.

@jasnell, @mscdex eventNames is used by some of the modules, but I can't atm give an estimate of how many of those (if any) are properties of eventemitter objects.

Quick grep for \.eventNames[^a-zA-Z_]: https://gist.github.com/ChALkeR/8ff143bd3189b9026d7e.

Sorry for the delay.

Actually, the usage looks low.
The new name LGTM (and the PR itself LGTM too).

This is technically a semver-minor, but I would prefer this to be merged in a major release (i.e. 6.0).

@jasnell It looks like at least one existing test needs to be updated: test-event-emitter-method-names.js

I think I agree with @ChALkeR about landing this in 6.0.

LGTM

Landing as a major SGTM. Always good to be safe. I'll take a look at the
test later on today.
On Mar 13, 2016 4:13 AM, "Brian White" notifications@github.com wrote:

@jasnell https://github.com/jasnell It looks like at least one existing
test needs to be updated: test-event-emitter-method-names.js

I think I agree with @ChALkeR https://github.com/ChALkeR about landing
this in 6.0.

—
Reply to this email directly or view it on GitHub
#5617 (comment).

Updated. New CI : https://ci.nodejs.org/job/node-test-pull-request/1920/

CI is green with on apparently unrelated failure.

semver-minor but marking as don't land on v5 or v4 just to be extra cautious per #5617 (comment) and #5617 (comment)

@jasnell Thanks!

Per #1817, there are many modules that currently abuse the private `_events` property on EventEmitter. One of the ways it is used is to determine if a particular event is being listened for. This adds a simple `eventNames()` method that returns an array of the events with currently registered listeners. PR-URL: #5617 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Landed in b6e3fa7

Shouldn't we check if the symbol property is enumerable events.propertyIsEnumerable(symbol) and add it to the array only if it is?

jasnell 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 Mar 9, 2016

jasnell force-pushed the events-api branch from 934561b to 1ffe275 Compare March 9, 2016 05:34

jasnell added 2 commits March 11, 2016 19:26

Rename to eventNames()

1d173c6

jasnell force-pushed the events-api branch from 1ffe275 to 1d173c6 Compare March 12, 2016 03:44

Update the test case to use eventNames

fe7ffc9

mscdex reviewed Mar 12, 2016
View reviewed changes

address nits

673c837

mscdex reviewed Mar 12, 2016
View reviewed changes

Address nits

fabfa63

thefourtheye reviewed Mar 13, 2016
View reviewed changes

ChALkeR mentioned this pull request Mar 13, 2016

EventEmitter API #1817

Closed

3 tasks

Fix test

e70f2d2

jasnell changed the title events: add listEvents() method events: add eventNames() method Mar 14, 2016

jasnell added dont-land-on-v5.x labels Mar 14, 2016

jasnell closed this Mar 15, 2016

ChALkeR mentioned this pull request Mar 15, 2016

Don't rely on _events, use .eventNames() instead when it's present. primus/ultron#6

Closed

lpinca reviewed Mar 20, 2016
View reviewed changes

ChALkeR mentioned this pull request Apr 27, 2016

doc: fix EventEmitter#eventNames() example #6417

Closed

2 tasks

+                if (events) {
+                  return Object.getOwnPropertyNames(events).concat(
+                    Object.getOwnPropertySymbols(events));
+                } else return [];

+                if (this._eventsCount > 0) {
+                  const events = this._events;
+                  return Object.keys(events).concat(
+                    Object.getOwnPropertySymbols(events));


		Returns `true` if event had listeners, `false` otherwise.

		### emitter.eventNames()

events: add eventNames() method by jasnell · Pull Request #5617 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: add eventNames() method - #5617

events: add eventNames() method#5617jasnell wants to merge 6 commits intonodejs:masternodejs/node:masterfrom jasnell:events-apijasnell/node:events-apiCopy head branch name to clipboard

Conversation

jasnell commented Mar 9, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented Mar 9, 2016

Uh oh!

mscdex commented Mar 9, 2016

Uh oh!

ChALkeR commented Mar 9, 2016

Uh oh!

mscdex commented Mar 9, 2016

Uh oh!

jasnell commented Mar 10, 2016

Uh oh!

ChALkeR commented Mar 10, 2016

Uh oh!

jasnell commented Mar 11, 2016

Uh oh!

mscdex commented Mar 11, 2016

Uh oh!

jasnell commented Mar 11, 2016

Uh oh!

jasnell commented Mar 12, 2016

Uh oh!

mscdex Mar 12, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 12, 2016

Uh oh!

mscdex Mar 12, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Mar 12, 2016

Uh oh!

mscdex commented Mar 12, 2016

Uh oh!

thefourtheye Mar 13, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR Mar 13, 2016

Choose a reason for hiding this comment

Uh oh!

thefourtheye Mar 13, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Mar 13, 2016

Uh oh!

ChALkeR commented Mar 13, 2016

Uh oh!

mscdex commented Mar 13, 2016

Uh oh!

thefourtheye commented Mar 13, 2016

Uh oh!

jasnell commented Mar 13, 2016

Uh oh!

jasnell commented Mar 14, 2016

Uh oh!

jasnell commented Mar 14, 2016

Uh oh!

jasnell commented Mar 14, 2016

Uh oh!

ChALkeR commented Mar 15, 2016

Uh oh!

jasnell commented Mar 15, 2016

Uh oh!

lpinca Mar 20, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

events: add eventNames() method#5617
jasnell wants to merge 6 commits into
nodejs:masterfrom
jasnell:events-api