Remove ability to pass array of event names to EventEmitter.prototype.once
#196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lawrence-forooghian
force-pushed
the
remove-EventEmitter-once-multiple-events
branch
from
e8cda5b
to
e4a4641
Compare
lawrence-forooghian
force-pushed
the
remove-EventEmitter-once-multiple-events
branch
from
e4a4641
to
9137e98
Compare
dpiatek
force-pushed
the
remove-EventEmitter-once-multiple-events
branch
from
9137e98
to
ab13900
Compare
….once
This copies across the change from commit cd4f7b3 in ably-js:
> This functionality is implemented by wrapping the listener argument in
> another listener. This means that the event emitter does not hold a
> reference to the listener argument (other than that held indirectly
> through the wrapper) and so it is not possible to remove this listener
> using `off(..., listener)`.
>
> The client library specification does not specify a version of `once`
> which accepts an array of event names, and we do not advertise it as
> part of the public API. So, I think the simplest thing is to remove this
> functionality.
The removed code in the current commit also had a bug — the second
argument in the removed code `self.off(eventName, this)` was incorrectly
populated and this made the removed code equivalent to
`self.off(eventName)`; that is, it removed _all_ listeners for the given
event name. I believe that the removal of this code accounts for the
increased expected number of calls to context.spy in one of the tests
here. I’m not sure what reasoning led to the previous expected count of
3 (perhaps the expectation was written just based on the number of calls
observed when running the code), but here’s my reasoning for the
expectation of 4 calls:
Before `context.eventEmitter['off']('myEvent', context.spy)`, the
following calls are relevant to context.spy:
- eventEmitter['on'](context.spy);
- eventEmitter['on']('myEvent', context.spy);
- eventEmitter['on'](['myEvent', 'myOtherEvent', 'myThirdEvent'], context.spy);
- eventEmitter['once'](context.spy);
- eventEmitter['once']('myEvent', context.spy);
After `context.eventEmitter['off']('myEvent', context.spy)`, the
following calls are relevant to context.spy:
- eventEmitter['on'](context.spy);
- eventEmitter['on'](['myEvent' /* no longer applies */, 'myOtherEvent', 'myThirdEvent'], context.spy);
- eventEmitter['once'](context.spy);
After `context.eventEmitter['emit']('myEvent', '')`, the following calls
are relevant to context.spy:
- eventEmitter['on'](context.spy);
- eventEmitter['on'](['myEvent' /* no longer applies *\/, 'myOtherEvent', 'myThirdEvent'], context.spy);
And hence calling `context.eventEmitter['emit']('myOtherEvent', '')`
calls context.spy a further two times.
dpiatek
force-pushed
the
remove-EventEmitter-once-multiple-events
branch
from
ab13900
to
eee7507
Compare
dpiatek
approved these changes
Oct 6, 2023
lawrence-forooghian
deleted the
remove-EventEmitter-once-multiple-events
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This copies across the change from ably/ably-js#1453 — see explanation there.
The removed code in the current PR also had a bug — the second argument in the removed code
self.off(eventName, this)was incorrectly populated and this made the removed code equivalent toself.off(eventName); that is, it removed all listeners for the given event name. I believe that the removal of this code accounts for the increased expected number of calls tocontext.spyin one of the tests here. I’m not sure what reasoning led to the previous expected count of 3 (perhaps the expectation was written just based on the number of calls observed when running the code), but here’s my reasoning for the expectation of 4 calls:Before
context.eventEmitter['off']('myEvent', context.spy), the following calls are relevant tocontext.spy:eventEmitter['on'](context.spy);eventEmitter['on']('myEvent', context.spy);eventEmitter['on'](['myEvent', 'myOtherEvent', 'myThirdEvent'], context.spy);eventEmitter['once'](context.spy);eventEmitter['once']('myEvent', context.spy);After
context.eventEmitter['off']('myEvent', context.spy), the following calls are relevant tocontext.spy:eventEmitter['on'](context.spy);eventEmitter['on'](['myEvent' /* no longer applies *\/, 'myOtherEvent', 'myThirdEvent'], context.spy);eventEmitter['once'](context.spy);After
context.eventEmitter['emit']('myEvent', ''), the following calls are relevant tocontext.spy:eventEmitter['on'](context.spy);eventEmitter['on'](['myEvent' /* no longer applies */, 'myOtherEvent', 'myThirdEvent'], context.spy);And hence calling
context.eventEmitter['emit']('myOtherEvent', '')callscontext.spya further two times.