doc: update removeListener behaviour #5201

vaibhav93 · 2016-02-12T10:31:42Z

This commit updates events doc to describe removeListener behaviour
when it is called within a listener. An example is added to make it
more evident.

A test is also incuded to make this behaviour consistent in future
releases.
ref #4764
fixes #4759
@cjihrig @jasnell

This commit updates events doc to describe removeListener behaviour when it is called within a listener. An example is added to make it more evident. A test is also incuded to make this behaviour consistent in future releases. fixes nodejs#4759

tflanagan · 2016-02-12T11:11:26Z

doc/api/events.markdown

+not affect the current listener array. Subsequent events will behave as expected.
+
+```js
+const myEmitter = new MyEmitter();


There is a PR in place that validates code blocks in the docs per eslint rules.

Could you please add

const EventEmitter = require('events'); class MyEmitter extends EventEmitter {} const myEmitter = new MyEmitter();

const EventEmitter = require('events'); class MyEmitter extends EventEmitter {}

This initialization has been done in the first example of events doc and from there on each example simply uses myEmitter . I was trying to be consistent with the existing doc.
As for the linting of code blocks in docs, I'll do that at once.

Don't worry about the linting of other code blocks, only of the one you are adding in this PR.

Each code block is a separate file in linting world, so yes it was declared above, but to satisfy linting, it needs to be declared in this block as well.

So,

const EventEmitter = require('events'); class MyEmitter extends EventEmitter {}

should be added only for the sake of linting that block and then removed before the commit. Or should that piece of code be pushed assuming linted docs would be adopted in the future.

The latter, but hold up. @targos just made a comment about reducing the strictness of the linter.

I'd say ignore what I'm saying right now, if I get these changes landed I will fix this later.

cjihrig · 2016-02-12T15:07:01Z

test/parallel/test-event-emitter-remove-listeners.js

@@ -83,3 +83,29 @@ e5.once('removeListener', common.mustCall(function(name, cb) {
 }));
 e5.removeListener('hello', listener1);
 assert.deepEqual([], e5.listeners('hello'));
+
+var e6 = new events.EventEmitter();


Please use const here.

Add spaces before beginning of comments.

Improve code by replacing count variable to assert number of function calls with mustCall method from common module. Also use ES6 arrow function decalaration for anonymous functions.

vaibhav93 · 2016-02-12T16:33:54Z

@cjihrig Any more corrections ?

mscdex · 2016-02-12T16:38:05Z

doc/api/events.markdown

+Note that once an event has been emitted, all listeners attached to it at the
+time of emitting will be called in order. This implies that any `removeListener`
+call *after* emitting and *before* the last listener finishes execution will
+not affect the current listener array. Subsequent events will behave as expected.


Perhaps "will not affect the current listener array" is a bit misleading. To me it sounds like it's saying that listeners won't be removed at all, when in fact they actually are removed but it just doesn't affect the list of listeners used by emit(). Maybe it could instead read something like:

Note that once an event has been emitted, all listeners attached to it at the time of emitting will be called in order. Any calls to `removeListener()` or `removeAllListeners()` for this event will not affect the `emit()` in progress.

@mscdex I agree the statement is misleading because the internal array is cloned at the time of emit and listeners are executed from this cloned array. Meanwhile removeListener removes from the internal array. Was not quiet able to put that in words suitable for a doc. Your's definitely looks better.

Correct print order for removeListener test.

cjihrig · 2016-02-12T16:56:36Z

test/parallel/test-event-emitter-remove-listeners.js

+
+const e6 = new events.EventEmitter();
+
+var listener3 = common.mustCall(() => {


These vars can now be consts.

Previous commit was misleading by saying that internal array is not modified by removeListener or removeAllListeners call. Now worded appropriately.

cjihrig · 2016-02-13T18:09:41Z

LGTM

jasnell · 2016-02-15T15:35:14Z

LGTM

benjamingr · 2016-03-13T14:59:42Z

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

This commit updates events doc to describe removeListener behaviour when it is called within a listener. An example is added to make it more evident. A test is also incuded to make this behaviour consistent in future releases. Fixes: #4759 PR-URL: #5201 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

benjamingr · 2016-03-13T16:48:21Z

CI passed.

Landed in f2bd9cd

Thanks for the contribution @vaibhav93 :) - I took the liberty of editing your commit message a bit - note the structure of the commit message and the fact the commits were squished into a single commit for future contributions.

This commit updates events doc to describe removeListener behaviour when it is called within a listener. An example is added to make it more evident. A test is also incuded to make this behaviour consistent in future releases. Fixes: #4759 PR-URL: #5201 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

stepancar · 2023-08-17T20:33:29Z

Guys, but how to remove listeners during notifying subscribers?

stepancar · 2023-08-17T20:33:53Z

it would be nice to understand this design

doc: update removeListener behaviour

7b91ac0

This commit updates events doc to describe removeListener behaviour when it is called within a listener. An example is added to make it more evident. A test is also incuded to make this behaviour consistent in future releases. fixes nodejs#4759

tflanagan reviewed Feb 12, 2016
View reviewed changes

ChALkeR added doc Issues and PRs related to the documentations. events Issues and PRs related to the events subsystem / EventEmitter. labels Feb 12, 2016

cjihrig reviewed Feb 12, 2016
View reviewed changes

vaibhav93 added 2 commits February 12, 2016 20:55

doc: fix comment typo in events

e7dffb0

Add spaces before beginning of comments.

test: use mustCall instead of count variable

a21be7c

Improve code by replacing count variable to assert number of function calls with mustCall method from common module. Also use ES6 arrow function decalaration for anonymous functions.

mscdex reviewed Feb 12, 2016
View reviewed changes

doc: fix typo in events

b60d25f

Correct print order for removeListener test.

cjihrig reviewed Feb 12, 2016
View reviewed changes

doc: Improve wording for removeListener text

9ae5a70

Previous commit was misleading by saying that internal array is not modified by removeListener or removeAllListeners call. Now worded appropriately.

jasnell added the lts-watch-v4.x label Feb 15, 2016

silverwind added the test Issues and PRs related to the tests. label Feb 25, 2016

benjamingr closed this Mar 13, 2016

evanlucas mentioned this pull request Mar 14, 2016

Release proposal: v5.8.1 #5698

Closed

evanlucas mentioned this pull request Mar 14, 2016

v5.9.0 proposal #5702

Merged

MylesBorins added the land-on-v4.x label Mar 17, 2016

MylesBorins removed the lts-watch-v4.x label Mar 17, 2016

benjamingr mentioned this pull request Mar 20, 2016

doc: emitter.removeListener within emit cycle clarification #4764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: update removeListener behaviour #5201

doc: update removeListener behaviour #5201

vaibhav93 commented Feb 12, 2016

tflanagan Feb 12, 2016

vaibhav93 Feb 12, 2016

tflanagan Feb 12, 2016

vaibhav93 Feb 12, 2016

tflanagan Feb 12, 2016

cjihrig Feb 12, 2016

vaibhav93 commented Feb 12, 2016

mscdex Feb 12, 2016

vaibhav93 Feb 12, 2016

cjihrig Feb 12, 2016

cjihrig commented Feb 13, 2016

jasnell commented Feb 15, 2016

benjamingr commented Mar 13, 2016

benjamingr commented Mar 13, 2016

stepancar commented Aug 17, 2023

stepancar commented Aug 17, 2023


		const e6 = new events.EventEmitter();

		var listener3 = common.mustCall(() => {

doc: update removeListener behaviour #5201

doc: update removeListener behaviour #5201

Conversation

vaibhav93 commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaibhav93 commented Feb 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Feb 13, 2016

jasnell commented Feb 15, 2016

benjamingr commented Mar 13, 2016

benjamingr commented Mar 13, 2016

stepancar commented Aug 17, 2023

stepancar commented Aug 17, 2023