diff --git a/lib/bluetooth-device-wrapper.ts b/lib/bluetooth-device-wrapper.ts index 1c2697b..5754459 100644 --- a/lib/bluetooth-device-wrapper.ts +++ b/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, + private currentEvents: () => Array, 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, + currentEvents: () => Array, callbacks: ConnectCallbacks, ): Promise => { try { @@ -413,7 +416,7 @@ export const createBluetoothDeviceWrapper = async ( device, logging, dispatchTypedEvent, - addedServiceListeners, + currentEvents, callbacks, ); deviceIdToWrapper.set(device.id, bluetooth); diff --git a/lib/bluetooth.ts b/lib/bluetooth.ts index 625b7ee..a353d84 100644 --- a/lib/bluetooth.ts +++ b/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, { onConnecting: () => this.setStatus(ConnectionStatus.CONNECTING), onReconnecting: () => this.setStatus(ConnectionStatus.RECONNECTING), diff --git a/lib/events.test.ts b/lib/events.test.ts new file mode 100644 index 0000000..7b2acee --- /dev/null +++ b/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); + }); +}); diff --git a/lib/events.ts b/lib/events.ts index c5ca294..04d7354 100644 --- a/lib/events.ts +++ b/lib/events.ts @@ -109,8 +109,78 @@ export interface TypedEventTarget> { */ 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 = 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> extends EventTarget { +export class TypedEventTarget< + M extends ValueIsEvent, +> 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> 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; +}; diff --git a/lib/usb.ts b/lib/usb.ts index fc7ba18..f7c192f 100644 --- a/lib/usb.ts +++ b/lib/usb.ts @@ -130,9 +130,6 @@ export class MicrobitWebUSBConnection private logging: Logging; - private _addEventListener = this.addEventListener; - private _removeEventListener = this.removeEventListener; - private addedListeners: Record = { 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();