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

events: pass the original listener added by EventEmitter#once to the removeListener handler #6394

Closed
wants to merge 1 commit into from

Conversation

DavidCai1111
Copy link
Member

@DavidCai1111 DavidCai1111 commented Apr 26, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
  • a test and/or benchmark is included
Description of change

We use the _onceWrap function to wrap the listener added by EventEmitter#once, and use the flag fired inside to make sure the listener will only be called once, including the calling in the removeListener event:

'use strict'
const EventEmitter = require('events')

let ee = new EventEmitter()

ee.once('test', () => console.log('test'))

ee.on('removeListener', (eventName, listener) => {
  console.log(`eventName: ${eventName}`)
  listener.call(ee)
  listener.call(ee)
  listener.call(ee)
})

ee.emit('test')
// eventName: test
// test

But by explicitly invoking the listener in the removeListener handler the user expresses a pretty strong intent that the listener should be called. By now we can use a listener.listener trick to archive this:

'use strict'
const EventEmitter = require('events')

let ee = new EventEmitter()

ee.once('test', () => console.log('test'))

ee.on('removeListener', (eventName, listener) => {
  console.log(`eventName: ${eventName}`)
  listener.listener.call(ee)
  listener.listener.call(ee)
  listener.listener.call(ee)
})

ee.emit('test')
// eventName: test
// test
// test
// test
// test

But those who have not read the the code of lib/events.js should not know this trick at all, and using tricks is alway not a good way. so this PR is to pass the original listener added by EventEmitter#once to the removeListener handler:

'use strict'
const EventEmitter = require('events')

let ee = new EventEmitter()

ee.once('test', () => console.log('test'))

ee.on('removeListener', (eventName, listener) => {
  console.log(`eventName: ${eventName}`)
  listener()
  listener()
  listener()
})

ee.emit('test')
// eventName: test
// test
// test
// test
// test

@addaleax addaleax added the events Issues and PRs related to the events subsystem / EventEmitter. label Apr 26, 2016
@bnoordhuis
Copy link
Member

Why would we go through the trouble of trying to prevent that? Using the delete operator like that isn't exactly free, either.

@addaleax
Copy link
Member

I’d say by explicitly invoking the listener in the removeListener handler the user expresses a pretty strong intent that the listener should be called.

@DavidCai1111
Copy link
Member Author

DavidCai1111 commented Apr 26, 2016

@addaleax But as the output showed in the first code example, no matter invoking the listener in the removeListener handler how many times, it will only be called once eventually, unless we do the listener.listener trick.

@addaleax
Copy link
Member

I have to admit, I find the PR title here a bit confusing then.

I think a much cleaner solution (and more aligned with user expectations?) would be to pass the original listener which has been passed to .once() to the removeListener handler, though. That has the additional benifit of no extra work in the case that there is no removeListener handler.

@DavidCai1111
Copy link
Member Author

@addaleax It's a better solution, i will change the code.

@DavidCai1111 DavidCai1111 changed the title events: make sure the listener added by EventEmitter#once will only be called once events: pass the original listener added by EventEmitter#once to the removeListener handler Apr 26, 2016
@DavidCai1111
Copy link
Member Author

@addaleax Hey bro, I've changed the code and the content of this PR. Now the user will get the original listener which added by EventEmitter#once in removeListener handler 👍

@addaleax
Copy link
Member

I like the change, but would like to head other’s opinions on this. I think this could be considered a bugfix, because this is how I’d understand the documentation (if I didn’t know how .once() was implemented under the hood) and it aligns removeListener with newListener.

Hey bro

Appreciate you not wanting to be formal, but I’m definitely not a bro, for more than one reason. :)

@DavidCai1111
Copy link
Member Author

I think so, too : = )

@cjihrig
Copy link
Contributor

cjihrig commented Apr 26, 2016

This makes sense, but could you add tests.

@DavidCai1111
Copy link
Member Author

@cjihrig 👌

