-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Conversation
(there's an alternate proposed fix here: https://github.com/zetxx/node/commit/c691bf340effde7974e25e3be2aa409bf14 c2012)
This reverts commit 8360257.
This reverts commit f5620ba.
nodejs#8853 ... specifically, acknowledge in the docs that EE.addListener(...) does not check to see if the given listener has already been added, and that newListener will be called regardless of whether an already existing listener is added twice.
Adds a listener to the end of the listeners array for the specified `event`. | ||
No checks are made to see if the `listener` has already been added. Multiple | ||
calls passing the same combination of `event` and `listener` will result | ||
in the `listener` being added multiple times. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this its obvious that when you add a listener it adds a listener. Always. But, it is clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but what wasn't obvious (per the issue I was addressing) is that a single listener can be added multiple times.
You need to rebase an cleanup the branch, and fix your commit messages into the standard style. |
@@ -121,8 +129,9 @@ Return the number of listeners for a given event. | |||
* `event` {String} The event name | |||
* `listener` {Function} The event handler function | |||
|
|||
This event is emitted any time someone adds a new listener. It is unspecified | |||
if `listener` is in the list returned by `emitter.listeners(event)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence doesn't mean what you think it did. Its trying, poorly to say that there is not an event ordering between when the newListener event is emitted, and the listener is added to the array. The API is trying to formally weasly, allowing the EE implementation to choose whether to do do:
function addListener(event, listener) {
// emit('newListener', ... here?
this.listeners[event].push(listener)
// emit('newListener', ... here?
}
It would be much better for node to commit to doing either one, or the other, and document the event as occuring either "before" the listener is added or occuring "after" the listener is added, rather than telling people they should not rely on the order. Undefined event ordering is the bane of asynchronous development!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Then you should say what you mean,' the March Hare went on." - https://www.cs.cmu.edu/~rgs/alice-VII.html
If that's what it was trying to say it was rather a poor attempt at it indeed.
Will close this one and redo to clean things up a bit more... |
Documentation fix for #8853 ... specifically, acknowledge in the docs that EE.addListener(...) does not check to see if the given listener has already been added, and that newListener will be called regardless of whether an already existing listener is added twice.