Events

[asturur]

Modify how event listeners get deactivted.

Our current implementation slice the array for listeners in the moment it gets set to "off". The array shift and either it throws an error or it jumps a currently registered event.

While the use case are rare and often are just bad implementation, having a more robust code should not be problematic at performance level.

Solution proposed are:
clone the eventListener array before iterating over it
or
mark the events for deletion and delete them at the end of the trigger
closes #2706

[asturur]

@issein, @kangax this a draft for the event issue.
I will setup 2 fiddle on working with the old system and one with this.
I would like you to help me found edge cases where this may still fail or expose new problems that the old did not have.

I'll start adding some failing cases.

[asturur]

@inssein @kangax @Kienz, i'm for merging this, because it makes the tests behave better than the current situation. I'm not sure about performance implications. For the "fire" part we added a check for every function and a array.filter at the end.

[asturur]

This will not save us from everything. Some bad implementation could fire Event "A" , and then in an handler of "A" , delete some other "A" handlers and fire "A" again before the first one finished firing.

The sub event "A" would finish, clear the array from the false one, and then the outerloop would get an error because the array is shortened.
But this looks like a specially crafted logic trap more than a normal use.

Tracking wich event is being fired adds comeplexity, we should have an array of firing events, at the end of the loop we should check if the event is still firing and just in that moment clear the events marked for deletion.

[inssein]

return value !== false

[asturur]

of course ;p
just playing around trying to make a test fail.

[inssein]

@asturur after reading the code, I am still unsure about this. This is not a new problem, and every library I saw solves this problem the same way, by cloning the array before it loops over to fire the events. Maybe we should just try it and see how big of a performance impact it is.

[asturur]

I'm unsure as well, that is why I'm discussing with you.
The point is that with cloning we do not have immediate stop of the event.
If we have 2 handlers for an event, and from the first we deactivate the second, with the actual implementation and this one, the second handler is currently stopped.
(in current situation it throws error).
If we clone array, at the first iteration the second handler is executed anyway.

Just to add more doubts, "off" is an alias of "stopObserving". But "off" in my opinion would lead to immediate stop, while effectively "stopObserving" could suggest something different, like stop a the next time the event fires.

Only thing shure is that we should have same behaving for adding and stopping handlers.
Current implementation, error aparts, was mixed: removing immediate and adding for the next firing.

[inssein]

All the event emitter implementations I have seen have that problem (where the second listener will still fire if removed by the first). It doesn't really seem like a realistic scenario, and can't really see a valid use case for it.

[asturur]

I'm gonna merge this tomorrow, just leaving @kangax and @Kienz some time for theyr precious two cents :)

Behaviour is same as before but without errors.
If we wanna change the behaviour and do not stop events immediately, cloning is a better option as inssein said.