@DavidCai1111
Copy link
Member Author

@cjihrig Already added : = )

@sam-github
Copy link
Contributor

I had no idea the original function wasn't passed, I'd expect the listener passed to once to be the listener passed in the removeListener event!

@DavidCai1111
Copy link
Member Author

@sam-github It will be, if this PR merged.


const listener5 = () => {};
const listener6 = common.mustCall((eventName, listener) => {
assert.deepStrictEqual(eventName, 'hello');
Copy link
Contributor

Choose a reason for hiding this comment

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

Both assertions can be assert.strictEqual().

@cjihrig
Copy link
Contributor

cjihrig commented Apr 26, 2016

LGTM pending comments and CI.

Are we classifying this as semver major or patch? I'd lean toward patch, but this could potentially break things.

@addaleax
Copy link
Member

Yeah, I think this breaks only code that is already broken. So, leaning towards patch, too.

@sam-github
Copy link
Contributor

I lean towards patch, too.

@jasnell
Copy link
Member

jasnell commented Apr 27, 2016

Related (covers the same ground): #5564

@DavidCai1111
Copy link
Member Author

DavidCai1111 commented Apr 27, 2016

Seems that the solution code in this PR is more lightweight and harmless ?

@simonkcleung
Copy link

Look into #5564
Same problem appeared in EventEmitter.prototype.listeners

line 338: originalListener = list[i].listener;
line 360: this.emit('removeListener', type, originalListener || listener);

change to

originalListener = list[i].listener||listener;
this.emit('removeListener', type, originalListener);

seems more readable.

const e7 = new events.EventEmitter();

const listener5 = () => {};
const listener6 = common.mustCall((eventName, listener) => {
Copy link
Member

Choose a reason for hiding this comment

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

might be clearer to just inline listener6 with the removeListener line below instead of creating a separate decl for it

@jasnell
Copy link
Member

jasnell commented Apr 28, 2016

LGTM with a nit. Pending CI of course.

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

Looks good, landing

jasnell pushed a commit that referenced this pull request Apr 29, 2016
When removing a `once` listener, the listener being passed to
the `removeListener` callback is the wrapper. This unwraps the
listener so that `removeListener` is passed the actual listener.

PR-URL: #6394
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

Landed in 706778a

@mscdex
Copy link
Contributor

mscdex commented Apr 29, 2016

Shouldn't this be tagged as semver-major?

@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 29, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Apr 29, 2016

Yes. Done.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 29, 2016

Wait, I take that back. We talked about this the other day. Most people (myself included) were leaning toward patch. I can see how it would be a major though.

@addaleax
Copy link
Member

That was brought up a few comments up: #6394 (comment)

@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

@mscdex ... it could be see discussion here #6394 (comment) ... the existing behavior can be rightfully considered to be a bug and this is just a fix. I'm -1 on it being semver-major.

@cjihrig cjihrig removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 29, 2016
@jasnell
Copy link
Member

jasnell commented Apr 29, 2016

@mscdex ... to be on the safe side we can hold off on pulling this back to v5 or v4 for a while in case there are any regressions caused.

Fishrock123 pushed a commit that referenced this pull request May 4, 2016
When removing a `once` listener, the listener being passed to
the `removeListener` callback is the wrapper. This unwraps the
listener so that `removeListener` is passed the actual listener.

PR-URL: #6394
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
When removing a `once` listener, the listener being passed to
the `removeListener` callback is the wrapper. This unwraps the
listener so that `removeListener` is passed the actual listener.

PR-URL: nodejs#6394
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

do you think it is about time ot backport @nodejs/lts?

@MylesBorins
Copy link
Contributor

@nodejs/lts @nodejs/ctc thoughts on this being backported?

@sam-github
Copy link
Contributor

I think its reasonable to backport

MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
When removing a `once` listener, the listener being passed to
the `removeListener` callback is the wrapper. This unwraps the
listener so that `removeListener` is passed the actual listener.

PR-URL: #6394
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants