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: update removeListener behaviour #5201

Closed
wants to merge 5 commits into from
Closed

doc: update removeListener behaviour #5201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b91ac0
doc: update removeListener behaviour
vaibhav93 Feb 12, 2016
e7dffb0
doc: fix comment typo in events
vaibhav93 Feb 12, 2016
a21be7c
test: use mustCall instead of count variable
vaibhav93 Feb 12, 2016
b60d25f
doc: fix typo in events
vaibhav93 Feb 12, 2016
9ae5a70
doc: Improve wording for removeListener text
vaibhav93 Feb 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions doc/api/events.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,43 @@ listener array. If any single listener has been added multiple times to the
listener array for the specified `event`, then `removeListener` must be called
multiple times to remove each instance.

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()`
or `removeAllListeners()` calls *after* emitting and *before* the last listener
finishes execution will not remove them from `emit()` in progress. Subsequent
events will behave as expected.

```js
const myEmitter = new MyEmitter();
Copy link
Contributor

Choose a reason for hiding this comment

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

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


var callbackA = () => {
console.log('A');
myEmitter.removeListener('event', callbackB);
};

var callbackB = () => {
console.log('B');
};

myEmitter.on('event', callbackA);

myEmitter.on('event', callbackB);

// callbackA removes listener callbackB but it will still be called.
// Interal listener array at time of emit [callbackA, callbackB]
myEmitter.emit('event');
// Prints:
// A
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this backwards? It should be A followed by B.

// B

// callbackB is now removed.
// Interal listener array [callbackA]
myEmitter.emit('event');
// Prints:
// A

```

Because listeners are managed using an internal array, calling this will
change the position indices of any listener registered *after* the listener
being removed. This will not impact the order in which listeners are called,
Expand Down
19 changes: 19 additions & 0 deletions test/parallel/test-event-emitter-remove-listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,22 @@ e5.once('removeListener', common.mustCall(function(name, cb) {
}));
e5.removeListener('hello', listener1);
assert.deepEqual([], e5.listeners('hello'));

const e6 = new events.EventEmitter();

const listener3 = common.mustCall(() => {
e6.removeListener('hello', listener4);
}, 2);

const listener4 = common.mustCall(() => {}, 1);

e6.on('hello', listener3);
e6.on('hello', listener4);

// listener4 will still be called although it is removed by listener 3.
e6.emit('hello');
// This is so because the interal listener array at time of emit
// was [listener3,listener4]

// Interal listener array [listener3]
e6.emit('hello');