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.

Sign up for GitHub

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

Jump to bottom

doc: Infinity for emitter.setMaxListeners #2559

Closed
wants to merge 2 commits into from
Closed

doc: Infinity for emitter.setMaxListeners #2559

wants to merge 2 commits into from

Conversation

jasonkarns
Copy link
Member

Instead of recommending 0 as the magic value to set max listeners to unlimited, recommend Infinity.

This paves the way for 0 as a magic value eventually being deprecated and finally removed

original discussion: nodejs/node-v0.x-archive#22987
original PR: nodejs/node-v0.x-archive#25269

@jasonkarns
doc: Infinity for emitter.setMaxListeners
353646f 
Instead of recommending `0` as the magic value to set max listeners to unlimited, recommend `Infinity`.

This paves the way for `0` as a magic value eventually being deprecated and finally removed

closes nodejs/node-v0.x-archive#22987
@jasonkarns jasonkarns mentioned this pull request Aug 26, 2015
Closed
@cjihrig
Copy link
Contributor

cjihrig commented Aug 26, 2015

LGTM

@mscdex mscdex added the doc Issues and PRs related to the documentations. label Aug 26, 2015
@targos
Copy link
Member

targos commented Aug 26, 2015

LGTM, but I don't think we need to ever deprecate/remove the use of 0 as magic value. It is a common paradigm.

@chrisdickinson
Copy link
Contributor

One problem with removing references to setMaxListeners(0) (in favor of Infinity) is that if someone sees that in code and wants to look at the docs to figure out what's going on, the docs won't have an answer for them. Would you be opposed to rewording that to Set to Infinity (or zero) for unlimited.?

@jasonkarns
Copy link
Member Author

@chrisdickinson I'm okay with documenting 0 for a while. Ideally the whole point of this is for people to move away from using 0 at all. But for a transitional period, it makes sense to keep the old pattern documented. wording updated

@brendanashworth
Copy link
Contributor

LGTM.

silverwind pushed a commit that referenced this pull request Oct 9, 2015
@jasonkarns @silverwind
doc: recommend Infinity on emitter.setMaxListeners
775c01e 
Instead of recommending `0` as the magic value to set max listeners to
unlimited, recommend `Infinity`. This paves the way for `0` as a magic
value eventually being deprecated and finally removed.

PR-URL: #2559
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@silverwind
Copy link
Contributor

Thanks! Landed with some slight modifications to the commit message in 775c01e.

@silverwind silverwind closed this Oct 9, 2015
@jasonkarns jasonkarns deleted the document-emitter-setmaxlisteners-infinity branch October 9, 2015 17:45
jasnell pushed a commit that referenced this pull request Oct 10, 2015
@jasonkarns @jasnell
doc: recommend Infinity on emitter.setMaxListeners
0cfc6d3 
Instead of recommending `0` as the magic value to set max listeners to
unlimited, recommend `Infinity`. This paves the way for `0` as a magic
value eventually being deprecated and finally removed.

PR-URL: #2559
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@jasnell
Copy link
Member

jasnell commented Oct 10, 2015

Landed in v4.x with 0cfc6d3

@jasnell jasnell mentioned this pull request Oct 10, 2015
Closed
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
doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jasonkarns @cjihrig @targos @chrisdickinson @brendanashworth @silverwind @jasnell @mscdex @MylesBorins