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

[DavidCai1111]

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'constEventEmitter=require('events')letee=newEventEmitter()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'constEventEmitter=require('events')letee=newEventEmitter()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'constEventEmitter=require('events')letee=newEventEmitter()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

[bnoordhuis]

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

[addaleax]

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]

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

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]

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

[DavidCai1111]

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

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]

I think so, too : = )

[cjihrig]

This makes sense, but could you add tests.

[DavidCai1111]

@cjihrig 👌

[DavidCai1111]

@cjihrig Already added : = )

[sam-github]

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]

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

[cjihrig]

Both assertions can be assert.strictEqual().

[cjihrig]

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]

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

[sam-github]

I lean towards patch, too.

[jasnell]

Related (covers the same ground): #5564

[DavidCai1111]

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

[simonkcleung]

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.

[jasnell]

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

[jasnell]

LGTM with a nit. Pending CI of course.

[jasnell]

Looks good, landing

[jasnell]

Landed in 706778a

[mscdex]

Shouldn't this be tagged as semver-major?

[cjihrig]

Yes. Done.

[cjihrig]

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]

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

[jasnell]

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

[jasnell]

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

[MylesBorins]

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

[MylesBorins]

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

[sam-github]

I think its reasonable to backport