diff --git a/packages/dds/tree/src/events/events.ts b/packages/dds/tree/src/events/events.ts index c2ad4b7dc910..36606c41c659 100644 --- a/packages/dds/tree/src/events/events.ts +++ b/packages/dds/tree/src/events/events.ts @@ -4,7 +4,7 @@ */ import type { IEvent } from "@fluidframework/core-interfaces"; -import { UsageError } from "@fluidframework/telemetry-utils/internal"; +import { getOrCreate } from "../util/index.js"; /** * Convert a union of types to an intersection of those types. Useful for `TransformEvents`. @@ -95,7 +95,7 @@ export interface Listenable { * @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 an error. + * Doing so will result in undefined behavior, and is not guaranteed to behave the same in future versions of this library. */ on>(eventName: K, listener: TListeners[K]): Off; } @@ -262,10 +262,6 @@ export class EventEmitter> * @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. - * @privateRemarks - * TODO: - * invoking the returned callback can error even if its only called once if the same listener was provided to two calls to "on". - * This behavior is not documented and its unclear if its a bug or not: see note on listeners. */ public on>( eventName: K, @@ -281,23 +277,7 @@ export class EventEmitter> } }; - const listeners = this.listeners.get(eventName); - if (listeners === undefined) { - const map = new Map([[off, listener]]); - this.listeners.set(eventName, map); - } else { - // If the same listener function is already registered, error. - // This policy may change in the future, but in the meantime this is a conservative choice that can accommodate multiple future eventing API/implementation options. - // For example, adding an `Listenable.off()` method in the future could be problematic if we allowed registering the same function twice (should `off(f)` deregister _both_ `f`s or just one?). - for (const l of listeners.values()) { - if (Object.is(l, listener)) { - throw new UsageError( - "The same listener may not be registered more than once for the same event", - ); - } - } - listeners.set(off, listener); - } + getOrCreate(this.listeners, eventName, () => new Map()).set(off, listener); return off; } diff --git a/packages/dds/tree/src/test/events/eventEmitter.spec.ts b/packages/dds/tree/src/test/events/eventEmitter.spec.ts index a578fe7b06d3..f2d34d020fc6 100644 --- a/packages/dds/tree/src/test/events/eventEmitter.spec.ts +++ b/packages/dds/tree/src/test/events/eventEmitter.spec.ts @@ -96,12 +96,11 @@ describe("EventEmitter", () => { assert(!closed); }); - it("correctly handles multiple event listeners", () => { + it("correctly handles multiple registrations for the same event", () => { const emitter = createEmitter(); let count: number; const listener = () => (count += 1); const off1 = emitter.on("open", listener); - assert.throws(() => emitter.on("open", listener)); // Registering the exact same function is currently forbidden. const off2 = emitter.on("open", () => listener()); count = 0; @@ -119,6 +118,30 @@ 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", () => { + const emitter = createEmitter(); + let count: number; + 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.emit("open"); + assert.strictEqual(count, 1); + + count = 0; + off2(); + emitter.emit("open"); + assert.strictEqual(count, 0); + }); + it("allows repeat deregistrations", () => { const emitter = createEmitter(); const deregister = emitter.on("open", () => {});