diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index 7055d6984314..093f36d6555b 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -768,6 +768,7 @@ export interface RunTransaction { // @alpha @input export interface RunTransactionParams { + readonly deferTreeEvents?: boolean; readonly preconditions?: readonly TransactionConstraint[]; } diff --git a/packages/dds/tree/src/shared-tree/schematizingTreeView.ts b/packages/dds/tree/src/shared-tree/schematizingTreeView.ts index 45c92667a281..8c8d67ecdcb9 100644 --- a/packages/dds/tree/src/shared-tree/schematizingTreeView.ts +++ b/packages/dds/tree/src/shared-tree/schematizingTreeView.ts @@ -66,6 +66,7 @@ import { import { canInitialize, initialize, initializerFromChunk } from "./schematizeTree.js"; import type { ITreeCheckout, TreeCheckout } from "./treeCheckout.js"; +import { pauseTreeEvents } from "../simple-tree/index.js"; /** * Creating multiple tree views from the same checkout is not supported. This slot is used to detect if one already @@ -229,16 +230,10 @@ export class SchematizingSimpleTreeView< return this.flexTreeContext; } - /** - * {@inheritDoc @fluidframework/shared-tree#TreeViewAlpha.runTransaction} - */ public runTransaction( transaction: () => TransactionCallbackStatus, params?: RunTransactionParams, ): TransactionResultExt; - /** - * {@inheritDoc @fluidframework/shared-tree#TreeViewAlpha.runTransaction} - */ public runTransaction( transaction: () => VoidTransactionCallbackStatus | void, params?: RunTransactionParams, @@ -250,6 +245,8 @@ export class SchematizingSimpleTreeView< | void, params?: RunTransactionParams, ): TransactionResultExt | TransactionResult { + const { preconditions } = params ?? {}; + const addConstraints = ( constraintsOnRevert: boolean, constraints: readonly TransactionConstraint[] = [], @@ -260,7 +257,7 @@ export class SchematizingSimpleTreeView< this.checkout.transaction.start(); // Validate preconditions before running the transaction callback. - addConstraints(false /* constraintsOnRevert */, params?.preconditions); + addConstraints(false /* constraintsOnRevert */, preconditions); const transactionCallbackStatus = transaction(); const rollback = transactionCallbackStatus?.rollback; const value = ( diff --git a/packages/dds/tree/src/simple-tree/api/tree.ts b/packages/dds/tree/src/simple-tree/api/tree.ts index 90ef25d6c268..f0187fdee73d 100644 --- a/packages/dds/tree/src/simple-tree/api/tree.ts +++ b/packages/dds/tree/src/simple-tree/api/tree.ts @@ -168,14 +168,15 @@ export interface TreeBranch extends IDisposable { /** * Run a transaction which applies one or more edits to the tree as a single atomic unit. + * * @param transaction - The function to run as the body of the transaction. * It should return a status object of {@link TransactionCallbackStatus | TransactionCallbackStatus } type. * It includes a "rollback" property which may be returned as true at any point during the transaction. This will * abort the transaction and discard any changes it made so far. * "rollback" can be set to false or left undefined to indicate that the body of the transaction has successfully run. * @param params - The optional parameters for the transaction. It includes the constraints that will be checked before the transaction begins. - * @returns A result object of {@link TransactionResultExt | TransactionResultExt} type. It includes the following: * + * @returns A result object of {@link TransactionResultExt | TransactionResultExt} type. It includes the following: * - A "success" flag indicating whether the transaction was successful or not. * - The success or failure value as returned by the transaction function. * @@ -205,16 +206,17 @@ export interface TreeBranch extends IDisposable { ): TransactionResultExt; /** * Run a transaction which applies one or more edits to the tree as a single atomic unit. - * @param transaction - The function to run as the body of the transaction. It may return the following: * + * @param transaction - The function to run as the body of the transaction. It may return the following: * - Nothing to indicate that the body of the transaction has successfully run. * - A status object of {@link VoidTransactionCallbackStatus | VoidTransactionCallbackStatus } type. It includes a "rollback" property which * may be returned as true at any point during the transaction. This will abort the transaction and discard any changes it made so * far. "rollback" can be set to false or left undefined to indicate that the body of the transaction has successfully run. * * @param params - The optional parameters for the transaction. It includes the constraints that will be checked before the transaction begins. - * @returns A result object of {@link TransactionResult | TransactionResult} type. It includes a "success" flag indicating whether the - * transaction was successful or not. + * + * @returns A result object of {@link TransactionResult | TransactionResult} type. + * It includes a "success" flag indicating whether the transaction was successful or not. * * @remarks * This API will throw an error if the constraints are not met or something unexpected happens. 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..f9963c3c6767 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 TreeNodeKernelEventBatcher()); /** * 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,206 @@ const kernelEvents = ["childrenChangedAfterBatch", "subtreeChangedAfterBatch"] a type KernelEvents = Pick; +// #region Document this thoroughly + +const eventDeferralEmitter = createEmitter<{ + flush: () => void; +}>(); + +let pauseEvents: boolean = false; + +/** + * Pause events emitted by {@link TreeNode}s. + * @remarks Events that would otherwise have been emitted are buffered until the returned function is called. + * @returns A function that, when called, resumes event emission and flushes any buffered events. + */ +export function pauseTreeEvents(): () => void { + pauseEvents = true; + + return () => { + eventDeferralEmitter.emit("flush"); + pauseEvents = false; + }; +} + +// TODO: better name +class TreeNodeKernelEventBatcher + implements + Listenable, + Pick, "emit">, + HasListeners +{ + private disposed: boolean = false; + + private readonly disposeOnFlushListener = eventDeferralEmitter.on("flush", () => { + this.flush(); + }); + + private readonly childrenChangedListeners: Set< + (arg: { + changedFields: ReadonlySet; + }) => void + > = new Set(); + private readonly subTreeChangedListeners: Set<() => void> = new Set(); + + private readonly childrenChangedBuffer: Set = new Set(); + 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); + } + } + } + + // TODO: this can probably be cleaned up a lot + // TODO: overloads + public emit( + eventName: keyof KernelEvents, + arg?: { + changedFields: ReadonlySet; + }, + ): void { + this.assertNotDisposed(); + if (pauseEvents) { + // If events are currently paused, buffer the event to be flushed later. + switch (eventName) { + case "childrenChangedAfterBatch": + assert(arg !== undefined, "childrenChangedAfterBatch should have arg"); + for (const fieldKey of arg.changedFields) { + this.childrenChangedBuffer.add(fieldKey); + } + break; + case "subtreeChangedAfterBatch": + assert(arg === undefined, "subtreeChangedAfterBatch should not have arg"); + this.subTreeChangedBuffer = true; + break; + default: + unreachableCase(eventName); + } + } else { + // Otherwise, emit the event right away. + assert( + this.childrenChangedBuffer.size === 0, + "Events are not paused. Buffer should be empty.", + ); + assert( + this.subTreeChangedBuffer === false, + "Events are not paused. Buffer should be empty.", + ); + + switch (eventName) { + case "childrenChangedAfterBatch": + assert(arg !== undefined, "childrenChangedAfterBatch should have arg"); + for (const listener of this.childrenChangedListeners) { + listener(arg); + } + break; + case "subtreeChangedAfterBatch": + assert(arg === undefined, "subtreeChangedAfterBatch should not have arg"); + for (const listener of this.subTreeChangedListeners) { + listener(); + } + break; + default: + unreachableCase(eventName); + } + } + } + + // TODO: overloads + 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, + ); + break; + case "subtreeChangedAfterBatch": + this.subTreeChangedListeners.add(listener as () => void); + break; + default: + unreachableCase(eventName); + } + return () => this.off(eventName, listener); + } + + // TODO: overloads + 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 batched events. + */ + public flush(): void { + this.assertNotDisposed(); + assert(pauseEvents, "Expected events to be paused."); // TODO: remove this + + 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/tableSchema.ts b/packages/dds/tree/src/tableSchema.ts index b7665986e053..825352adb058 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: @@ -752,49 +753,30 @@ export namespace System_TableSchema { }); return removedColumns ?? fail("Transaction did not complete."); } else { - const columnsToRemove = indexOrColumns; - // If there are no columns to remove, do nothing - if (columnsToRemove.length === 0) { + if (indexOrColumns.length === 0) { return []; } - // Ensure the specified columns exists before starting transaction. + // Ensure the specified rows exists before starting transaction. // Improves user-facing error experience. - for (const columnToRemove of columnsToRemove) { - this._assertContainsColumn(columnToRemove); + const columnsToRemove: ColumnValueType[] = []; + for (const columnOrIdToRemove of indexOrColumns) { + columnsToRemove.push(this._getColumn(columnOrIdToRemove)); } - const removedColumns: ColumnValueType[] = []; - this._applyEditsInBatch(() => { - // Note, throwing an error within a transaction will abort the entire transaction. - // So if we throw an error here for any column, no columns will be removed. - for (const columnToRemove of columnsToRemove) { - const removedColumn = this._removeColumn(columnToRemove); - removedColumns.push(removedColumn); + this._applyEditsInBatch((): void => { + for (const column of columnsToRemove) { + // First, remove all cells that correspond to the column from each row: + this._removeCells(column); + + // Second, remove the column node: + this.columns.removeAt(this.columns.indexOf(column)); } }); - return removedColumns; - } - } - private _removeColumn(columnOrId: string | ColumnValueType): ColumnValueType { - const column = this._getColumn(columnOrId); - const index = this.columns.indexOf(column); - assert(index !== -1, "Column should exist"); - - this._applyEditsInBatch(() => { - // Remove the corresponding cell from all rows. - for (const row of this.rows) { - // TypeScript is unable to narrow the row type correctly here, hence the cast. - // See: https://github.com/microsoft/TypeScript/issues/52144 - (row as RowValueType).removeCell(column); - } - - this.columns.removeAt(index); - }); - - return column; + return columnsToRemove; + } } public removeRows( @@ -819,39 +801,27 @@ export namespace System_TableSchema { ); } - const rowsToRemove = indexOrRows; - // If there are no rows to remove, do nothing - if (rowsToRemove.length === 0) { + if (indexOrRows.length === 0) { return []; } // Ensure the specified rows exists before starting transaction. // Improves user-facing error experience. - for (const rowToRemove of rowsToRemove) { - this._assertContainsRow(rowToRemove); + const rowsToRemove: RowValueType[] = []; + for (const rowOrIdToRemove of indexOrRows) { + rowsToRemove.push(this._getRow(rowOrIdToRemove)); } - const removedRows: RowValueType[] = []; this._applyEditsInBatch(() => { // Note, throwing an error within a transaction will abort the entire transaction. // So if we throw an error here for any row, no rows will be removed. for (const rowToRemove of rowsToRemove) { - const removedRow = this._removeRow(rowToRemove); - removedRows.push(removedRow); + this.rows.removeAt(this.rows.indexOf(rowToRemove)); } }); - return removedRows; - } - - private _removeRow(rowOrId: string | RowValueType): RowValueType { - const rowToRemove = this._getRow(rowOrId); - const index = this.rows.indexOf(rowToRemove); - assert(index !== -1, "Row should exist"); - - this.rows.removeAt(index); - return rowToRemove; + return rowsToRemove; } public removeCell( @@ -915,37 +885,48 @@ 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(() => { + // To ensure the user is not spammed with events during the transaction, + // we will pause events for the duration. + // If this node is part of a collaborative session ("hydrated"), + // we will also batch the edits in a transaction to ensure the larger edit is atomic. + + const resumeAndFlushTreeEvents = 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, + // but we do still need to ensure the events are batched. applyEdits(); - }); + } else { + branch.runTransaction(() => { + applyEdits(); + }); + } + } finally { + resumeAndFlushTreeEvents(); } } /** - * Attempts to resolve a Column node or ID to a Column node. - * If a node is provided, it is returned as-is. - * If an ID is provided, we check the table for the corresponding Column node and return it if it exists, otherwise undefined. + * Attempts to resolve the provided Column node or ID to a Column node in the table. + * Returns `undefined` if there is no match. + * @remarks Searches for a match based strictly on the ID and returns that result. */ private _tryGetColumn( columnOrId: string | ColumnValueType, ): ColumnValueType | undefined { - return typeof columnOrId === "string" ? this.getColumn(columnOrId) : columnOrId; + const columnId = this._getColumnId(columnOrId); + return this.getColumn(columnId); } /** - * Attempts to resolve a Column node or ID to a Column node. - * If a node is provided, it is returned as-is. - * If an ID is provided, we check the table for the corresponding Column node and return it if it exists, otherwise we throw an exception. + * Attempts to resolve the provided Column node or ID to a Column node in the table. + * @throws Throws a `UsageError` if there is no match. + * @remarks Searches for a match based strictly on the ID and returns that result. */ private _getColumn(columnOrId: string | ColumnValueType): ColumnValueType { - const column = - typeof columnOrId === "string" ? this.getColumn(columnOrId) : columnOrId; + const column = this._tryGetColumn(columnOrId); if (column === undefined) { this._throwMissingColumnError(this._getColumnId(columnOrId)); } @@ -982,17 +963,6 @@ export namespace System_TableSchema { return this._getColumnIndex(columnId) !== undefined; } - /** - * Assert that the provided Column exists in the table. - * @throws Throws a `UsageError` if the Column does not exist. - */ - private _assertContainsColumn(columnOrId: ColumnValueType | string): void { - const columnId = this._getColumnId(columnOrId); - if (!this._containsColumnWithId(columnId)) { - this._throwMissingColumnError(columnId); - } - } - /** * Throw a `UsageError` for a missing Column by its ID. */ @@ -1001,21 +971,22 @@ export namespace System_TableSchema { } /** - * Attempts to resolve a Row node or ID to a Row node. - * If a node is provided, it is returned as-is. - * If an ID is provided, we check the table for the corresponding Row node and return it if it exists, otherwise undefined. + * Attempts to resolve the provided Row node or ID to a Row node in the table. + * Returns `undefined` if there is no match. + * @remarks Searches for a match based strictly on the ID and returns that result. */ private _tryGetRow(rowOrId: string | RowValueType): RowValueType | undefined { - return typeof rowOrId === "string" ? this.getRow(rowOrId) : rowOrId; + const rowId = this._getRowId(rowOrId); + return this.getRow(rowId); } /** - * Attempts to resolve a Row node or ID to a Row node. - * If a node is provided, it is returned as-is. - * If an ID is provided, we check the table for the corresponding Row node and return it if it exists, otherwise we throw an exception. + * Attempts to resolve the provided Row node or ID to a Row node in the table. + * @throws Throws a `UsageError` if there is no match. + * @remarks Searches for a match based strictly on the ID and returns that result. */ private _getRow(rowOrId: string | RowValueType): RowValueType { - const row = typeof rowOrId === "string" ? this.getRow(rowOrId) : rowOrId; + const row = this._tryGetRow(rowOrId); if (row === undefined) { this._throwMissingRowError(this._getRowId(rowOrId)); } @@ -1051,17 +1022,6 @@ export namespace System_TableSchema { return this._getRowIndex(rowId) !== undefined; } - /** - * Assert that the provided Row exists in the table. - * @throws Throws a `UsageError` if the Row does not exist. - */ - private _assertContainsRow(rowOrId: RowValueType | string): void { - const rowId = this._getRowId(rowOrId); - if (!this._containsRowWithId(rowId)) { - this._throwMissingRowError(rowId); - } - } - /** * Throw a `UsageError` for a missing Row by its ID. */ diff --git a/packages/dds/tree/src/test/simple-tree/core/treeNodeKernel.spec.ts b/packages/dds/tree/src/test/simple-tree/core/treeNodeKernel.spec.ts index b3cb11d75d6e..d55dc36c5575 100644 --- a/packages/dds/tree/src/test/simple-tree/core/treeNodeKernel.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/core/treeNodeKernel.spec.ts @@ -165,4 +165,6 @@ describe("simple-tree proxies", () => { assert(anchors.isEmpty()); }); } + + // TODO: event deferral tests }); diff --git a/packages/dds/tree/src/test/tableSchema.spec.ts b/packages/dds/tree/src/test/tableSchema.spec.ts index 4d4c140382ba..ef2945c4ff99 100644 --- a/packages/dds/tree/src/test/tableSchema.spec.ts +++ b/packages/dds/tree/src/test/tableSchema.spec.ts @@ -757,7 +757,7 @@ describe("TableFactory unit tests", () => { }); }); - describeHydration("removeColumns", (initializeTree) => { + describeHydration("removeColumns", (initializeTree, hydrated) => { it("Remove empty list", () => { const table = initializeTree(Table, { columns: [ @@ -959,7 +959,56 @@ describe("TableFactory unit tests", () => { ); // Additionally, `column-0` should not have been removed. - assert.equal(table.columns.length, 1); + assert(table.columns.length === 1); + }); + + it("Remove empty range", () => { + const table = initializeTree(Table, { + columns: [new Column({ id: "column-0", props: {} })], + rows: [], + }); + + table.removeColumns(0, 0); + assertEqualTrees(table, { + columns: [{ id: "column-0", props: {} }], + rows: [], + }); + }); + + it("Remove by index range", () => { + const column0 = new Column({ id: "column-0", props: {} }); + const column1 = new Column({ id: "column-1", props: {} }); + const column2 = new Column({ id: "column-2", props: {} }); + const column3 = new Column({ id: "column-3", props: {} }); + const table = initializeTree(Table, { + columns: [column0, column1, column2, column3], + rows: [ + new Row({ + id: "row-0", + cells: { + "column-0": { value: "Hello" }, + "column-2": { value: "world" }, + }, + }), + ], + }); + + // Remove columns 1-2 + table.removeColumns(1, 2); + assertEqualTrees(table, { + columns: [ + { id: "column-0", props: {} }, + { id: "column-3", props: {} }, + ], + rows: [ + { + id: "row-0", + cells: { + "column-0": { value: "Hello" }, + }, + }, + ], + }); }); it("Removing by range fails for invalid ranges", () => {