Repository navigation
Prevent event handlers from being triggered after they are "off"ed - #14463
Conversation
Prevents handlers from being fired when emit is triggered right before off is called, but in the same event loop.
kimjoar
left a comment
There was a problem hiding this comment.
Emit being async is strange. But it is what it is, so this change is very much needed. LGTM.
| setTimeout(() => { | ||
| sinon.assert.calledOnce(listenerStub); | ||
| done(); | ||
| }, 100); |
There was a problem hiding this comment.
The emit should trigger on the next tick, so I think it could be a bit shorter.
There was a problem hiding this comment.
You'd think. :( I did as well, and initially had it at 1, but the first test broke. I suspect it's because of the emit chain, and the initial emitChain is set to Promise.resolve, so maybe it actually takes two cycles, not one? I didn't bother digging deeper though and just set it to 100.
btw, I agree this async emit stuff is whacky. Actually like a year ago I had a long chat with @spalger about how to remove the reliance on the angular Promise class in here, in an effort to split out framework agnostic business portions (and a lot of the code I wanted to convert over ended up needing the custom Promise class just because of this event emitter).
The conversation was long ago but if I recall correctly, the whole reason for this complication is to automatically insert digest loops with the use of angular promises, so the UI will automatically update after every handler is called. We did discuss an alternative, which I never got around to implementing, but I think it still involved using Promises to inject digest loops.
Given the recent conversation we had about angular vs native promises (#13855) I think the best way forward would be to revert back to a normal event emitter and get rid of all this confusing promise wrapping, and require angular callbacks to use angular promises for their digest loops, rather than relying on this very deeply nested logic.
Given how deeply entrenched state management/event emitter is in our angular code, it may be risky (though IMO, still worth investigating).
|
I think this should at least go into 6.0 too. |
Prevents handlers from being fired when emit is triggered right before off is called, but in the same event loop.
Prevents handlers from being fired when emit is triggered right before off is called, but in the same event loop.
Prevents handlers from being fired when emit is triggered right before off is called, but in the same event loop.
Prevents handlers from being fired when emit is triggered right before
off is called, but in the same event loop.
Fixes #14462
Not sure how far back I should take this. It's likely existed a long time and just hasn't been exposed until a very recent PR.