Adds ability to removeListener when in the same emit loop

[bigtimebuddy]

Overview

This PR fixes a bug introduces a new feature when trying to remove an event listener immediately when the removeListener call present in one of the listeners of the stack currently being called.

Bug Example

var e = new EventEmitter();

function first(){
    console.log("first");
    e.removeListener('update', second);
}
function second(){
    console.log("second");
}
function third(){
   console.log("third");
}
e.on('update', first);
e.on('update', second);
e.on('update', third);
e.emit('update');

// should log: 
//   first
//   third

[coveralls]

Coverage remained the same at 100.0% when pulling 4bc3480 on jiborobot:master into ef31a0b on primus:master.

[jcrugzz]

shouldnt second never be called? we should test that this is the case

[bigtimebuddy]

Copying this test to what's in master (no change from PR), this test fails with TypeError: Cannot read property 'bar' of null. Current master attempts to call second in the same emit call. It's not removed until the next stack.

[lpinca]

The Node.js EventEmitter has the same behavior. It calls second. I'm not sure if it is a bug.

[bigtimebuddy]

The use-case for this was creating an update loop that things could subscribe to. If anything is done synchronously in the callbacks to destroy other listeners to the same event loop, it complained.

@lpinca If not a bug fix, would you consider this a feature update? There are already some divergent things from Node's EventEmitter.

[lpinca]

Here are a couple of relevant discussions:

nodejs/node-v0.x-archive#7872
nodejs/node#1182

This change might have sense as it is probably the expected behavior. It is a breaking change but the next version will be a major anyway as there are other breaking changes in master.
I would like to hear @3rd-Eden opinion here.

[bigtimebuddy]

That change makes sense for NodeJS. One of the Node Foundation comments said "Your best bet might be to use an alternative EE implementation". So that's why I'm here. Would love to see this happen.

[3rd-Eden]

On vacation atm, will look at it once im back

On Aug 11, 2016, at 6:44 PM, Matt Karl [email protected] wrote:

That change makes sense for NodeJS. One of the Node Foundation comments said "Your best bet might be to use an alternative EE implementation". So that's why I'm here. Would love to see this happen.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[lpinca]

@bigtimebuddy can you add docs for this change? Another element in the existing bulleted list at the top of the README should suffice.

[bigtimebuddy]

Sure, you bet.

[3rd-Eden]

I'm a -1 on this and the reason for that is plain simple. Adding this will break compatibility with Node.js as we are no longer handling events in the same way they are using. I can see the use-case here though but I don't know if this edge case is enough to break compatibility for.

[bigtimebuddy]

@3rd-Eden Thanks for chiming in. The README lists a bunch of subtle differences between Node and EE3 where changed implementation or API differs slightly. This doesn't seem like the first breaking-change.

Like you said, this is an edge-case so it will probably not affect many people. It doesn't seem unreasonable to assume that if one removes a listener from within another callback, that should happen immediately not asynchronously.

Any compromises here? Maybe until the next major release, throwing a console.warn or info to let developers know when this happens in their code? Would make it easier for developers to upgrade their code, if this breaking change does, in fact, present issues.

[coveralls]

Coverage remained the same at 100.0% when pulling eff8e91 on jiborobot:master into ef31a0b on primus:master.

[lpinca]

I don't know if it makes sense to put this behavior under an option disabled by default.
A fork might also have sense and shouldn't be that hard to maintain (assuming that upstream updates are still wanted).

[bigtimebuddy]

I have created a fork for this. Thanks for the consideration.