diff --git a/shepherd.js/src/evented.ts b/shepherd.js/src/evented.ts index 95f55500f..ae30d73be 100644 --- a/shepherd.js/src/evented.ts +++ b/shepherd.js/src/evented.ts @@ -51,18 +51,18 @@ export class Evented { * @returns */ off(event: string, handler?: AnyHandler) { - if (isUndefined(this.bindings) || isUndefined(this.bindings[event])) { + const bindings = this.bindings?.[event]; + + if (isUndefined(bindings)) { return this; } if (isUndefined(handler)) { delete this.bindings[event]; } else { - this.bindings[event]?.forEach((binding, index) => { - if (binding.handler === handler) { - this.bindings[event]?.splice(index, 1); - } - }); + this.bindings[event] = bindings.filter( + (binding) => binding.handler !== handler + ); } return this; @@ -76,18 +76,30 @@ export class Evented { */ // eslint-disable-next-line @typescript-eslint/no-explicit-any trigger(event: string, ...args: any[]) { - if (!isUndefined(this.bindings) && this.bindings[event]) { - this.bindings[event]?.forEach((binding, index) => { - const { ctx, handler, once } = binding; + const bindings = this.bindings?.[event]; + + if (isUndefined(bindings)) { + return this; + } + + // Iterate over a copy, since handlers may add or remove bindings while we + // are dispatching. + for (const binding of bindings.slice()) { + const { ctx, handler, once } = binding; + + const context = ctx || this; - const context = ctx || this; + handler.apply(context, args as []); - handler.apply(context, args as []); + if (once) { + // Look the binding up by identity rather than by loop index, since + // indexes shift as bindings are removed. + const index = this.bindings[event]?.indexOf(binding) ?? -1; - if (once) { + if (index !== -1) { this.bindings[event]?.splice(index, 1); } - }); + } } return this; diff --git a/shepherd.js/test/unit/evented.spec.js b/shepherd.js/test/unit/evented.spec.js index 41b4c0935..9193367f2 100644 --- a/shepherd.js/test/unit/evented.spec.js +++ b/shepherd.js/test/unit/evented.spec.js @@ -38,6 +38,48 @@ describe('Evented', () => { step: { id: 'test', text: 'A step' } }); }); + + it('does not skip event bindings after removing an event binding', () => { + testEvent.once('testOn', () => true); + const handlerSpy = vi.fn(); + testEvent.on('testOn', handlerSpy); + + testEvent.trigger('testOn'); + + expect(handlerSpy).toHaveBeenCalled(); + }); + + it('calls every once handler and removes all of them', () => { + const firstSpy = vi.fn(); + const secondSpy = vi.fn(); + const thirdSpy = vi.fn(); + testEvent.once('multipleOnce', firstSpy); + testEvent.once('multipleOnce', secondSpy); + testEvent.once('multipleOnce', thirdSpy); + + testEvent.trigger('multipleOnce'); + + expect(firstSpy).toHaveBeenCalledTimes(1); + expect(secondSpy).toHaveBeenCalledTimes(1); + expect(thirdSpy).toHaveBeenCalledTimes(1); + expect( + testEvent.bindings.multipleOnce, + 'no spent once bindings left behind' + ).toHaveLength(0); + }); + + it('only calls a once handler for the first trigger', () => { + const onceSpy = vi.fn(); + const onSpy = vi.fn(); + testEvent.once('mixed', onceSpy); + testEvent.on('mixed', onSpy); + + testEvent.trigger('mixed'); + testEvent.trigger('mixed'); + + expect(onceSpy).toHaveBeenCalledTimes(1); + expect(onSpy).toHaveBeenCalledTimes(2); + }); }); describe('off()', () => { @@ -60,6 +102,23 @@ describe('Evented', () => { ).toBe(1); }); + it('removes every binding for a handler registered more than once', () => { + const handler = () => {}; + testEvent.on('testOn', handler); + testEvent.on('testOn', handler); + expect( + testEvent.bindings.testOn.length, + '3 event listeners for testOn' + ).toBe(3); + + testEvent.off('testOn', handler); + + expect( + testEvent.bindings.testOn.length, + '1 event listener for testOn' + ).toBe(1); + }); + it('does not remove uncreated events', () => { testEvent.off('testBlank'); expect( @@ -76,7 +135,7 @@ describe('Evented', () => { expect( testEvent.bindings.testOnce, 'custom event removed after one trigger' - ).toBeTruthy(); + ).toHaveLength(0); }); }); });