From c59225db033a516ee20e459ae31567d97ce8776c Mon Sep 17 00:00:00 2001 From: Noah Encke <78610362+noencke@users.noreply.github.com> Date: Mon, 11 Nov 2024 17:03:27 -0800 Subject: [PATCH] Add `off()` method to Listenable event interface (#23046) ## Description This exposes the `off()` method on the `Listenable` event interface, providing an additional way to unsubscribe from events. The current way of unsubscribing, via the deregistration function returned by `on()`, is still supported. Having both options available is meant to give customers options without being opinionated about the best pattern, as it depends on preference and use case. To prevent the two strategies from interfering with each other, it has also been made illegal to register the same listener more than once for the same event. It is now documented as throwing an error rather than as undefined behavior. This also adds a test for registering events with keys that are JS symbols and does some minor doc cleanup. --- .changeset/lovely-taxis-notice.md | 27 +++ .../dds/tree/api-report/tree.alpha.api.md | 1 + packages/dds/tree/api-report/tree.beta.api.md | 1 + .../tree/api-report/tree.legacy.alpha.api.md | 1 + .../tree/api-report/tree.legacy.public.api.md | 1 + .../dds/tree/api-report/tree.public.api.md | 1 + packages/dds/tree/src/events/emitter.ts | 55 +++--- packages/dds/tree/src/events/listeners.ts | 22 ++- .../dds/tree/src/test/events/events.spec.ts | 167 ++++++++++++++---- .../api-report/fluid-framework.alpha.api.md | 1 + .../api-report/fluid-framework.beta.api.md | 1 + .../fluid-framework.legacy.alpha.api.md | 1 + .../fluid-framework.legacy.public.api.md | 1 + .../api-report/fluid-framework.public.api.md | 1 + 14 files changed, 218 insertions(+), 63 deletions(-) create mode 100644 .changeset/lovely-taxis-notice.md diff --git a/.changeset/lovely-taxis-notice.md b/.changeset/lovely-taxis-notice.md new file mode 100644 index 000000000000..7b382d050175 --- /dev/null +++ b/.changeset/lovely-taxis-notice.md @@ -0,0 +1,27 @@ +--- +"fluid-framework": minor +"@fluidframework/tree": minor +--- +--- +"section": tree +--- + +SharedTree event listeners that implement `Listenable` now allow deregistration of event listeners via an `off()` function. + +The ability to deregister events via a callback returned by `on()` remains the same. +Both strategies will remain supported and consumers of SharedTree events may choose which method of deregistration they prefer in a given instance. + +```typescript +// The new behavior +function deregisterViaOff(view: TreeView): { + const listener = () => { /* ... */ }; + view.events.on("commitApplied", listener); // Register + view.events.off("commitApplied", listener); // Deregister +} + +// The existing behavior (still supported) +function deregisterViaCallback(view: TreeView): { + const off = view.events.on("commitApplied", () => { /* ... */ }); // Register + off(); // Deregister +} +``` diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index 3265e4a9b9aa..ef5219ac56b9 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -417,6 +417,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md index 7c570d89640b..30cd48fe2a2c 100644 --- a/packages/dds/tree/api-report/tree.beta.api.md +++ b/packages/dds/tree/api-report/tree.beta.api.md @@ -225,6 +225,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/dds/tree/api-report/tree.legacy.alpha.api.md b/packages/dds/tree/api-report/tree.legacy.alpha.api.md index 4df875307225..7ace7471ef4e 100644 --- a/packages/dds/tree/api-report/tree.legacy.alpha.api.md +++ b/packages/dds/tree/api-report/tree.legacy.alpha.api.md @@ -225,6 +225,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/dds/tree/api-report/tree.legacy.public.api.md b/packages/dds/tree/api-report/tree.legacy.public.api.md index b2d24dd09fc8..13ca282bad87 100644 --- a/packages/dds/tree/api-report/tree.legacy.public.api.md +++ b/packages/dds/tree/api-report/tree.legacy.public.api.md @@ -225,6 +225,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md index b2d24dd09fc8..13ca282bad87 100644 --- a/packages/dds/tree/api-report/tree.public.api.md +++ b/packages/dds/tree/api-report/tree.public.api.md @@ -225,6 +225,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/dds/tree/src/events/emitter.ts b/packages/dds/tree/src/events/emitter.ts index 73bbbdddbfcc..f352b374a79d 100644 --- a/packages/dds/tree/src/events/emitter.ts +++ b/packages/dds/tree/src/events/emitter.ts @@ -3,7 +3,8 @@ * Licensed under the MIT License. */ -import { setInNestedMap } from "../util/index.js"; +import { UsageError } from "@fluidframework/telemetry-utils/internal"; +import { getOrCreate } from "../util/index.js"; import type { Listenable, Listeners, Off } from "./listeners.js"; /** @@ -103,7 +104,7 @@ export interface HasListeners> { * ```typescript * class MyExposingClass { * private readonly _events = createEmitter(); - * public readonly events: Listenable = this._events; + * public readonly events: Listenable = this._events; * * private load() { * this._events.emit("loaded"); @@ -117,7 +118,7 @@ export class EventEmitter> { protected readonly listeners = new Map< keyof TListeners, - Map TListeners[keyof TListeners]> + Set<(...args: any[]) => TListeners[keyof TListeners]> >(); // Because this is protected and not public, calling this externally (not from a subclass) makes sending events to the constructed instance impossible. @@ -132,11 +133,10 @@ export class EventEmitter> if (listeners !== undefined) { // Current tsc (5.4.5) cannot spread `args` into `listener()`. const argArray: unknown[] = args; - // This explicitly copies listeners so that new listeners added during this call to emit will not receive this event. - for (const [off, listener] of [...listeners]) { + for (const listener of [...listeners]) { // If listener has been unsubscribed while invoking other listeners, skip it. - if (listeners.has(off)) { + if (listeners.has(listener)) { listener(...argArray); } } @@ -159,29 +159,32 @@ export class EventEmitter> return []; } - /** - * Register an event listener. - * @param eventName - the name of the event - * @param listener - the handler to run when the event is fired by the emitter - * @returns a function which will deregister the listener when run. - * This function will error if called more than once. - */ public on>( eventName: K, listener: TListeners[K], ): Off { - const off: Off = () => { - const currentListeners = this.listeners.get(eventName); - if (currentListeners?.delete(off) === true) { - if (currentListeners.size === 0) { - this.listeners.delete(eventName); - this.noListeners?.(eventName); - } - } - }; + const listeners = getOrCreate(this.listeners, eventName, () => new Set()); + if (listeners.has(listener)) { + const eventDescription = + typeof eventName === "symbol" ? eventName.description : String(eventName.toString()); - setInNestedMap(this.listeners, eventName, off, listener); - return off; + throw new UsageError( + `Attempted to register the same listener object twice for event ${eventDescription}`, + ); + } + listeners.add(listener); + return () => this.off(eventName, listener); + } + + public off>( + eventName: K, + listener: TListeners[K], + ): void { + const listeners = this.listeners.get(eventName); + if (listeners?.delete(listener) === true && listeners.size === 0) { + this.listeners.delete(eventName); + this.noListeners?.(eventName); + } } public hasListeners(eventName?: keyof TListeners): boolean { @@ -239,6 +242,10 @@ class ComposableEventEmitter> * public on(eventName: K, listener: MyEvents[K]): Off { * return this.events.on(eventName, listener); * } + * + * public off(eventName: K, listener: MyEvents[K]): void { + * return this.events.off(eventName, listener); + * } * } * ``` */ diff --git a/packages/dds/tree/src/events/listeners.ts b/packages/dds/tree/src/events/listeners.ts index 06745b1eccda..d87d6d3b6060 100644 --- a/packages/dds/tree/src/events/listeners.ts +++ b/packages/dds/tree/src/events/listeners.ts @@ -50,13 +50,25 @@ export interface Listenable { /** * Register an event listener. * @param eventName - The name of the event. - * @param listener - the handler to run when the event is fired by the emitter - * @returns a {@link Off | function} which will deregister the listener when called. - * This deregistration function is idempotent and therefore may be safely called more than once with no effect. - * @remarks Do not register the exact same `listener` object for the same event more than once. - * Doing so will result in undefined behavior, and is not guaranteed to behave the same in future versions of this library. + * @param listener - The listener function to run when the event is fired. + * @returns A {@link Off | function} which will deregister the listener when called. + * Calling the deregistration function more than once will have no effect. + * + * Listeners may also be deregistered by passing the listener to {@link Listenable.off | off()}. + * @remarks Registering the exact same `listener` object for the same event more than once will throw an error. + * If registering the same listener for the same event multiple times is desired, consider using a wrapper function for the second subscription. */ on>(eventName: K, listener: TListeners[K]): Off; + + /** + * Deregister an event listener. + * @param eventName - The name of the event. + * @param listener - The listener function to remove from the current set of event listeners. + * @remarks If `listener` is not currently registered, this method will have no effect. + * + * Listeners may also be deregistered by calling the {@link Off | deregistration function} returned when they are {@link Listenable.on | registered}. + */ + off>(eventName: K, listener: TListeners[K]): void; } /** diff --git a/packages/dds/tree/src/test/events/events.spec.ts b/packages/dds/tree/src/test/events/events.spec.ts index e92a45600fe4..ca1b31bc3a08 100644 --- a/packages/dds/tree/src/test/events/events.spec.ts +++ b/packages/dds/tree/src/test/events/events.spec.ts @@ -5,6 +5,8 @@ import { strict as assert } from "node:assert"; +import { validateAssertionError } from "@fluidframework/test-runtime-utils/internal"; + import { EventEmitter, createEmitter, @@ -71,27 +73,31 @@ describe("EventEmitter", () => { assert(closed); }); - it("deregisters events", () => { + it("deregisters events via callback", () => { const emitter = createEmitter(); let error = false; - const deregister = emitter.on("close", (e: boolean) => { - error = e; - }); + const deregister = emitter.on("close", (e: boolean) => (error = e)); deregister(); emitter.emit("close", true); assert.strictEqual(error, false); }); - it("deregisters multiple events", () => { + it("deregisters events via off", () => { + const emitter = createEmitter(); + let error = false; + const listener = (e: boolean) => (error = e); + emitter.on("close", listener); + emitter.off("close", listener); + emitter.emit("close", true); + assert.strictEqual(error, false); + }); + + it("deregisters multiple events via callback", () => { const emitter = createEmitter(); let opened = false; let closed = false; - const deregisterOpen = emitter.on("open", () => { - opened = true; - }); - const deregisterClosed = emitter.on("close", () => { - closed = true; - }); + const deregisterOpen = emitter.on("open", () => (opened = true)); + const deregisterClosed = emitter.on("close", () => (closed = true)); deregisterOpen(); deregisterClosed(); emitter.emit("open"); @@ -102,6 +108,24 @@ describe("EventEmitter", () => { assert(!closed); }); + it("deregisters multiple events via off", () => { + const emitter = createEmitter(); + let opened = false; + let closed = false; + const listenerOpen = () => (opened = true); + const listenerClosed = () => (closed = true); + emitter.on("open", listenerOpen); + emitter.on("close", listenerClosed); + emitter.off("open", listenerOpen); + emitter.off("close", listenerClosed); + emitter.emit("open"); + assert(!opened); + assert(!closed); + emitter.emit("close", false); + assert(!opened); + assert(!closed); + }); + it("correctly handles multiple registrations for the same event", () => { const emitter = createEmitter(); let count: number; @@ -124,56 +148,127 @@ describe("EventEmitter", () => { assert.strictEqual(count, 0); }); - // Note: This behavior is not contractually required (see docs for `Listenable.on()`), - // but is tested here to check for changes or regressions. - it("correctly handles multiple registrations of the same listener", () => { + it("errors on multiple registrations of the same listener", () => { const emitter = createEmitter(); - let count: number; + let count = 0; const listener = () => (count += 1); - const off1 = emitter.on("open", listener); - const off2 = emitter.on("open", listener); - - count = 0; - emitter.emit("open"); - assert.strictEqual(count, 2); // Listener should be fired twice - - count = 0; - off1(); + emitter.on("open", listener); + assert.throws( + () => emitter.on("open", listener), + (e: Error) => validateAssertionError(e, /register.*twice.*open/), + ); + // If error is caught, the listener should still fire once for the first registration emitter.emit("open"); assert.strictEqual(count, 1); + }); - count = 0; - off2(); - emitter.emit("open"); - assert.strictEqual(count, 0); + it("includes symbol description in the error message on multiple registrations of the same listener", () => { + // This test ensures that symbol types are registered, error on double registration, and include the description of the symbol in the error message. + const eventSymbol = Symbol("TestEvent"); + const emitter = createEmitter<{ [eventSymbol]: () => void }>(); + const listener = () => {}; + emitter.on(eventSymbol, listener); + emitter.emit(eventSymbol); + assert.throws( + () => emitter.on(eventSymbol, listener), + (e: Error) => validateAssertionError(e, /register.*twice.*TestEvent/), + ); }); it("allows repeat deregistrations", () => { const emitter = createEmitter(); const deregister = emitter.on("open", () => {}); - const deregisterB = emitter.on("open", () => {}); + const listenerB = () => {}; + emitter.on("open", listenerB); deregister(); deregister(); - deregisterB(); - deregisterB(); + emitter.off("open", listenerB); + emitter.off("open", listenerB); }); - it("skips events adding during event", () => { + it("skips events added during event", () => { const emitter = createEmitter(); const log: string[] = []; - const unsubscribe = emitter.on("open", () => { + const off = emitter.on("open", () => { log.push("A"); emitter.on("open", () => { log.push("B"); }); }); emitter.emit("open"); - unsubscribe(); + off(); assert.deepEqual(log, ["A"]); emitter.emit("open"); assert.deepEqual(log, ["A", "B"]); }); + it("skips events removed during event", () => { + function test(remove: boolean, expected: string[]): void { + const log: string[] = []; + const emitter = createEmitter(); + emitter.on("open", () => { + log.push("A"); + if (remove) { + offB(); + } + }); + const offB = emitter.on("open", () => { + log.push("B"); + }); + emitter.emit("open"); + assert.deepEqual(log, expected); + } + + // Because event ordering is not guaranteed, we first test the control case to ensure that the second event fires after the first... + test(false, ["A", "B"]); + // ... and then test the same scenario but with the second event removed before it can fire. + test(true, ["A"]); + // If event ordering ever changes, this test will need to be updated to account for that. + }); + + it("fires the noListeners callback when the last listener is removed", () => { + let noListenersFired = false; + const emitter = createEmitter(() => (noListenersFired = true)); + const offA = emitter.on("open", () => {}); + const offB = emitter.on("open", () => {}); + assert.equal(noListenersFired, false); + offA(); + assert.equal(noListenersFired, false); + offB(); + assert.equal(noListenersFired, true); + }); + + it("reports whether or not it has listeners", () => { + const emitter = createEmitter(); + assert.equal(emitter.hasListeners(), false); + const offA = emitter.on("open", () => {}); + assert.equal(emitter.hasListeners(), true); + const offB = emitter.on("open", () => {}); + assert.equal(emitter.hasListeners(), true); + offB(); + assert.equal(emitter.hasListeners(), true); + offA(); + assert.equal(emitter.hasListeners(), false); + }); + + it("reports whether or not it has listeners for a given event", () => { + const emitter = createEmitter(); + assert.equal(emitter.hasListeners("open"), false); + assert.equal(emitter.hasListeners("close"), false); + const offA = emitter.on("open", () => {}); + assert.equal(emitter.hasListeners("open"), true); + assert.equal(emitter.hasListeners("close"), false); + const offB = emitter.on("close", () => {}); + assert.equal(emitter.hasListeners("open"), true); + assert.equal(emitter.hasListeners("close"), true); + offA(); + assert.equal(emitter.hasListeners("open"), false); + assert.equal(emitter.hasListeners("close"), true); + offB(); + assert.equal(emitter.hasListeners("open"), false); + assert.equal(emitter.hasListeners("close"), false); + }); + it("reentrant events", () => { const emitter = createEmitter(); const log: string[] = []; @@ -216,6 +311,10 @@ class MyCompositionClass implements Listenable { public on(eventName: K, listener: MyEvents[K]): () => void { return this.events.on(eventName, listener); } + + public off(eventName: K, listener: MyEvents[K]): void { + return this.events.off(eventName, listener); + } } class MyExposingClass { diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md index 37eac2a1a112..ff7d9fddd292 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md @@ -757,6 +757,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md index 267375e03fa0..8c46317e8238 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md @@ -562,6 +562,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md index cb71efc1816b..9738a1e306d2 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md @@ -864,6 +864,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md index 8b4ca68b0fba..6a9645003fdd 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md @@ -598,6 +598,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; } diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.public.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.public.api.md index 8eedb915642f..dea2872cb5e4 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.public.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.public.api.md @@ -562,6 +562,7 @@ export type LazyItem = Item | (() => Item); // @public @sealed export interface Listenable { + off>(eventName: K, listener: TListeners[K]): void; on>(eventName: K, listener: TListeners[K]): Off; }