events: add eventNames() method #5617

jasnell · 2016-03-09T05:32:34Z

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

jasnell · 2016-03-09T05:36:01Z

@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?

mscdex · 2016-03-09T09:10:55Z

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

ChALkeR · 2016-03-09T10:22:08Z

@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.

mscdex · 2016-03-09T11:05:00Z

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.

jasnell · 2016-03-10T17:19:23Z

@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.

ChALkeR · 2016-03-10T17:36:09Z

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

jasnell · 2016-03-11T03:20:03Z

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

mscdex · 2016-03-11T03:35:41Z

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

jasnell · 2016-03-11T15:51:51Z

+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.

jasnell · 2016-03-12T03:46:45Z

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

mscdex · 2016-03-12T04:48:05Z

lib/events.js

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


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

jasnell · 2016-03-12T22:21:09Z

@mscdex ... updated!

mscdex · 2016-03-12T22:24:37Z

lib/events.js

@@ -436,6 +436,15 @@ function listenerCount(type) {
 return 0;
 }

+EventEmitter.prototype.eventNames = function() {
+ const events = this._events;


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

jasnell · 2016-03-12T22:42:55Z

@mscdex ... fixed!

mscdex · 2016-03-12T23:02:14Z

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

thefourtheye · 2016-03-13T07:38:40Z

doc/api/events.markdown

@@ -295,6 +295,24 @@ they were registered, passing the supplied arguments to each.

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

+### emitter.eventNames()


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.

ChALkeR · 2016-03-13T07:57:15Z

@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.

ChALkeR · 2016-03-13T08:00:02Z

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).

mscdex · 2016-03-13T11:13:10Z

@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.

thefourtheye · 2016-03-13T11:26:44Z

LGTM

jasnell · 2016-03-13T14:37:49Z

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).

jasnell · 2016-03-14T15:18:22Z

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

jasnell · 2016-03-14T15:51:42Z

CI is green with on apparently unrelated failure.

jasnell · 2016-03-14T15:55:30Z

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

ChALkeR · 2016-03-15T08:05:14Z

@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>

jasnell · 2016-03-15T16:26:46Z

Landed in b6e3fa7

lpinca · 2016-03-20T13:26:03Z

lib/events.js

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


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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

events: add eventNames() method #5617

events: add eventNames() method #5617

jasnell commented Mar 9, 2016

jasnell commented Mar 9, 2016

mscdex commented Mar 9, 2016

ChALkeR commented Mar 9, 2016

mscdex commented Mar 9, 2016

jasnell commented Mar 10, 2016

ChALkeR commented Mar 10, 2016

jasnell commented Mar 11, 2016

mscdex commented Mar 11, 2016

jasnell commented Mar 11, 2016

jasnell commented Mar 12, 2016

mscdex Mar 12, 2016

jasnell commented Mar 12, 2016

mscdex Mar 12, 2016

jasnell commented Mar 12, 2016

mscdex commented Mar 12, 2016

thefourtheye Mar 13, 2016

ChALkeR Mar 13, 2016

thefourtheye Mar 13, 2016

ChALkeR commented Mar 13, 2016

ChALkeR commented Mar 13, 2016

mscdex commented Mar 13, 2016

thefourtheye commented Mar 13, 2016

jasnell commented Mar 13, 2016

jasnell commented Mar 14, 2016

jasnell commented Mar 14, 2016

jasnell commented Mar 14, 2016

ChALkeR commented Mar 15, 2016

jasnell commented Mar 15, 2016

lpinca Mar 20, 2016

		@@ -295,6 +295,24 @@ they were registered, passing the supplied arguments to each.

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

		### emitter.eventNames()

events: add eventNames() method #5617

events: add eventNames() method #5617

Conversation

jasnell commented Mar 9, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

jasnell commented Mar 9, 2016

mscdex commented Mar 9, 2016

ChALkeR commented Mar 9, 2016

mscdex commented Mar 9, 2016

jasnell commented Mar 10, 2016

ChALkeR commented Mar 10, 2016

jasnell commented Mar 11, 2016

mscdex commented Mar 11, 2016

jasnell commented Mar 11, 2016

jasnell commented Mar 12, 2016

mscdex Mar 12, 2016

Choose a reason for hiding this comment

jasnell commented Mar 12, 2016

mscdex Mar 12, 2016

Choose a reason for hiding this comment

jasnell commented Mar 12, 2016

mscdex commented Mar 12, 2016

thefourtheye Mar 13, 2016

Choose a reason for hiding this comment

ChALkeR Mar 13, 2016

Choose a reason for hiding this comment

thefourtheye Mar 13, 2016

Choose a reason for hiding this comment

ChALkeR commented Mar 13, 2016

ChALkeR commented Mar 13, 2016

mscdex commented Mar 13, 2016

thefourtheye commented Mar 13, 2016

jasnell commented Mar 13, 2016

jasnell commented Mar 14, 2016

jasnell commented Mar 14, 2016

jasnell commented Mar 14, 2016

ChALkeR commented Mar 15, 2016

jasnell commented Mar 15, 2016

lpinca Mar 20, 2016

Choose a reason for hiding this comment