Skip to content

Commit

Permalink
Remove ability to pass array of event names to EventEmitter.prototype…
Browse files Browse the repository at this point in the history
….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.
  • Loading branch information
lawrence-forooghian committed Oct 2, 2023
1 parent 21528ac commit 9137e98
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 40 deletions.
15 changes: 1 addition & 14 deletions src/utilities/EventEmitter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ describe('EventEmitter', () => {
eventEmitter['once'](altListener);
eventEmitter['once']('myEvent', context.spy);
eventEmitter['once']('myEvent', altListener);
eventEmitter['once'](['myEvent', 'myOtherEvent', 'myThirdEvent'], altListener);
context.eventEmitter = eventEmitter;
});

Expand Down Expand Up @@ -171,7 +170,7 @@ describe('EventEmitter', () => {
context.eventEmitter['emit']('myEvent', '');
expect(context.spy).toHaveBeenCalledTimes(2);
context.eventEmitter['emit']('myOtherEvent', '');
expect(context.spy).toHaveBeenCalledTimes(3);
expect(context.spy).toHaveBeenCalledTimes(4);
});

it('removes a specific listener from multiple events', () => {
Expand Down Expand Up @@ -254,18 +253,6 @@ describe('EventEmitter', () => {
context.eventEmitter['emit']('myEvent', '');
expect(context.spy).toHaveBeenCalledOnce();
});

it('adds a listener to multiple eventOnce fields on calling `once` with a listener and event name; and after emitting any of the events, all are removed', (context) => {
context.eventEmitter['once'](['myEvent', 'myOtherEvent', 'myThirdEvent'], context.spy);
expect(context.eventEmitter['eventsOnce']['myEvent']).toHaveLength(1);
expect(context.eventEmitter['eventsOnce']['myOtherEvent']).toHaveLength(1);
expect(context.eventEmitter['eventsOnce']['myThirdEvent']).toHaveLength(1);
expect(context.eventEmitter['emit']('myEvent', ''));
expect(context.eventEmitter['eventsOnce']['myEvent']).toBe(undefined);
expect(context.eventEmitter['eventsOnce']['myOtherEvent']).toBe(undefined);
expect(context.eventEmitter['eventsOnce']['myThirdEvent']).toBe(undefined);
expect(context.spy).toHaveBeenCalledOnce();
});
});

describe('calling the emit method', () => {
Expand Down
33 changes: 7 additions & 26 deletions src/utilities/EventEmitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,46 +223,27 @@ export default class EventEmitter<T extends EventMap> {

/**
* Listen for a single occurrence of an event
* @param listenerOrEvents (optional) the name of the event to listen to
* @param listenerOrEvent (optional) the name of the event to listen to
* @param listener (optional) the listener to be called
*/
once<K extends EventKey<T>>(
listenerOrEvents: K | K[] | EventListener<T[K]>,
listenerOrEvent: K | EventListener<T[K]>,
listener?: EventListener<T[K]>,
): void | Promise<any> {
// .once("eventName", () => {})
if (isString(listenerOrEvents) && isFunction(listener)) {
const listeners = this.eventsOnce[listenerOrEvents] || (this.eventsOnce[listenerOrEvents] = []);
if (isString(listenerOrEvent) && isFunction(listener)) {
const listeners = this.eventsOnce[listenerOrEvent] || (this.eventsOnce[listenerOrEvent] = []);
listeners.push(listener);
return;
}

// .once(["eventName"], () => {})
if (isArray(listenerOrEvents) && isFunction(listener)) {
const self = this;

listenerOrEvents.forEach(function (eventName) {
const listenerWrapper: EventListener<T[K]> = function (this: EventListener<T[K]>, listenerThis) {
const innerArgs = Array.prototype.slice.call(arguments) as [params: T[K]];
listenerOrEvents.forEach((eventName) => {
self.off(eventName, this);
});

listener.apply(listenerThis, innerArgs);
};
self.once(eventName, listenerWrapper);
});

return;
}

// .once(() => {})
if (isFunction(listenerOrEvents)) {
this.anyOnce.push(listenerOrEvents);
if (isFunction(listenerOrEvent)) {
this.anyOnce.push(listenerOrEvent);
return;
}

throw new InvalidArgumentError('EventEmitter.once(): invalid arguments:' + inspect([listenerOrEvents, listener]));
throw new InvalidArgumentError('EventEmitter.once(): invalid arguments:' + inspect([listenerOrEvent, listener]));
}

/**
Expand Down

0 comments on commit 9137e98

Please sign in to comment.