Skip to content

events,lib: don't require EE#listenerCount() - #2661

Merged
Fishrock123 merged 1 commit into
nodejs:masterfrom
Fishrock123:ee-static-listenerCount
Sep 3, 2015
Merged

events,lib: don't require EE#listenerCount()#2661
Fishrock123 merged 1 commit into
nodejs:masterfrom
Fishrock123:ee-static-listenerCount

Conversation

@Fishrock123

@Fishrock123 Fishrock123 commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

This makes it so certain public APIs of ours no longer require EventEmitters to have a listenerCount() on their prototype.

Fixes: #2655

See: 8f58fb9

I don't have the time to write other tests right now and i'm not sure they are necessary.

@Fishrock123 Fishrock123 added the events Issues and PRs related to the events subsystem / EventEmitter. label Sep 2, 2015
@Fishrock123 Fishrock123 added this to the 4.0.0 milestone Sep 2, 2015
cjihrig
cjihrig reviewed Sep 2, 2015
View reviewed changes
Comment thread test/parallel/test-stream-pipe-without-listenerCount.js Outdated

@cjihrig cjihrig Sep 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you const these.

@thefourtheye

thefourtheye commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

LGTM

chrisdickinson
chrisdickinson reviewed Sep 2, 2015
View reviewed changes
Comment thread lib/_http_client.js Outdated

@chrisdickinson chrisdickinson Sep 2, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

req.listenerCount should be fine, unless userland code is punching the _httpMessage property.

@chrisdickinson

chrisdickinson commented Sep 2, 2015

Copy link
Copy Markdown
Contributor

Hm, re-reading the events that led up to this PR, it seems like deprecating EE.listenerCount and moving to ee.listenerCount is perhaps not desirable?

@Fishrock123

Fishrock123 commented Sep 2, 2015

Copy link
Copy Markdown
Contributor Author

@chrisdickinson How so? This is only to keep things from breaking while we message a move to #listenerCount().

@jasnell

jasnell commented Sep 2, 2015

Copy link
Copy Markdown
Member

Over all this is fine. The only real question I would have is what, if any impact this would have a event listeners that have implemented their own listenerCount. Would anything break? Given the intent and general use, I'd be very surprised if anything would.

@Fishrock123

Fishrock123 commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

Would anything break?

I guess?

Hmm, we could check if listenerCount() exists.

@rvagg rvagg mentioned this pull request Sep 3, 2015
Closed
10 tasks
@Fishrock123
Fishrock123 force-pushed the ee-static-listenerCount branch from 129693c to dc12f42 Compare September 3, 2015 13:49
@Fishrock123

Fishrock123 commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

Updated @chrisdickinson / @jasnell / @cjihrig PTAL

@cjihrig

cjihrig commented Sep 3, 2015

Copy link
Copy Markdown
Contributor

LGTM if the CI is happy

@Fishrock123

Fishrock123 commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

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

@orangemocha

orangemocha commented Sep 3, 2015

Copy link
Copy Markdown
Contributor

Failed linting. Tests still in progress. https://ci.nodejs.org/job/node-linter/474/nodes=linter/console

@jasnell

jasnell commented Sep 3, 2015

Copy link
Copy Markdown
Member

LGTM so long as everything passes!

@jasnell

jasnell commented Sep 3, 2015

Copy link
Copy Markdown
Member

Ok, given that the CI is still having issues, this will need to land manually. Locally tests on my end show this passing all tests so LGTM. @Fishrock123, want to go ahead and get this landed the manual way?

@Fishrock123
Fishrock123 force-pushed the ee-static-listenerCount branch from dc12f42 to f21fd3b Compare September 3, 2015 18:16
@Fishrock123

Fishrock123 commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

Updated to fix the linter. CI to land: https://ci.nodejs.org/job/node-accept-pull-request/123/

@orangemocha

orangemocha commented Sep 3, 2015

Copy link
Copy Markdown
Contributor

The CI run looks green, but it's probably doomed if others have pushed to the same branch manually in the meantime.

@Fishrock123

Fishrock123 commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author 
not ok 69 - test-cluster-dgram-2.js
# worker 1 sent 10 packets
# worker 2 sent 10 packets
# worker 3 sent 10 packets
# worker 4 sent 10 packets
# master received 40 packets

@orangemocha ... ok seriously what is going on here? This is expected output....

@Fishrock123

Fishrock123 commented Sep 3, 2015

Copy link
Copy Markdown
Contributor Author

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

@Fishrock123
events,lib: don't require EE#listenerCount()
b513a33 
Now parts of our public and public-ish APIs fall back to old-style
listenerCount() if the emitter does not have a listenerCount function.

Fixes: nodejs#2655
Refs: 8f58fb9

PR-URL: nodejs#2661
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Fishrock123
Fishrock123 force-pushed the ee-static-listenerCount branch from f21fd3b to b513a33 Compare September 3, 2015 21:37
@Fishrock123
Fishrock123 merged commit b513a33 into nodejs:master Sep 3, 2015
Fishrock123 added a commit that referenced this pull request Sep 3, 2015
@Fishrock123
events,lib: don't require EE#listenerCount()
8391e1b 
Now parts of our public and public-ish APIs fall back to old-style
listenerCount() if the emitter does not have a listenerCount function.

Fixes: #2655
Refs: 8f58fb9

PR-URL: #2661
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 3, 2015
@Fishrock123
events,lib: don't require EE#listenerCount()
b30f393 
Now parts of our public and public-ish APIs fall back to old-style
listenerCount() if the emitter does not have a listenerCount function.

Fixes: nodejs#2655
Refs: 8f58fb9

PR-URL: nodejs#2661
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg rvagg mentioned this pull request Sep 5, 2015
Merged
Fishrock123 added a commit that referenced this pull request Sep 6, 2015
@Fishrock123 @rvagg
events,lib: don't require EE#listenerCount()
daf6c53 
Now parts of our public and public-ish APIs fall back to old-style
listenerCount() if the emitter does not have a listenerCount function.

Fixes: #2655
Refs: 8f58fb9

PR-URL: #2661
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 added a commit that referenced this pull request Sep 6, 2015
@Fishrock123 @rvagg
events,lib: don't require EE#listenerCount()
f3696f6 
Now parts of our public and public-ish APIs fall back to old-style
listenerCount() if the emitter does not have a listenerCount function.

Fixes: #2655
Refs: 8f58fb9

PR-URL: #2661
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 added a commit that referenced this pull request Sep 6, 2015
@Fishrock123 @rvagg
events,lib: don't require EE#listenerCount()
1f0b51a 
Now parts of our public and public-ish APIs fall back to old-style
listenerCount() if the emitter does not have a listenerCount function.

Fixes: #2655
Refs: 8f58fb9

PR-URL: #2661
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 added a commit that referenced this pull request Sep 6, 2015
@Fishrock123 @rvagg
events,lib: don't require EE#listenerCount()
46b7d15 
Now parts of our public and public-ish APIs fall back to old-style
listenerCount() if the emitter does not have a listenerCount function.

Fixes: #2655
Refs: 8f58fb9

PR-URL: #2661
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg rvagg mentioned this pull request Sep 7, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

events Issues and PRs related to the events subsystem / EventEmitter.

Projects

None yet

Milestone

4.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Fishrock123 @thefourtheye @chrisdickinson @jasnell @cjihrig @orangemocha