doc: update removeListener behaviour by vaibhav93 · Pull Request #5201 · nodejs/node · GitHub

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

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.

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.

@cjihrig Any more corrections ?

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.

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.

LGTM

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>

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>

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

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

               e5.removeListener('hello', listener1);
               assert.deepEqual([], e5.listeners('hello'));
+              var e6 = new events.EventEmitter();


		const e6 = new events.EventEmitter();

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

doc: update removeListener behaviour by vaibhav93 · Pull Request #5201 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: update removeListener behaviour - #5201

doc: update removeListener behaviour#5201vaibhav93 wants to merge 5 commits intonodejs:masternodejs/node:masterfrom vaibhav93:removeListenervaibhav93/node:removeListenerCopy head branch name to clipboard

Conversation

vaibhav93 commented Feb 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vaibhav93 commented Feb 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Feb 13, 2016

Uh oh!

jasnell commented Feb 15, 2016

Uh oh!

benjamingr commented Mar 13, 2016

Uh oh!

benjamingr commented Mar 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

doc: update removeListener behaviour#5201
vaibhav93 wants to merge 5 commits into
nodejs:masterfrom
vaibhav93:removeListener