diff --git a/packages/dds/tree/src/simple-tree/core/index.ts b/packages/dds/tree/src/simple-tree/core/index.ts index d9724d97c13d..4197748d5b70 100644 --- a/packages/dds/tree/src/simple-tree/core/index.ts +++ b/packages/dds/tree/src/simple-tree/core/index.ts @@ -14,6 +14,7 @@ export { treeNodeFromAnchor, getSimpleNodeSchemaFromInnerNode, SimpleContextSlot, + pauseTreeEvents, } from "./treeNodeKernel.js"; export { type WithType, typeNameSymbol, typeSchemaSymbol } from "./withType.js"; export { diff --git a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts index f2895da01155..839a3b1005a6 100644 --- a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts @@ -4,8 +4,19 @@ */ import { createEmitter } from "@fluid-internal/client-utils"; -import type { Listenable, Off } from "@fluidframework/core-interfaces"; -import { assert, Lazy, fail, debugAssert } from "@fluidframework/core-utils/internal"; +import type { + HasListeners, + IEmitter, + Listenable, + Off, +} from "@fluidframework/core-interfaces/internal"; +import { + assert, + Lazy, + fail, + debugAssert, + unreachableCase, +} from "@fluidframework/core-utils/internal"; import { UsageError } from "@fluidframework/telemetry-utils/internal"; import { @@ -13,6 +24,7 @@ import { type AnchorEvents, type AnchorNode, type AnchorSet, + type FieldKey, type TreeValue, type UpPath, } from "../../core/index.js"; @@ -126,7 +138,7 @@ export class TreeNodeKernel { * This means optimizations like skipping processing data in subtrees where no subtreeChanged events are subscribed to would be able to work, * since the kernel does not unconditionally subscribe to those events (like a design which simply forwards all events would). */ - readonly #unhydratedEvents = new Lazy(createEmitter); + readonly #events = new Lazy(() => new TreeNodeEventBuffer()); /** * Create a TreeNodeKernel which can be looked up with {@link getKernel}. @@ -156,7 +168,7 @@ export class TreeNodeKernel { this.#hydrationState = { innerNode, off: innerNode.events.on("childrenChangedAfterBatch", ({ changedFields }) => { - this.#unhydratedEvents.value.emit("childrenChangedAfterBatch", { + this.#events.value.emit("childrenChangedAfterBatch", { changedFields, }); @@ -165,7 +177,7 @@ export class TreeNodeKernel { const treeNode = unhydratedNode.treeNode; if (treeNode !== undefined) { const kernel = getKernel(treeNode); - kernel.#unhydratedEvents.value.emit("subtreeChangedAfterBatch"); + kernel.#events.value.emit("subtreeChangedAfterBatch"); } const parentNode: FlexTreeNode | undefined = unhydratedNode.parentField.parent.parent; @@ -181,6 +193,15 @@ export class TreeNodeKernel { // Hydrated case this.#hydrationState = this.createHydratedState(innerNode.anchorNode); this.#hydrationState.innerNode = innerNode; + + // Forward relevant anchorNode events to our event handler + for (const eventName of kernelEvents) { + this.#hydrationState.offAnchorNode.add( + innerNode.anchorNode.events.on(eventName, (arg) => + this.#events.value.emit(eventName, arg), + ), + ); + } } } @@ -213,18 +234,11 @@ export class TreeNodeKernel { this.#hydrationState = this.createHydratedState(anchorNode); this.#hydrationState.offAnchorNode.add(() => anchors.forget(anchor)); - // If needed, register forwarding emitters for events from before hydration - if (this.#unhydratedEvents.evaluated) { - const events = this.#unhydratedEvents.value; - for (const eventName of kernelEvents) { - if (events.hasListeners(eventName)) { - this.#hydrationState.offAnchorNode.add( - // Argument is forwarded between matching events, so the type should be correct. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - anchorNode.events.on(eventName, (arg: any) => events.emit(eventName, arg)), - ); - } - } + // Forward relevant anchorNode events to our event handler + for (const eventName of kernelEvents) { + this.#hydrationState.offAnchorNode.add( + anchorNode.events.on(eventName, (arg) => this.#events.value.emit(eventName, arg)), + ); } } @@ -267,10 +281,7 @@ export class TreeNodeKernel { } public get events(): Listenable { - // Retrieve the correct events object based on whether this node is pre or post hydration. - return isHydrated(this.#hydrationState) - ? this.#hydrationState.anchorNode.events - : this.#unhydratedEvents.value; + return this.#events.value; } public dispose(): void { @@ -281,6 +292,9 @@ export class TreeNodeKernel { off(); } } + if (this.#events.evaluated) { + this.#events.value.dispose(); + } // TODO: go to the context and remove myself from withAnchors } @@ -354,6 +368,249 @@ const kernelEvents = ["childrenChangedAfterBatch", "subtreeChangedAfterBatch"] a type KernelEvents = Pick; +// #region Document this thoroughly + +/** + * Tracks the number of times {@link pauseTreeEvents} has been called without + * being matched by a corresponding resume call. + * Events will be paused while this counter is \> 0. + */ +let pauseTreeEventsStack: number = 0; + +/** + * Pause events emitted by {@link TreeNode}s. + * + * @remarks + * Events that would otherwise have been emitted are buffered until the returned function is called. + * + * Note: this should be used with caution. User application behaviors are implicitly coupled to event timing. + * Disrupting this timing can lead to unexpected behavior. + * + * It is also vitally important that the returned callback be invoked to ensure events are resumed. + * Failing to do so will result in events never being emitted again. + * + * @privateRemarks + * If we had access to `Symbol.dispose`, that would probably be a better pattern than returning a callback. + * Users could then use this API with `using` to ensure proper cleanup. + * + * @returns A function that, when called, resumes event emission and flushes any buffered events. + */ +export function pauseTreeEvents(): () => void { + pauseTreeEventsStack++; + + return () => { + pauseTreeEventsStack--; + assert(pauseTreeEventsStack >= 0, "pauseEvents count should never be negative"); + if (pauseTreeEventsStack === 0) { + flushEventsEmitter.emit("flush"); + } + }; +} + +/** + * Event emitter to notify subscribers when tree events buffered due to {@link pauseTreeEvents} should be flushed. + */ +const flushEventsEmitter = createEmitter<{ + flush: () => void; +}>(); + +/** + * Event emitter for {@link TreeNodeKernel}, which optionally buffers events based on {@link pauseTreeEvents}. + * @remarks Listens to {@link flushEventsEmitter} to know when to flush any buffered events. + */ +class TreeNodeEventBuffer + implements + Listenable, + Pick, "emit">, + HasListeners +{ + private disposed: boolean = false; + + /** + * Listen to {@link flushEventsEmitter} to know when to flush buffered events. + */ + private readonly disposeOnFlushListener = flushEventsEmitter.on("flush", () => { + this.flush(); + }); + + /** + * {@link AnchorEvents.childrenChangedAfterBatch} listeners. + */ + private readonly childrenChangedListeners: Set< + (arg: { + changedFields: ReadonlySet; + }) => void + > = new Set(); + + /** + * {@link AnchorEvents.subTreeChanged} listeners. + */ + private readonly subTreeChangedListeners: Set<() => void> = new Set(); + + /** + * Buffer of fields that have changed since events were paused. + * When events are flushed, a single {@link AnchorEvents.childrenChangedAfterBatch} event will be emitted + * containing the accumulated set of changed fields. + */ + private readonly childrenChangedBuffer: Set = new Set(); + + /** + * Whether or not the subtree has changed since events were paused. + * When events are flushed, a single {@link AnchorEvents.subTreeChanged} event will be emitted if and only + * if the subtree has changed. + */ + private subTreeChangedBuffer: boolean = false; + + public hasListeners(eventName: keyof KernelEvents): boolean { + switch (eventName) { + case "childrenChangedAfterBatch": + return this.childrenChangedListeners.size > 0; + case "subtreeChangedAfterBatch": + return this.subTreeChangedListeners.size > 0; + default: { + unreachableCase(eventName); + } + } + } + + public emit( + eventName: keyof KernelEvents, + arg?: { + changedFields: ReadonlySet; + }, + ): void { + this.assertNotDisposed(); + switch (eventName) { + case "childrenChangedAfterBatch": + assert(arg !== undefined, "childrenChangedAfterBatch should have arg"); + return this.handleChildrenChangedAfterBatch(arg.changedFields); + case "subtreeChangedAfterBatch": + return this.handleSubtreeChangedAfterBatch(); + default: + unreachableCase(eventName); + } + } + + private handleChildrenChangedAfterBatch(changedFields: ReadonlySet): void { + if (pauseTreeEventsStack) { + for (const fieldKey of changedFields) { + this.childrenChangedBuffer.add(fieldKey); + } + } else { + for (const listener of this.childrenChangedListeners) { + listener({ changedFields }); + } + } + } + + private handleSubtreeChangedAfterBatch(): void { + if (pauseTreeEventsStack) { + this.subTreeChangedBuffer = true; + } else { + for (const listener of this.subTreeChangedListeners) { + listener(); + } + } + } + + public on( + eventName: "childrenChangedAfterBatch", + listener: (arg: { + changedFields: ReadonlySet; + }) => void, + ): () => void; + public on(eventName: "subtreeChangedAfterBatch", listener: () => void): () => void; + public on( + eventName: keyof KernelEvents, + listener: (arg: { + changedFields: ReadonlySet; + }) => void | (() => void), + ): () => void { + this.assertNotDisposed(); + switch (eventName) { + case "childrenChangedAfterBatch": + this.childrenChangedListeners.add( + listener as (arg: { + changedFields: ReadonlySet; + }) => void, + ); + return () => this.off("childrenChangedAfterBatch", listener); + case "subtreeChangedAfterBatch": + this.subTreeChangedListeners.add(listener as () => void); + return () => this.off("subtreeChangedAfterBatch", listener as () => void); + default: + unreachableCase(eventName); + } + } + + public off( + eventName: "childrenChangedAfterBatch", + listener: (arg: { + changedFields: ReadonlySet; + }) => void, + ): void; + public off(eventName: "subtreeChangedAfterBatch", listener: () => void): void; + public off( + eventName: keyof KernelEvents, + listener: (arg: { + changedFields: ReadonlySet; + }) => void | (() => void), + ): void { + this.assertNotDisposed(); + switch (eventName) { + case "childrenChangedAfterBatch": + this.childrenChangedListeners.delete( + listener as (arg: { + changedFields: ReadonlySet; + }) => void, + ); + break; + case "subtreeChangedAfterBatch": + this.subTreeChangedListeners.delete(listener as () => void); + break; + default: + unreachableCase(eventName); + } + } + + /** + * Flushes any events buffered due to {@link pauseTreeEvents}. + */ + public flush(): void { + this.assertNotDisposed(); + + if (this.childrenChangedBuffer.size > 0) { + for (const listener of this.childrenChangedListeners) { + listener({ changedFields: this.childrenChangedBuffer }); + } + this.childrenChangedBuffer.clear(); + } + + if (this.subTreeChangedBuffer) { + for (const listener of this.subTreeChangedListeners) { + listener(); + } + this.subTreeChangedBuffer = false; + } + } + + private assertNotDisposed(): void { + assert(!this.disposed, "Event handler disposed."); + } + + public dispose(): void { + if (this.disposed) { + return; + } + + this.disposeOnFlushListener(); + + this.disposed = true; + } +} + +// #endregion + /** * For "cooked" nodes this is a HydratedFlexTreeNode thats a projection of forest content. * For {@link Unhydrated} nodes this is a UnhydratedFlexTreeNode. diff --git a/packages/dds/tree/src/simple-tree/index.ts b/packages/dds/tree/src/simple-tree/index.ts index 5a0987de52f3..3d945d1c5817 100644 --- a/packages/dds/tree/src/simple-tree/index.ts +++ b/packages/dds/tree/src/simple-tree/index.ts @@ -56,6 +56,7 @@ export { walkAllowedTypes, type SchemaVisitor, type SimpleNodeSchemaBase, + pauseTreeEvents, } from "./core/index.js"; export { walkFieldSchema } from "./walkFieldSchema.js"; export type { UnsafeUnknownSchema, Insertable } from "./unsafeUnknownSchema.js"; diff --git a/packages/dds/tree/src/simple-tree/node-kinds/array/arrayNode.ts b/packages/dds/tree/src/simple-tree/node-kinds/array/arrayNode.ts index fa0108483f29..f27f47d14913 100644 --- a/packages/dds/tree/src/simple-tree/node-kinds/array/arrayNode.ts +++ b/packages/dds/tree/src/simple-tree/node-kinds/array/arrayNode.ts @@ -49,6 +49,7 @@ import { type FlexContent, type TreeNodeSchemaPrivateData, convertAllowedTypes, + pauseTreeEvents, } from "../../core/index.js"; import { type FactoryContent, @@ -1080,16 +1081,26 @@ abstract class CustomArrayNodeBase ); } - if (sourceField !== destinationField || destinationGap < sourceStart) { - destinationField.editor.insert( - destinationGap, - sourceField.editor.remove(sourceStart, movedCount), - ); - } else if (destinationGap > sourceStart + movedCount) { - destinationField.editor.insert( - destinationGap - movedCount, - sourceField.editor.remove(sourceStart, movedCount), - ); + // We implement move here via subsequent `remove` and `insert`. + // This is strictly an implementation detail and should not be observable by the user. + // TODO:AB#47457: Implement proper move support for unhydrated trees. + // As a temporary mitigation, we will pause tree events until both edits have been completed. + // That way, users will only see a single change event for the array instead of 2. + const resumeAndFlushEvents = pauseTreeEvents(); + try { + if (sourceField !== destinationField || destinationGap < sourceStart) { + destinationField.editor.insert( + destinationGap, + sourceField.editor.remove(sourceStart, movedCount), + ); + } else if (destinationGap > sourceStart + movedCount) { + destinationField.editor.insert( + destinationGap - movedCount, + sourceField.editor.remove(sourceStart, movedCount), + ); + } + } finally { + resumeAndFlushEvents(); } } else { if (!sourceField.context.isHydrated()) { diff --git a/packages/dds/tree/src/tableSchema.ts b/packages/dds/tree/src/tableSchema.ts index 5ce497697f69..0ef8045c3f69 100644 --- a/packages/dds/tree/src/tableSchema.ts +++ b/packages/dds/tree/src/tableSchema.ts @@ -29,6 +29,7 @@ import { type UnannotateImplicitFieldSchema, isArrayNodeSchema, type InsertableField, + pauseTreeEvents, } from "./simple-tree/index.js"; // Future improvement TODOs: @@ -891,15 +892,20 @@ export namespace System_TableSchema { private _applyEditsInBatch(applyEdits: () => void): void { const branch = TreeAlpha.branch(this); - if (branch === undefined) { - // If this node does not have a corresponding branch, then it is unhydrated. - // I.e., it is not part of a collaborative session yet. - // Therefore, we don't need to run the edits as a transaction. - applyEdits(); - } else { - branch.runTransaction(() => { + const resumeTreeEvents = pauseTreeEvents(); + try { + if (branch === undefined) { + // If this node does not have a corresponding branch, then it is unhydrated. + // I.e., it is not part of a collaborative session yet. + // Therefore, we don't need to run the edits as a transaction. applyEdits(); - }); + } else { + branch.runTransaction(() => { + applyEdits(); + }); + } + } finally { + resumeTreeEvents(); } } diff --git a/packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts index 676266540db2..2cdf7da659da 100644 --- a/packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts @@ -1789,7 +1789,7 @@ describe("treeNodeApi", () => { }); }); - describeHydration("array node", (initializeTree, hydrated) => { + describeHydration("array node", (initializeTree) => { const sb = new SchemaFactory("array-node-tests"); class myObject extends sb.object("object", { myNumber: sb.number, @@ -1874,53 +1874,49 @@ describe("treeNodeApi", () => { assert.equal(a2DeepChanges, 1, `treeChanged should fire once.`); }); - // Extra events are fired for move operation within unhydrated array nodes. - // TODO:AB#47457: Fix and re-enable this test in unhydrated mode. - if (hydrated) { - it(`all operations on the node trigger 'nodeChanged' and 'treeChanged' the correct number of times`, () => { - const testSchema = sb.array("listRoot", sb.number); - const root = initializeTree(testSchema, []); + it(`all operations on the node trigger 'nodeChanged' and 'treeChanged' the correct number of times`, () => { + const testSchema = sb.array("listRoot", sb.number); + const root = initializeTree(testSchema, []); - let shallowChanges = 0; - let deepChanges = 0; - Tree.on(root, "treeChanged", () => { - deepChanges++; - }); - Tree.on(root, "nodeChanged", () => { - shallowChanges++; - }); - - // Insert single item - root.insertAtStart(1); - assert.equal(shallowChanges, 1); - assert.equal(deepChanges, 1); - - // Insert multiple items - root.insertAtEnd(2, 3); - assert.equal(shallowChanges, 2); - assert.equal(deepChanges, 2); - - // Move one item within the same node - root.moveToEnd(0); - assert.equal(shallowChanges, 3); - assert.equal(deepChanges, 3); - - // Move multiple items within the same node - root.moveRangeToEnd(0, 2); - assert.equal(shallowChanges, 4); - assert.equal(deepChanges, 4); - - // Remove single item - root.removeAt(0); - assert.equal(shallowChanges, 5); - assert.equal(deepChanges, 5); - - // Remove multiple items - root.removeRange(0, 2); - assert.equal(shallowChanges, 6); - assert.equal(deepChanges, 6); + let shallowChanges = 0; + let deepChanges = 0; + Tree.on(root, "treeChanged", () => { + deepChanges++; }); - } + Tree.on(root, "nodeChanged", () => { + shallowChanges++; + }); + + // Insert single item + root.insertAtStart(1); + assert.equal(shallowChanges, 1); + assert.equal(deepChanges, 1); + + // Insert multiple items + root.insertAtEnd(2, 3); + assert.equal(shallowChanges, 2); + assert.equal(deepChanges, 2); + + // Move one item within the same node + root.moveToEnd(0); + assert.equal(shallowChanges, 3); + assert.equal(deepChanges, 3); + + // Move multiple items within the same node + root.moveRangeToEnd(0, 2); + assert.equal(shallowChanges, 4); + assert.equal(deepChanges, 4); + + // Remove single item + root.removeAt(0); + assert.equal(shallowChanges, 5); + assert.equal(deepChanges, 5); + + // Remove multiple items + root.removeRange(0, 2); + assert.equal(shallowChanges, 6); + assert.equal(deepChanges, 6); + }); }); describeHydration("map node", (initializeTree) => { diff --git a/packages/dds/tree/src/test/tableSchema.spec.ts b/packages/dds/tree/src/test/tableSchema.spec.ts index 4d4c140382ba..2f2202057b3c 100644 --- a/packages/dds/tree/src/test/tableSchema.spec.ts +++ b/packages/dds/tree/src/test/tableSchema.spec.ts @@ -1299,7 +1299,7 @@ describe("TableFactory unit tests", () => { }); }); - describeHydration("Responding to changes", (initializeTree, hydrated) => { + describeHydration("Responding to changes", (initializeTree) => { it("Responding to any changes in the table", () => { const table = initializeTree(Table, Table.empty()); @@ -1319,7 +1319,11 @@ describe("TableFactory unit tests", () => { // Add a column table.insertColumns({ - columns: [{ id: "column-0", props: {} }], + columns: [ + { id: "column-0", props: {} }, + { id: "column-1", props: {} }, + { id: "column-2", props: {} }, + ], }); assert.equal(eventCount, 2); @@ -1341,48 +1345,48 @@ describe("TableFactory unit tests", () => { }) ?? fail("Cell not found"); cell.value = "Updated value!"; assert.equal(eventCount, 4); + + // Remove columns + table.removeColumns(["column-0", "column-2"]); + assert.equal(eventCount, 5); }); - // Extra events are fired for move operation within unhydrated array nodes. - // TODO:AB#47457: Fix and re-enable this test in unhydrated mode. - if (hydrated) { - it("Responding to column list changes", () => { - const table = initializeTree(Table, Table.empty()); + it("Responding to column list changes", () => { + const table = initializeTree(Table, Table.empty()); - let eventCount = 0; + let eventCount = 0; - // Bind listener to the columns list, so we know when a column is added or removed. - // The "nodeChanged" event will fire only when the specified node itself changes (i.e., its own properties change). - Tree.on(table.columns, "nodeChanged", () => { - eventCount++; - }); + // Bind listener to the columns list, so we know when a column is added or removed. + // The "nodeChanged" event will fire only when the specified node itself changes (i.e., its own properties change). + Tree.on(table.columns, "nodeChanged", () => { + eventCount++; + }); - // Add columns - table.insertColumns({ - columns: [ - { id: "column-0", props: {} }, - { id: "column-0", props: {} }, - ], - }); - assert.equal(eventCount, 1); + // Add columns + table.insertColumns({ + columns: [ + { id: "column-0", props: {} }, + { id: "column-0", props: {} }, + ], + }); + assert.equal(eventCount, 1); - // Update column props - table.columns[0].props = { label: "Column 0" }; - assert.equal(eventCount, 1); // Event should not have fired for column node changes + // Update column props + table.columns[0].props = { label: "Column 0" }; + assert.equal(eventCount, 1); // Event should not have fired for column node changes - // Insert a row - table.insertRows({ rows: [{ id: "row-0", cells: {}, props: {} }] }); - assert.equal(eventCount, 1); // Event should not have fired for row insertion + // Insert a row + table.insertRows({ rows: [{ id: "row-0", cells: {}, props: {} }] }); + assert.equal(eventCount, 1); // Event should not have fired for row insertion - // Re-order columns - table.columns.moveToEnd(0); - assert.equal(eventCount, 2); + // Re-order columns + table.columns.moveToEnd(0); + assert.equal(eventCount, 2); - // Remove column - table.removeColumns(["column-0"]); - assert.equal(eventCount, 3); - }); - } + // Remove column + table.removeColumns(["column-0"]); + assert.equal(eventCount, 3); + }); }); describeHydration("Reading values", (initializeTree) => {