fix: do not skip event handlers registered after a once handler

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;

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);
     });
   });
 });