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.

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

doc: update removeListener behaviour #5201

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
36 changes: 36 additions & 0 deletions doc/api/events.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,42 @@ 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`
call *after* emitting and *before* the last listener finishes execution will
not affect the current listener array. Subsequent events will behave as expected.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


```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:
// B
// 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.


// 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
22 changes: 22 additions & 0 deletions test/parallel/test-event-emitter-remove-listeners.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,3 +83,25 @@ e5.once('removeListener', common.mustCall(function(name, cb) {
}));
e5.removeListener('hello', listener1);
assert.deepEqual([], e5.listeners('hello'));

const e6 = new events.EventEmitter();

var listener3 = common.mustCall(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

These vars can now be consts.

console.log('listener3');
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for console.log()s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig Anything else ?

e6.removeListener('hello', listener4);
}, 2);

var listener4 = common.mustCall(() => {
console.log('listener4');
}, 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');