Centralise event handling

Try to account for options etc. - needs tests for the detail which I'll
add in a follow up PR.

Queue starting/stopping notifications to avoid making a mess.

Author: microbit-matt-hillsdon

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.ts

@@ -109,14 +109,158 @@ 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.
+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.eqForAdd(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) {
+      let registrations = this.activeEventTracking.get(type) ?? [];
+      const registration = new Registration(callback, options ?? false);
+      registrations = registrations.filter((r) => !r.eqForRemove(registration));
+      this.activeEventTracking.set(type, registrations);
+      if (registrations.length == 0) {
+        this.eventDeactivated(type);
+      }
+    }
+  }
+
+  dispatchEvent(event: Event): boolean {
+    const result = super.dispatchEvent(event);
+    let registrations = this.activeEventTracking.get(event.type) ?? [];
+    registrations = registrations.filter((r) => !r.isOnce());
+    this.activeEventTracking.set(event.type, registrations);
+    if (registrations.length === 0) {
+      this.eventDeactivated(event.type);
+    }
+    return result;
+  }
+
+  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
    * was not invoked, and false otherwise.
    */
   public dispatchTypedEvent<T extends keyof M>(_type: T, event: M[T]): boolean {
+    // Remove any "once" listeners for the event
     return super.dispatchEvent(event);
   }
 }
+
+class Registration {
+  constructor(
+    private callback: EventListenerOrEventListenerObject,
+    private options: AddEventListenerOptions | boolean,
+  ) {}
+
+  isOnce() {
+    return typeof this.options === "object" && this.options.once === true;
+  }
+
+  eqForAdd(other: Registration) {
+    return (
+      other.callback === this.callback &&
+      eqOptionsForAdd(this.options, other.options)
+    );
+  }
+
+  eqForRemove(other: Registration) {
+    return (
+      other.callback === this.callback &&
+      eqOptionsForRemove(this.options, other.options)
+    );
+  }
+}
+
+const eqOptionsForAdd = (
+  left: AddEventListenerOptions | boolean,
+  right: AddEventListenerOptions | boolean,
+) => {
+  if (!eqUseCapture(left, right)) {
+    return false;
+  }
+  if (!eqOptionValue(left, right, "passive")) {
+    return false;
+  }
+  if (!eqOptionValue(left, right, "signal")) {
+    return false;
+  }
+  if (!eqOptionValue(left, right, "once")) {
+    return false;
+  }
+  return true;
+};
+
+const eqOptionsForRemove = (
+  left: AddEventListenerOptions | boolean,
+  right: AddEventListenerOptions | boolean,
+) => {
+  return eqUseCapture(left, right);
+};
+
+const eqUseCapture = (
+  left: AddEventListenerOptions | boolean,
+  right: AddEventListenerOptions | boolean,
+) => {
+  return eqOptionValue(
+    left,
+    right,
+    "capture",
+    left as boolean,
+    right as boolean,
+  );
+};
+
+const eqOptionValue = (
+  left: AddEventListenerOptions | boolean,
+  right: AddEventListenerOptions | boolean,
+  key: "once" | "capture" | "passive" | "signal",
+  valueIfBoolLeft: boolean = false,
+  valueIfBoolRight: boolean = false,
+) => {
+  const leftValue =
+    typeof left === "boolean" ? valueIfBoolLeft : left[key] ?? false;
+  const rightValue =
+    typeof right === "boolean" ? valueIfBoolRight : right[key] ?? 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();