From 5abfa015aff9d639d82830f3ad828324d5680bd7 Mon Sep 17 00:00:00 2001 From: Ji Kim <111468570+jikim-msft@users.noreply.github.com> Date: Wed, 20 Nov 2024 15:45:41 -0800 Subject: [PATCH] Add Clone to SharedTree Revertible (#23044) #### Description [13864](https://dev.azure.com/fluidframework/internal/_workitems/edit/13864/) This PR adds forkable revertible feature to the `Revertible` object of SharedTree. - Removed `DisposableRevertible` and replaced by `RevertibleAlpha`. - Added `clone()` method to the new interface. - Uses `TreeBranch` (which is subset of `TreeCheckout`) to access data necessary for revert operation. --------- Co-authored-by: Noah Encke <78610362+noencke@users.noreply.github.com> Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com> Co-authored-by: Jenn --- .changeset/dull-words-work.md | 11 + .../dds/tree/api-report/tree.alpha.api.md | 12 +- packages/dds/tree/src/core/index.ts | 8 +- packages/dds/tree/src/core/revertible.ts | 36 ++- packages/dds/tree/src/index.ts | 2 + .../src/shared-tree/schematizingTreeView.ts | 2 +- .../dds/tree/src/shared-tree/treeCheckout.ts | 131 ++++++---- packages/dds/tree/src/simple-tree/api/tree.ts | 10 +- .../tree/src/test/shared-tree/undo.spec.ts | 224 +++++++++++++++++- packages/dds/tree/src/test/utils.ts | 18 +- .../api-report/fluid-framework.alpha.api.md | 12 +- 11 files changed, 398 insertions(+), 68 deletions(-) create mode 100644 .changeset/dull-words-work.md diff --git a/.changeset/dull-words-work.md b/.changeset/dull-words-work.md new file mode 100644 index 000000000000..8fe0c13e78e2 --- /dev/null +++ b/.changeset/dull-words-work.md @@ -0,0 +1,11 @@ +--- +"fluid-framework": minor +"@fluidframework/tree": minor +--- +--- +"section": tree +--- + +Enables Revertible objects to be cloned using `RevertibleAlpha.clone()`. + +Replaced `DisposableRevertible` with `RevertibleAlpha`. The new `RevertibleAlpha` interface extends `Revertible` and includes a `clone(branch: TreeBranch)` method to facilitate cloning a Revertible to a specified target branch. The source branch where the `RevertibleAlpha` was created must share revision logs with the target branch where the `RevertibleAlpha` is being cloned. If this condition is not met, the operation will throw an error. diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index dfbfce02afbc..519f1c3f4c90 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -539,6 +539,14 @@ export interface Revertible { readonly status: RevertibleStatus; } +// @alpha @sealed +export interface RevertibleAlpha extends Revertible { + clone: (branch: TreeBranch) => RevertibleAlpha; +} + +// @alpha @sealed +export type RevertibleAlphaFactory = (onRevertibleDisposed?: (revertible: RevertibleAlpha) => void) => RevertibleAlpha; + // @public @sealed export type RevertibleFactory = (onRevertibleDisposed?: (revertible: Revertible) => void) => Revertible; @@ -723,8 +731,8 @@ export interface TreeBranch extends IDisposable { // @alpha @sealed export interface TreeBranchEvents { - changed(data: CommitMetadata, getRevertible?: RevertibleFactory): void; - commitApplied(data: CommitMetadata, getRevertible?: RevertibleFactory): void; + changed(data: CommitMetadata, getRevertible?: RevertibleAlphaFactory): void; + commitApplied(data: CommitMetadata, getRevertible?: RevertibleAlphaFactory): void; schemaChanged(): void; } diff --git a/packages/dds/tree/src/core/index.ts b/packages/dds/tree/src/core/index.ts index 948b96836e26..9becff643c25 100644 --- a/packages/dds/tree/src/core/index.ts +++ b/packages/dds/tree/src/core/index.ts @@ -206,4 +206,10 @@ export { AllowedUpdateType, } from "./schema-view/index.js"; -export { type Revertible, RevertibleStatus, type RevertibleFactory } from "./revertible.js"; +export { + type Revertible, + RevertibleStatus, + type RevertibleFactory, + type RevertibleAlphaFactory, + type RevertibleAlpha, +} from "./revertible.js"; diff --git a/packages/dds/tree/src/core/revertible.ts b/packages/dds/tree/src/core/revertible.ts index 735daaeba40e..5fbbaebe59ef 100644 --- a/packages/dds/tree/src/core/revertible.ts +++ b/packages/dds/tree/src/core/revertible.ts @@ -3,6 +3,8 @@ * Licensed under the MIT License. */ +import type { TreeBranch } from "../simple-tree/index.js"; + /** * Allows reversion of a change made to SharedTree. * @@ -36,6 +38,23 @@ export interface Revertible { dispose(): void; } +/** + * A {@link Revertible} with features that are not yet stable. + * + * @sealed @alpha + */ +export interface RevertibleAlpha extends Revertible { + /** + * Clones the {@link Revertible} to a target branch. + * + * @param branch - A target branch to apply the revertible to. + * The target branch must contain the same commit that this revertible is meant to revert, otherwise will throw an error. + * @returns A cloned revertible is independent of the original, meaning disposing of one will not affect the other, + * provided they do not belong to the same branch. Both revertibles can be reverted independently. + */ + clone: (branch: TreeBranch) => RevertibleAlpha; +} + /** * The status of a {@link Revertible}. * @@ -50,7 +69,7 @@ export enum RevertibleStatus { /** * Factory for creating a {@link Revertible}. - * Will error if invoked outside the scope of the `changed` event that provides it, or if invoked multiple times. + * @throws error if invoked outside the scope of the `changed` event that provides it, or if invoked multiple times. * * @param onRevertibleDisposed - A callback that will be invoked when the `Revertible` generated by this factory is disposed. * This happens when the `Revertible` is disposed manually, or when the `TreeView` that the `Revertible` belongs to is disposed, @@ -62,3 +81,18 @@ export enum RevertibleStatus { export type RevertibleFactory = ( onRevertibleDisposed?: (revertible: Revertible) => void, ) => Revertible; + +/** + * Factory for creating a {@link RevertibleAlpha}. + * @throws error if invoked outside the scope of the `changed` event that provides it, or if invoked multiple times. + * + * @param onRevertibleDisposed - A callback that will be invoked when the {@link RevertibleAlpha | Revertible} generated by this factory is disposed. + * This happens when the `Revertible` is disposed manually, or when the `TreeView` that the `Revertible` belongs to is disposed, + * whichever happens first. + * This is typically used to clean up any resources associated with the `Revertible` in the host application. + * + * @sealed @alpha + */ +export type RevertibleAlphaFactory = ( + onRevertibleDisposed?: (revertible: RevertibleAlpha) => void, +) => RevertibleAlpha; diff --git a/packages/dds/tree/src/index.ts b/packages/dds/tree/src/index.ts index 5f123fb5a167..a7319cb689eb 100644 --- a/packages/dds/tree/src/index.ts +++ b/packages/dds/tree/src/index.ts @@ -29,6 +29,8 @@ export { MapNodeStoredSchema, LeafNodeStoredSchema, type RevertibleFactory, + type RevertibleAlphaFactory, + type RevertibleAlpha, } from "./core/index.js"; export { type Brand } from "./util/index.js"; diff --git a/packages/dds/tree/src/shared-tree/schematizingTreeView.ts b/packages/dds/tree/src/shared-tree/schematizingTreeView.ts index bcf1d14dc851..1bf94a594809 100644 --- a/packages/dds/tree/src/shared-tree/schematizingTreeView.ts +++ b/packages/dds/tree/src/shared-tree/schematizingTreeView.ts @@ -409,7 +409,7 @@ export class SchematizingSimpleTreeView< * @remarks Currently, all contexts are also {@link SchematizingSimpleTreeView}s. * Other checkout implementations (e.g. not associated with a view) may be supported in the future. */ -function getCheckout(context: TreeBranch): TreeCheckout { +export function getCheckout(context: TreeBranch): TreeCheckout { if (context instanceof SchematizingSimpleTreeView) { return context.checkout; } diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index 5088b8d22252..2cf1e23bd644 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -24,7 +24,6 @@ import { type IEditableForest, type IForestSubscription, type JsonableTree, - type Revertible, RevertibleStatus, type RevisionTag, type RevisionTagCodec, @@ -37,7 +36,8 @@ import { rootFieldKey, tagChange, visitDelta, - type RevertibleFactory, + type RevertibleAlphaFactory, + type RevertibleAlpha, } from "../core/index.js"; import { type HasListeners, @@ -80,8 +80,9 @@ import type { TreeViewConfiguration, UnsafeUnknownSchema, ViewableTree, + TreeBranch, } from "../simple-tree/index.js"; -import { SchematizingSimpleTreeView } from "./schematizingTreeView.js"; +import { getCheckout, SchematizingSimpleTreeView } from "./schematizingTreeView.js"; /** * Events for {@link ITreeCheckout}. @@ -104,7 +105,7 @@ export interface CheckoutEvents { * @param getRevertible - a function provided that allows users to get a revertible for the change. If not provided, * this change is not revertible. */ - changed(data: CommitMetadata, getRevertible?: RevertibleFactory): void; + changed(data: CommitMetadata, getRevertible?: RevertibleAlphaFactory): void; } /** @@ -409,7 +410,7 @@ export class TreeCheckout implements ITreeCheckoutFork { /** * Set of revertibles maintained for automatic disposal */ - private readonly revertibles = new Set(); + private readonly revertibles = new Set(); /** * Each branch's head commit corresponds to a revertible commit. @@ -542,7 +543,7 @@ export class TreeCheckout implements ITreeCheckoutFork { const getRevertible = hasSchemaChange(change) ? undefined - : (onRevertibleDisposed?: (revertible: Revertible) => void) => { + : (onRevertibleDisposed?: (revertible: RevertibleAlpha) => void) => { if (!withinEventContext) { throw new UsageError( "Cannot get a revertible outside of the context of a changed event.", @@ -553,42 +554,12 @@ export class TreeCheckout implements ITreeCheckoutFork { "Cannot generate the same revertible more than once. Note that this can happen when multiple changed event listeners are registered.", ); } - const revertibleCommits = this.revertibleCommitBranches; - const revertible: DisposableRevertible = { - get status(): RevertibleStatus { - const revertibleCommit = revertibleCommits.get(revision); - return revertibleCommit === undefined - ? RevertibleStatus.Disposed - : RevertibleStatus.Valid; - }, - revert: (release: boolean = true) => { - if (revertible.status === RevertibleStatus.Disposed) { - throw new UsageError( - "Unable to revert a revertible that has been disposed.", - ); - } - - const revertMetrics = this.revertRevertible(revision, kind); - this.logger?.sendTelemetryEvent({ - eventName: TreeCheckout.revertTelemetryEventName, - ...revertMetrics, - }); - - if (release) { - revertible.dispose(); - } - }, - dispose: () => { - if (revertible.status === RevertibleStatus.Disposed) { - throw new UsageError( - "Unable to dispose a revertible that has already been disposed.", - ); - } - this.disposeRevertible(revertible, revision); - onRevertibleDisposed?.(revertible); - }, - }; - + const revertible = this.createRevertible( + revision, + kind, + this, + onRevertibleDisposed, + ); this.revertibleCommitBranches.set(revision, _branch.fork(commit)); this.revertibles.add(revertible); return revertible; @@ -643,6 +614,76 @@ export class TreeCheckout implements ITreeCheckoutFork { } } + /** + * Creates a {@link RevertibleAlpha} object that can undo a specific change in the tree's history. + * Revision must exist in the given {@link TreeCheckout}'s branch. + * + * @param revision - The revision tag identifying the change to be made revertible. + * @param kind - The {@link CommitKind} that produced this revertible (e.g., Default, Undo, Redo). + * @param checkout - The {@link TreeCheckout} instance this revertible belongs to. + * @param onRevertibleDisposed - Callback function that will be called when the revertible is disposed. + * @returns - {@link RevertibleAlpha} + */ + private createRevertible( + revision: RevisionTag, + kind: CommitKind, + checkout: TreeCheckout, + onRevertibleDisposed: ((revertible: RevertibleAlpha) => void) | undefined, + ): RevertibleAlpha { + const commitBranches = checkout.revertibleCommitBranches; + + const revertible: RevertibleAlpha = { + get status(): RevertibleStatus { + const revertibleCommit = commitBranches.get(revision); + return revertibleCommit === undefined + ? RevertibleStatus.Disposed + : RevertibleStatus.Valid; + }, + revert: (release: boolean = true) => { + if (revertible.status === RevertibleStatus.Disposed) { + throw new UsageError("Unable to revert a revertible that has been disposed."); + } + + const revertMetrics = checkout.revertRevertible(revision, kind); + checkout.logger?.sendTelemetryEvent({ + eventName: TreeCheckout.revertTelemetryEventName, + ...revertMetrics, + }); + + if (release) { + revertible.dispose(); + } + }, + clone: (forkedBranch: TreeBranch) => { + if (forkedBranch === undefined) { + return this.createRevertible(revision, kind, checkout, onRevertibleDisposed); + } + + // TODO:#23442: When a revertible is cloned for a forked branch, optimize to create a fork of a revertible branch once per revision NOT once per revision per checkout. + const forkedCheckout = getCheckout(forkedBranch); + const revertibleBranch = this.revertibleCommitBranches.get(revision); + assert( + revertibleBranch !== undefined, + "change to revert does not exist on the given forked branch", + ); + forkedCheckout.revertibleCommitBranches.set(revision, revertibleBranch.fork()); + + return this.createRevertible(revision, kind, forkedCheckout, onRevertibleDisposed); + }, + dispose: () => { + if (revertible.status === RevertibleStatus.Disposed) { + throw new UsageError( + "Unable to dispose a revertible that has already been disposed.", + ); + } + checkout.disposeRevertible(revertible, revision); + onRevertibleDisposed?.(revertible); + }, + }; + + return revertible; + } + // For the new TreeViewAlpha API public viewWith( config: TreeViewConfiguration>, @@ -809,7 +850,7 @@ export class TreeCheckout implements ITreeCheckoutFork { } } - private disposeRevertible(revertible: DisposableRevertible, revision: RevisionTag): void { + private disposeRevertible(revertible: RevertibleAlpha, revision: RevisionTag): void { this.revertibleCommitBranches.get(revision)?.dispose(); this.revertibleCommitBranches.delete(revision); this.revertibles.delete(revertible); @@ -922,7 +963,3 @@ export function runSynchronous( ? view.transaction.abort() : view.transaction.commit(); } - -interface DisposableRevertible extends Revertible { - dispose: () => void; -} diff --git a/packages/dds/tree/src/simple-tree/api/tree.ts b/packages/dds/tree/src/simple-tree/api/tree.ts index c693afb90f7f..370ac30f0403 100644 --- a/packages/dds/tree/src/simple-tree/api/tree.ts +++ b/packages/dds/tree/src/simple-tree/api/tree.ts @@ -6,7 +6,11 @@ import type { IFluidLoadable, IDisposable } from "@fluidframework/core-interfaces"; import { UsageError } from "@fluidframework/telemetry-utils/internal"; -import type { CommitMetadata, RevertibleFactory } from "../../core/index.js"; +import type { + CommitMetadata, + RevertibleAlphaFactory, + RevertibleFactory, +} from "../../core/index.js"; import type { Listenable } from "../../events/index.js"; import { @@ -629,7 +633,7 @@ export interface TreeBranchEvents { * @param getRevertible - a function that allows users to get a revertible for the change. If not provided, * this change is not revertible. */ - changed(data: CommitMetadata, getRevertible?: RevertibleFactory): void; + changed(data: CommitMetadata, getRevertible?: RevertibleAlphaFactory): void; /** * Fired when: @@ -644,7 +648,7 @@ export interface TreeBranchEvents { * @param getRevertible - a function provided that allows users to get a revertible for the commit that was applied. If not provided, * this commit is not revertible. */ - commitApplied(data: CommitMetadata, getRevertible?: RevertibleFactory): void; + commitApplied(data: CommitMetadata, getRevertible?: RevertibleAlphaFactory): void; } /** diff --git a/packages/dds/tree/src/test/shared-tree/undo.spec.ts b/packages/dds/tree/src/test/shared-tree/undo.spec.ts index 5115b9140183..0cdcb14cb554 100644 --- a/packages/dds/tree/src/test/shared-tree/undo.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/undo.spec.ts @@ -6,13 +6,19 @@ import { type FieldUpPath, type Revertible, + RevertibleStatus, type UpPath, rootFieldKey, } from "../../core/index.js"; import { singleJsonCursor } from "../json/index.js"; import { SharedTreeFactory, type ITreeCheckout } from "../../shared-tree/index.js"; import { type JsonCompatible, brand } from "../../util/index.js"; -import { createTestUndoRedoStacks, expectJsonTree, moveWithin } from "../utils.js"; +import { + createTestUndoRedoStacks, + expectJsonTree, + moveWithin, + TestTreeProviderLite, +} from "../utils.js"; import { insert, jsonSequenceRootSchema, remove } from "../sequenceRootUtils.js"; import { createIdCompressor } from "@fluidframework/id-compressor/internal"; import { @@ -21,7 +27,11 @@ import { MockStorage, } from "@fluidframework/test-runtime-utils/internal"; import assert from "node:assert"; -import { SchemaFactory, TreeViewConfiguration } from "../../simple-tree/index.js"; +import { + asTreeViewAlpha, + SchemaFactory, + TreeViewConfiguration, +} from "../../simple-tree/index.js"; // eslint-disable-next-line import/no-internal-modules import { initialize } from "../../shared-tree/schematizeTree.js"; @@ -158,6 +168,44 @@ const testCases: { }, ]; +/** + * Schema definitions for forkable revertible test suites. + * TODO: Should be removed once #24414 is implemented. + */ +function createInitializedView() { + const factory = new SchemaFactory("shared-tree-test"); + class ChildNodeSchema extends factory.object("child-item", { + propertyOne: factory.optional(factory.number), + propertyTwo: factory.object("propertyTwo-item", { + itemOne: factory.string, + }), + }) {} + class RootNodeSchema extends factory.object("root-item", { + child: factory.optional(ChildNodeSchema), + }) {} + const provider = new TestTreeProviderLite(); + const view = asTreeViewAlpha( + provider.trees[0].viewWith( + new TreeViewConfiguration({ + schema: RootNodeSchema, + }), + ), + ); + + view.initialize( + new RootNodeSchema({ + child: { + propertyOne: 128, + propertyTwo: { + itemOne: "", + }, + }, + }), + ); + + return view; +} + describe("Undo and redo", () => { for (const attached of [true, false]) { const attachStr = attached ? "attached" : "detached"; @@ -453,6 +501,178 @@ describe("Undo and redo", () => { revertible.revert(); assert.equal(view.root.foo, 1); }); + + // TODO:#24414: Enable forkable revertibles tests to run on attached/detached mode. + it("reverts original & forked revertibles after making change to the original view", () => { + const originalView = createInitializedView(); + const { undoStack } = createTestUndoRedoStacks(originalView.events); + + assert(originalView.root.child !== undefined); + originalView.root.child.propertyOne = 256; // 128 -> 256 + + const forkedView = originalView.fork(); + + const propertyOneUndo = undoStack.pop(); + const clonedPropertyOneUndo = propertyOneUndo?.clone(forkedView); + + propertyOneUndo?.revert(); + + assert.equal(originalView.root.child?.propertyOne, 128); + assert.equal(forkedView.root.child?.propertyOne, 256); + assert.equal(propertyOneUndo?.status, RevertibleStatus.Disposed); + assert.equal(clonedPropertyOneUndo?.status, RevertibleStatus.Valid); + + clonedPropertyOneUndo?.revert(); + + assert.equal(forkedView.root.child?.propertyOne, 128); + assert.equal(clonedPropertyOneUndo?.status, RevertibleStatus.Disposed); + }); + + // TODO:#24414: Enable forkable revertibles tests to run on attached/detached mode. + it("reverts original & forked revertibles after making separate changes to the original & forked view", () => { + const originalView = createInitializedView(); + const { undoStack: undoStack1 } = createTestUndoRedoStacks(originalView.events); + + assert(originalView.root.child !== undefined); + originalView.root.child.propertyOne = 256; // 128 -> 256 + originalView.root.child.propertyTwo.itemOne = "newItem"; + + const forkedView = originalView.fork(); + const { undoStack: undoStack2 } = createTestUndoRedoStacks(forkedView.events); + + assert(forkedView.root.child !== undefined); + forkedView.root.child.propertyOne = 512; // 256 -> 512 + + undoStack2.pop()?.revert(); + assert.equal(forkedView.root.child?.propertyOne, 256); + + const undoOriginalPropertyTwo = undoStack1.pop(); + const clonedUndoOriginalPropertyTwo = undoOriginalPropertyTwo?.clone(forkedView); + + const undoOriginalPropertyOne = undoStack1.pop(); + const clonedUndoOriginalPropertyOne = undoOriginalPropertyOne?.clone(forkedView); + + undoOriginalPropertyOne?.revert(); + undoOriginalPropertyTwo?.revert(); + + assert.equal(originalView.root.child?.propertyOne, 128); + assert.equal(originalView.root.child?.propertyTwo.itemOne, ""); + assert.equal(forkedView.root.child?.propertyOne, 256); + assert.equal(forkedView.root.child?.propertyTwo.itemOne, "newItem"); + + clonedUndoOriginalPropertyOne?.revert(); + clonedUndoOriginalPropertyTwo?.revert(); + + assert.equal(forkedView.root.child?.propertyOne, 128); + assert.equal(forkedView.root.child?.propertyTwo.itemOne, ""); + + assert.equal(undoOriginalPropertyOne?.status, RevertibleStatus.Disposed); + assert.equal(undoOriginalPropertyTwo?.status, RevertibleStatus.Disposed); + assert.equal(clonedUndoOriginalPropertyOne?.status, RevertibleStatus.Disposed); + assert.equal(clonedUndoOriginalPropertyTwo?.status, RevertibleStatus.Disposed); + }); + + // TODO:#24414: Enable forkable revertibles tests to run on attached/detached mode. + it("reverts cloned revertible on original view", () => { + const view = createInitializedView(); + const { undoStack } = createTestUndoRedoStacks(view.events); + + assert(view.root.child !== undefined); + view.root.child.propertyOne = 256; // 128 -> 256 + view.root.child.propertyTwo.itemOne = "newItem"; + + const undoOriginalPropertyTwo = undoStack.pop(); + const undoOriginalPropertyOne = undoStack.pop(); + + const clonedUndoOriginalPropertyTwo = undoOriginalPropertyTwo?.clone(view); + const clonedUndoOriginalPropertyOne = undoOriginalPropertyOne?.clone(view); + + clonedUndoOriginalPropertyTwo?.revert(); + clonedUndoOriginalPropertyOne?.revert(); + + assert.equal(view.root.child?.propertyOne, 128); + assert.equal(view.root.child?.propertyTwo.itemOne, ""); + assert.equal(undoOriginalPropertyOne?.status, RevertibleStatus.Disposed); + assert.equal(undoOriginalPropertyTwo?.status, RevertibleStatus.Disposed); + assert.equal(clonedUndoOriginalPropertyOne?.status, RevertibleStatus.Disposed); + assert.equal(clonedUndoOriginalPropertyTwo?.status, RevertibleStatus.Disposed); + }); + + // TODO:#24414: Enable forkable revertibles tests to run on attached/detached mode. + it("reverts cloned revertible prior to original revertible", () => { + const originalView = createInitializedView(); + const { undoStack } = createTestUndoRedoStacks(originalView.events); + + assert(originalView.root.child !== undefined); + originalView.root.child.propertyOne = 256; // 128 -> 256 + originalView.root.child.propertyTwo.itemOne = "newItem"; + + const forkedView = originalView.fork(); + + const undoOriginalPropertyTwo = undoStack.pop(); + const undoOriginalPropertyOne = undoStack.pop(); + + const clonedUndoOriginalPropertyTwo = undoOriginalPropertyTwo?.clone(forkedView); + const clonedUndoOriginalPropertyOne = undoOriginalPropertyOne?.clone(forkedView); + + clonedUndoOriginalPropertyTwo?.revert(); + clonedUndoOriginalPropertyOne?.revert(); + + assert.equal(originalView.root.child?.propertyOne, 256); + assert.equal(originalView.root.child?.propertyTwo.itemOne, "newItem"); + assert.equal(forkedView.root.child?.propertyOne, 128); + assert.equal(forkedView.root.child?.propertyTwo.itemOne, ""); + assert.equal(undoOriginalPropertyOne?.status, RevertibleStatus.Valid); + assert.equal(undoOriginalPropertyTwo?.status, RevertibleStatus.Valid); + assert.equal(clonedUndoOriginalPropertyOne?.status, RevertibleStatus.Disposed); + assert.equal(clonedUndoOriginalPropertyTwo?.status, RevertibleStatus.Disposed); + + undoOriginalPropertyTwo?.revert(); + undoOriginalPropertyOne?.revert(); + + assert.equal(originalView.root.child?.propertyOne, 128); + assert.equal(originalView.root.child?.propertyTwo.itemOne, ""); + assert.equal(undoOriginalPropertyOne?.status, RevertibleStatus.Disposed); + assert.equal(undoOriginalPropertyTwo?.status, RevertibleStatus.Disposed); + }); + + // TODO:#24414: Enable forkable revertibles tests to run on attached/detached mode. + it("clone revertible fails if trees are different", () => { + const viewA = createInitializedView(); + const viewB = createInitializedView(); + + const { undoStack } = createTestUndoRedoStacks(viewA.events); + + assert(viewA.root.child !== undefined); + viewA.root.child.propertyOne = 256; // 128 -> 256 + + const undoOriginalPropertyOne = undoStack.pop(); + + assert.throws(() => undoOriginalPropertyOne?.clone(viewB).revert(), "Error: 0x576"); + }); + + // TODO:#24414: Enable forkable revertibles tests to run on attached/detached mode. + it("cloned revertible fails if already applied", () => { + const view = createInitializedView(); + const { undoStack } = createTestUndoRedoStacks(view.events); + + assert(view.root.child !== undefined); + view.root.child.propertyOne = 256; // 128 -> 256 + + const undoOriginalPropertyOne = undoStack.pop(); + const clonedUndoOriginalPropertyOne = undoOriginalPropertyOne?.clone(view); + + undoOriginalPropertyOne?.revert(); + + assert.equal(view.root.child?.propertyOne, 128); + assert.equal(undoOriginalPropertyOne?.status, RevertibleStatus.Disposed); + assert.equal(clonedUndoOriginalPropertyOne?.status, RevertibleStatus.Disposed); + + assert.throws( + () => clonedUndoOriginalPropertyOne?.revert(), + "Error: Unable to revert a revertible that has been disposed.", + ); + }); }); /** diff --git a/packages/dds/tree/src/test/utils.ts b/packages/dds/tree/src/test/utils.ts index e59db37a61f3..7d53459ada19 100644 --- a/packages/dds/tree/src/test/utils.ts +++ b/packages/dds/tree/src/test/utils.ts @@ -62,7 +62,6 @@ import { type IEditableForest, type IForestSubscription, type JsonableTree, - type Revertible, type RevisionInfo, type RevisionMetadataSource, type RevisionTag, @@ -86,7 +85,8 @@ import { type TreeStoredSchemaSubscription, type ITreeCursorSynchronous, CursorLocationType, - type RevertibleFactory, + type RevertibleAlpha, + type RevertibleAlphaFactory, } from "../core/index.js"; import type { HasListeners, IEmitter, Listenable } from "../events/index.js"; import { typeboxValidator } from "../external-utilities/index.js"; @@ -450,7 +450,7 @@ export function spyOnMethod( } /** - * @returns `true` iff the given delta has a visible impact on the document tree. + * Determines whether or not the given delta has a visible impact on the document tree. */ export function isDeltaVisible(delta: DeltaFieldChanges): boolean { for (const mark of delta.local ?? []) { @@ -1049,14 +1049,14 @@ export function rootFromDeltaFieldMap( export function createTestUndoRedoStacks( events: Listenable, ): { - undoStack: Revertible[]; - redoStack: Revertible[]; + undoStack: RevertibleAlpha[]; + redoStack: RevertibleAlpha[]; unsubscribe: () => void; } { - const undoStack: Revertible[] = []; - const redoStack: Revertible[] = []; + const undoStack: RevertibleAlpha[] = []; + const redoStack: RevertibleAlpha[] = []; - function onDispose(disposed: Revertible): void { + function onDispose(disposed: RevertibleAlpha): void { const redoIndex = redoStack.indexOf(disposed); if (redoIndex !== -1) { redoStack.splice(redoIndex, 1); @@ -1068,7 +1068,7 @@ export function createTestUndoRedoStacks( } } - function onNewCommit(commit: CommitMetadata, getRevertible?: RevertibleFactory): void { + function onNewCommit(commit: CommitMetadata, getRevertible?: RevertibleAlphaFactory): void { if (getRevertible !== undefined) { const revertible = getRevertible(onDispose); if (commit.kind === CommitKind.Undo) { 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 c26c6e686899..d109b5934b61 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 @@ -892,6 +892,14 @@ export interface Revertible { readonly status: RevertibleStatus; } +// @alpha @sealed +export interface RevertibleAlpha extends Revertible { + clone: (branch: TreeBranch) => RevertibleAlpha; +} + +// @alpha @sealed +export type RevertibleAlphaFactory = (onRevertibleDisposed?: (revertible: RevertibleAlpha) => void) => RevertibleAlpha; + // @public @sealed export type RevertibleFactory = (onRevertibleDisposed?: (revertible: Revertible) => void) => Revertible; @@ -1098,8 +1106,8 @@ export interface TreeBranch extends IDisposable { // @alpha @sealed export interface TreeBranchEvents { - changed(data: CommitMetadata, getRevertible?: RevertibleFactory): void; - commitApplied(data: CommitMetadata, getRevertible?: RevertibleFactory): void; + changed(data: CommitMetadata, getRevertible?: RevertibleAlphaFactory): void; + commitApplied(data: CommitMetadata, getRevertible?: RevertibleAlphaFactory): void; schemaChanged(): void; }