Centralise event handling

Queue starting/stopping notifications to avoid making a mess.

lib/bluetooth-device-wrapper.ts

@@ -138,8 +138,7 @@ export class BluetoothDeviceWrapper {
     public readonly device: BluetoothDevice,
     private logging: Logging = new NullLogging(),
     private dispatchTypedEvent: TypedServiceEventDispatcher,
-    // We recreate this for the same connection and need to re-setup notifications for the old events
-    private events: Record<keyof ServiceConnectionEventMap, boolean>,
+    private currentEvents: () => Array<keyof ServiceConnectionEventMap>,
     private callbacks: ConnectCallbacks,
   ) {
     device.addEventListener(
@@ -235,7 +234,7 @@ export class BluetoothDeviceWrapper {
         this.connecting = false;
       }
 
-      Object.keys(this.events).forEach((e) =>
+      this.currentEvents().forEach((e) =>
         this.startNotifications(e as TypedServiceEvent),
       );
 
@@ -382,13 +381,17 @@ export class BluetoothDeviceWrapper {
     if (serviceInfo) {
       // TODO: type cheat! why?
       const service = await this.createIfNeeded(serviceInfo as any, true);
-      service?.startNotifications(type);
+      this.queueGattOperation(
+        async () => await service?.startNotifications(type),
+      );
     }
   }
 
   async stopNotifications(type: TypedServiceEvent) {
     const serviceInfo = this.serviceInfo.find((s) => s.events.includes(type));
-    serviceInfo?.get()?.stopNotifications(type);
+    this.queueGattOperation(
+      async () => await serviceInfo?.get()?.stopNotifications(type),
+    );
   }
 
   private disposeServices() {
@@ -401,7 +404,7 @@ export const createBluetoothDeviceWrapper = async (
   device: BluetoothDevice,
   logging: Logging,
   dispatchTypedEvent: TypedServiceEventDispatcher,
-  addedServiceListeners: Record<keyof ServiceConnectionEventMap, boolean>,
+  currentEvents: () => Array<keyof ServiceConnectionEventMap>,
   callbacks: ConnectCallbacks,
 ): Promise<BluetoothDeviceWrapper | undefined> => {
   try {
@@ -413,7 +416,7 @@ export const createBluetoothDeviceWrapper = async (
         device,
         logging,
         dispatchTypedEvent,
-        addedServiceListeners,
+        currentEvents,
         callbacks,
       );
     deviceIdToWrapper.set(device.id, bluetooth);

lib/bluetooth.ts

@@ -20,7 +20,10 @@ import {
 } from "./device.js";
 import { TypedEventTarget } from "./events.js";
 import { Logging, NullLogging } from "./logging.js";
-import { ServiceConnectionEventMap } from "./service-events.js";
+import {
+  ServiceConnectionEventMap,
+  TypedServiceEvent,
+} from "./service-events.js";
 
 const requestDeviceTimeoutDuration: number = 30000;
 
@@ -46,15 +49,6 @@ export class MicrobitWebBluetoothConnection
   private logging: Logging;
   connection: BluetoothDeviceWrapper | undefined;
 
-  private _addEventListener = this.addEventListener;
-  private _removeEventListener = this.removeEventListener;
-
-  private activeEvents = {
-    accelerometerdatachanged: false,
-    buttonachanged: false,
-    buttonbchanged: false,
-  };
-
   private availabilityListener = (e: Event) => {
     // TODO: is this called? is `value` correct?
     const value = (e as any).value as boolean;
@@ -66,14 +60,14 @@ export class MicrobitWebBluetoothConnection
   constructor(options: MicrobitWebBluetoothConnectionOptions = {}) {
     super();
     this.logging = options.logging || new NullLogging();
-    this.addEventListener = (type, ...args) => {
-      this._addEventListener(type, ...args);
-      this.connection?.startNotifications(type);
-    };
-    this.removeEventListener = (type, ...args) => {
-      this.connection?.stopNotifications(type);
-      this._removeEventListener(type, ...args);
-    };
+  }
+
+  protected eventActivated(type: string): void {
+    this.connection?.startNotifications(type as TypedServiceEvent);
+  }
+
+  protected eventDeactivated(type: string): void {
+    this.connection?.stopNotifications(type as TypedServiceEvent);
   }
 
   private log(v: any) {
@@ -161,7 +155,7 @@ export class MicrobitWebBluetoothConnection
         device,
         this.logging,
         this.dispatchTypedEvent.bind(this),
-        this.activeEvents,
+        () => this.getActiveEvents() as Array<keyof ServiceConnectionEventMap>,
         {
           onConnecting: () => this.setStatus(ConnectionStatus.CONNECTING),
           onReconnecting: () => this.setStatus(ConnectionStatus.RECONNECTING),

lib/events.test.ts

@@ -0,0 +1,114 @@
+import { describe, expect, it, vi } from "vitest";
+import { TrackingEventTarget } from "./events.js";
+
+class TestTrackingEventTarget extends TrackingEventTarget {
+  constructor(
+    private activate: (type: string) => void,
+    private deactivate: (type: string) => void,
+  ) {
+    super();
+  }
+  public getActiveEvents(): string[] {
+    return super.getActiveEvents();
+  }
+  protected eventActivated(type: string): void {
+    this.activate(type);
+  }
+  protected eventDeactivated(type: string): void {
+    this.deactivate(type);
+  }
+}
+
+describe("TrackingEventTarget", () => {
+  const listener = () => {};
+
+  it("add remove", () => {
+    const activate = vi.fn();
+    const deactivate = vi.fn();
+    const target = new TestTrackingEventTarget(activate, deactivate);
+    expect(target.getActiveEvents()).toEqual([]);
+
+    target.addEventListener("foo", listener);
+    expect(activate).toBeCalledTimes(1);
+    expect(deactivate).toBeCalledTimes(0);
+    expect(target.getActiveEvents()).toEqual(["foo"]);
+
+    target.removeEventListener("foo", listener);
+    expect(activate).toBeCalledTimes(1);
+    expect(deactivate).toBeCalledTimes(1);
+    expect(target.getActiveEvents()).toEqual([]);
+  });
+
+  it("callback equality", () => {
+    const listenerAlt = () => {};
+
+    const activate = vi.fn();
+    const deactivate = vi.fn();
+    const target = new TestTrackingEventTarget(activate, deactivate);
+    expect(target.getActiveEvents()).toEqual([]);
+
+    target.addEventListener("foo", listenerAlt);
+    target.addEventListener("foo", listener);
+    target.addEventListener("foo", listener);
+    target.removeEventListener("foo", listener);
+    expect(target.getActiveEvents()).toEqual(["foo"]);
+    target.removeEventListener("foo", listenerAlt);
+    expect(target.getActiveEvents()).toEqual([]);
+  });
+
+  it("option equality - capture", () => {
+    const fooListener = vi.fn();
+    const activate = vi.fn();
+    const deactivate = vi.fn();
+    const target = new TestTrackingEventTarget(activate, deactivate);
+    expect(target.getActiveEvents()).toEqual([]);
+
+    target.addEventListener("foo", fooListener, { capture: true });
+    target.addEventListener("foo", fooListener, false);
+    target.removeEventListener("foo", fooListener, true);
+    expect(target.getActiveEvents()).toEqual(["foo"]);
+    target.dispatchEvent(new Event("foo"));
+    expect(fooListener).toBeCalledTimes(1);
+  });
+
+  it("option equality", () => {
+    const fooListener = vi.fn();
+    const activate = vi.fn();
+    const deactivate = vi.fn();
+    const target = new TestTrackingEventTarget(activate, deactivate);
+
+    // Despite MDN docs claiming all options can result in another listener added
+    // it seems only capture counts for both add and remove
+    target.addEventListener("foo", fooListener, { passive: true });
+    target.addEventListener("foo", fooListener, { once: true });
+    target.addEventListener("foo", fooListener, { capture: true });
+    target.addEventListener("foo", fooListener, { capture: false });
+    target.dispatchEvent(new Event("foo"));
+    expect(fooListener).toBeCalledTimes(2);
+
+    target.removeEventListener("foo", fooListener, true);
+    expect(target.getActiveEvents()).toEqual(["foo"]);
+    target.dispatchEvent(new Event("foo"));
+    expect(fooListener).toBeCalledTimes(3);
+
+    target.removeEventListener("foo", fooListener, false);
+    expect(target.getActiveEvents()).toEqual([]);
+    target.dispatchEvent(new Event("foo"));
+    expect(fooListener).toBeCalledTimes(3);
+  });
+
+  it("once", () => {
+    const fooListener = vi.fn();
+    const activate = vi.fn();
+    const deactivate = vi.fn();
+    const target = new TestTrackingEventTarget(activate, deactivate);
+
+    target.addEventListener("foo", fooListener, { once: true });
+    target.dispatchEvent(new Event("foo"));
+    expect(fooListener).toBeCalledTimes(1);
+    expect(deactivate).toBeCalledTimes(1);
+
+    target.dispatchEvent(new Event("foo"));
+    expect(fooListener).toBeCalledTimes(1);
+  });
+});

lib/events.ts

@@ -109,8 +109,78 @@ export interface TypedEventTarget<M extends ValueIsEvent<M>> {
    */
   dispatchEvent: (event: Event) => boolean;
 }
+
+// We've added this in to keep track of what events are active.
+// Having done this it's questionable whether it's worth the reimplementation
+// just to use an EventTarget API.
+export class TrackingEventTarget extends EventTarget {
+  private activeEventTracking: Map<string, Registration[]> = new Map();
+
+  addEventListener(
+    type: string,
+    callback: EventListenerOrEventListenerObject | null,
+    options?: AddEventListenerOptions | boolean,
+  ): void {
+    if (callback !== null) {
+      const registrations = this.activeEventTracking.get(type) ?? [];
+      const wasEmpty = registrations.length === 0;
+      const registration = new Registration(callback, options ?? false);
+      if (!registrations.find((r) => r.eq(registration))) {
+        registrations.push(registration);
+        this.activeEventTracking.set(type, registrations);
+        if (wasEmpty) {
+          this.eventActivated(type);
+        }
+      }
+    }
+    super.addEventListener(type, callback, options);
+  }
+
+  removeEventListener(
+    type: string,
+    callback: EventListenerOrEventListenerObject | null,
+    options?: EventListenerOptions | boolean,
+  ): void {
+    if (callback !== null) {
+      const registration = new Registration(callback, options ?? false);
+      this.filterRegistrations(type, (r) => !r.eq(registration));
+    }
+    super.removeEventListener(type, callback, options);
+  }
+
+  dispatchEvent(event: Event): boolean {
+    const result = super.dispatchEvent(event);
+    this.filterRegistrations(event.type, (r) => !r.isOnce());
+    return result;
+  }
+
+  private filterRegistrations(
+    type: string,
+    predicate: (r: Registration) => boolean,
+  ): void {
+    let registrations = this.activeEventTracking.get(type) ?? [];
+    registrations = registrations.filter(predicate);
+    if (registrations.length === 0) {
+      this.activeEventTracking.delete(type);
+      this.eventDeactivated(type);
+    } else {
+      this.activeEventTracking.set(type, registrations);
+    }
+  }
+
+  protected eventActivated(type: string) {}
+
+  protected eventDeactivated(type: string) {}
+
+  protected getActiveEvents(): string[] {
+    return [...this.activeEventTracking.keys()];
+  }
+}
+
 // eslint-disable-next-line @typescript-eslint/no-unsafe-declaration-merging
-export class TypedEventTarget<M extends ValueIsEvent<M>> extends EventTarget {
+export class TypedEventTarget<
+  M extends ValueIsEvent<M>,
+> extends TrackingEventTarget {
   /**
    * Dispatches a synthetic event event to target and returns true if either
    * event's cancelable attribute value is false or its preventDefault() method
@@ -120,3 +190,31 @@ export class TypedEventTarget<M extends ValueIsEvent<M>> extends EventTarget {
     return super.dispatchEvent(event);
   }
 }
+
+class Registration {
+  constructor(
+    private callback: EventListenerOrEventListenerObject,
+    private options: AddEventListenerOptions | boolean,
+  ) {}
+
+  isOnce() {
+    return typeof this.options === "object" && this.options.once === true;
+  }
+
+  eq(other: Registration) {
+    return (
+      other.callback === this.callback &&
+      eqUseCapture(this.options, other.options)
+    );
+  }
+}
+
+const eqUseCapture = (
+  left: AddEventListenerOptions | boolean,
+  right: AddEventListenerOptions | boolean,
+) => {
+  const leftValue = typeof left === "boolean" ? left : left.capture ?? false;
+  const rightValue =
+    typeof right === "boolean" ? right : right.capture ?? false;
+  return leftValue === rightValue;
+};

lib/usb.ts

@@ -130,9 +130,6 @@ export class MicrobitWebUSBConnection
 
   private logging: Logging;
 
-  private _addEventListener = this.addEventListener;
-  private _removeEventListener = this.removeEventListener;
-
   private addedListeners: Record<string, number> = {
     serialdata: 0,
   };
@@ -142,20 +139,6 @@ export class MicrobitWebUSBConnection
   ) {
     super();
     this.logging = options.logging;
-    // TODO: this doesn't account for the rules around add/remove call equivalence
-    this.addEventListener = (type, ...args) => {
-      this._addEventListener(type, ...args);
-      if (++this.addedListeners[type] === 1 && !this.flashing) {
-        this.startNotifications(type);
-      }
-    };
-    this.removeEventListener = (type, ...args) => {
-      this._removeEventListener(type, ...args);
-      if (--this.addedListeners[type] <= 0) {
-        this.addedListeners[type] = 0;
-        this.stopNotifications(type);
-      }
-    };
   }
 
   private log(v: any) {
@@ -442,7 +425,7 @@ export class MicrobitWebUSBConnection
     return this.device;
   }
 
-  private async startNotifications(type: string) {
+  protected eventActivated(type: string): void {
     switch (type as keyof DeviceConnectionEventMap) {
       case "serialdata": {
         this.startSerialInternal();
@@ -451,7 +434,7 @@ export class MicrobitWebUSBConnection
     }
   }
 
-  private async stopNotifications(type: string) {
+  protected async eventDeactivated(type: string) {
     switch (type as keyof DeviceConnectionEventMap) {
       case "serialdata": {
         this.stopSerialInternal();