Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Clone to SharedTree Revertible #23044

Open
wants to merge 47 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
408ff9e
first commit
jikim-msft Nov 8, 2024
bdfbd3a
add getCheckout & test case
jikim-msft Nov 8, 2024
7f0777e
no nuke map
jikim-msft Nov 8, 2024
94b8b5a
add test
jikim-msft Nov 9, 2024
73dfa36
Merge branch 'main' into revertible-alpha
jikim-msft Nov 9, 2024
1364971
policy fix
jikim-msft Nov 9, 2024
f0a7c6e
Merge branch 'revertible-alpha' of https://github.com/jikim-msft/Flui…
jikim-msft Nov 9, 2024
0d593b4
refactor
jikim-msft Nov 9, 2024
09f10e5
change test suite name
jikim-msft Nov 9, 2024
896f22f
change name to RevertibleAlpha & remove DisposableRevertible
jikim-msft Nov 11, 2024
b967512
inline logic
jikim-msft Nov 11, 2024
33e1fde
refactor create undo/redo stack function & change name
jikim-msft Nov 12, 2024
0f62865
add doc
jikim-msft Nov 12, 2024
698e1c1
add failing test case
jikim-msft Nov 12, 2024
5caf3c6
remove only
jikim-msft Nov 12, 2024
1784895
change onDispose to required
jikim-msft Nov 12, 2024
0d2bef6
Merge branch 'microsoft:main' into revertible-alpha
jikim-msft Nov 12, 2024
022404a
Revert "refactor create undo/redo stack function & change name"
jikim-msft Nov 12, 2024
2ab318c
revert & replace
jikim-msft Nov 12, 2024
aaff66b
Update packages/dds/tree/src/shared-tree/treeCheckout.ts
jikim-msft Nov 12, 2024
115354b
Update packages/dds/tree/src/core/revertible.ts
jikim-msft Nov 12, 2024
f0c989c
Update packages/dds/tree/src/core/revertible.ts
jikim-msft Nov 12, 2024
dea35f7
Update packages/dds/tree/src/core/revertible.ts
jikim-msft Nov 12, 2024
7596fa7
Update packages/dds/tree/src/core/revertible.ts
jikim-msft Nov 12, 2024
0ec8140
Update packages/dds/tree/src/core/revertible.ts
jikim-msft Nov 12, 2024
e518bb7
change API tag
jikim-msft Nov 12, 2024
13be6d4
Merge branch 'revertible-alpha' of https://github.com/jikim-msft/Flui…
jikim-msft Nov 12, 2024
b3ac268
Merge branch 'microsoft:main' into revertible-alpha
jikim-msft Nov 12, 2024
855fe89
add TODO & update api
jikim-msft Nov 13, 2024
cec77a5
change name back to createTestUndoRedoStacks
jikim-msft Nov 13, 2024
c4ed841
add doc
jikim-msft Nov 13, 2024
85319e7
fix import
jikim-msft Nov 13, 2024
80946ab
change from commit to changed
jikim-msft Nov 13, 2024
7d87eb7
change name of forkedBranch
jikim-msft Nov 13, 2024
bd83bd2
update api doc
jikim-msft Nov 13, 2024
684943e
Update packages/dds/tree/src/test/utils.ts
jikim-msft Nov 13, 2024
9969510
fix doc
jikim-msft Nov 13, 2024
41acc0d
change function param
jikim-msft Nov 13, 2024
b7b2b2f
Merge branch 'microsoft:main' into revertible-alpha
jikim-msft Nov 13, 2024
fa5e774
simplify test
jikim-msft Nov 13, 2024
ec15843
update api
jikim-msft Nov 13, 2024
233749d
update doc
jikim-msft Nov 13, 2024
e50bc39
Update packages/dds/tree/src/core/revertible.ts
jikim-msft Nov 14, 2024
fec248e
Update packages/dds/tree/src/core/revertible.ts
jikim-msft Nov 14, 2024
5603232
Update packages/dds/tree/src/shared-tree/treeCheckout.ts
jikim-msft Nov 14, 2024
eddc5a3
fix nit
jikim-msft Nov 14, 2024
04fdc50
add test
jikim-msft Nov 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/dds/tree/api-report/tree.alpha.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,14 @@ export interface Revertible {
readonly status: RevertibleStatus;
}

// @alpha @sealed
export interface RevertibleAlpha extends Revertible {
clone: (forkedBranch?: TreeBranch) => RevertibleAlpha;
}

// @alpha @sealed
export type RevertibleAlphaFactory = (onRevertibleDisposed?: (revertible: RevertibleAlpha) => void) => RevertibleAlpha;

// @public @sealed
export type RevertibleFactory = (onRevertibleDisposed?: (revertible: Revertible) => void) => Revertible;

Expand Down Expand Up @@ -721,8 +729,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;
}

Expand Down
8 changes: 7 additions & 1 deletion packages/dds/tree/src/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
31 changes: 31 additions & 0 deletions packages/dds/tree/src/core/revertible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -36,6 +38,21 @@ export interface Revertible {
dispose(): void;
}

/**
* A {@link Revertible} with features that are net yet stable.
*
* @sealed @alpha
*/
export interface RevertibleAlpha extends Revertible {
/**
* TODO: Add description
*
* @param forkedBranch - A target branch to apply the revertible to. If not given, spawns a cloned revertible of the original branch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should prob add additional details about what we exactly expect of this branch, mainly that it has the same commit that the revertible is meant to revert. also, what happens if we have a cloned revertible of the original branch? are the lifetimes of the clone and the original tied together?

* @returns A cloned revertible.
*/
clone: (forkedBranch?: TreeBranch) => RevertibleAlpha;
jikim-msft marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* The status of a {@link Revertible}.
*
Expand All @@ -62,3 +79,17 @@ export enum RevertibleStatus {
export type RevertibleFactory = (
onRevertibleDisposed?: (revertible: Revertible) => void,
) => Revertible;

/**
* Will error if invoked outside the scope of the `changed` event that provides it, or if invoked multiple times.
jikim-msft marked this conversation as resolved.
Show resolved Hide resolved
*
* @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;
2 changes: 2 additions & 0 deletions packages/dds/tree/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ export {
MapNodeStoredSchema,
LeafNodeStoredSchema,
type RevertibleFactory,
type RevertibleAlphaFactory,
type RevertibleAlpha,
} from "./core/index.js";
export { type Brand } from "./util/index.js";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
jenn-le marked this conversation as resolved.
Show resolved Hide resolved
if (context instanceof SchematizingSimpleTreeView) {
return context.checkout;
}
Expand Down
118 changes: 71 additions & 47 deletions packages/dds/tree/src/shared-tree/treeCheckout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import {
type IEditableForest,
type IForestSubscription,
type JsonableTree,
type Revertible,
RevertibleStatus,
type RevisionTag,
type RevisionTagCodec,
Expand All @@ -37,7 +36,8 @@ import {
rootFieldKey,
tagChange,
visitDelta,
type RevertibleFactory,
type RevertibleAlphaFactory,
type RevertibleAlpha,
} from "../core/index.js";
import {
type HasListeners,
Expand Down Expand Up @@ -68,8 +68,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}.
Expand All @@ -92,7 +93,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;
}

/**
Expand Down Expand Up @@ -397,7 +398,7 @@ export class TreeCheckout implements ITreeCheckoutFork {
/**
* Set of revertibles maintained for automatic disposal
*/
private readonly revertibles = new Set<DisposableRevertible>();
private readonly revertibles = new Set<RevertibleAlpha>();

/**
* Each branch's head commit corresponds to a revertible commit.
Expand Down Expand Up @@ -525,7 +526,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.",
Expand All @@ -536,42 +537,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;
Expand Down Expand Up @@ -626,6 +597,63 @@ export class TreeCheckout implements ITreeCheckoutFork {
}
}

private createRevertible(
jikim-msft marked this conversation as resolved.
Show resolved Hide resolved
revision: RevisionTag,
kind: CommitKind,
checkout: TreeCheckout,
onRevertibleDisposed: (revertible: RevertibleAlpha) => void,
jikim-msft marked this conversation as resolved.
Show resolved Hide resolved
): 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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what was the reasoning behind allowing forkedBranch to be optional? why not just make it required and not allow clone to be called without a fork? the behavior for cloning a revertible onto the same checkout doesn't seem obvious to me and i'm not sure why we'd want to do so

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloning a revertible onto the same branch is useful for an application that wants to provide ownership of a revertible to multiple components. Suppose I have two components in my application that:

  1. Don't know about each other
  2. Are operating on the same branch (e.g. the main branch)
  3. Both want to be able to revert (e.g. they both have undo buttons)

You'd want to clone the revertible, giving one to the first component and the clone to the second. Or, perhaps the parent component retains ownership of the original revertible and gives a new clone to each subcomponent. If you can't clone the revertible for the same branch, then you have to share the same revertible between the parent and both components, and now they do need to know about each other so that they know when to (or when not to) dispose the revertible.

Regarding the parameter being optional, it's just a convenience. For an application like the above, which operates solely on the main branch, it would be annoying at best and confusing at worst to be forced to pass in the main branch every time it wants to do a clone. With it being optional, the user doesn't need to understand the branching feature at all in order to clone revertibles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be useful, but I'm not entirely convinced that the changes here sufficiently support it. At the very least, I think it needs to be documented better.

some thoughts:

  • if one of the components does a revert, how does it get communicated to the other? do we just fail whenever the other ones tries to do a revert and we can't find the repair data anymore?
  • it seems like the status is currently cloned as well, which is fine since revertibles can be disposed without being reverted, but I think we'd need some way to properly communicate between clones whether the repair data they share can still be used
  • do we maybe want a different api for cloning with the same branch? it kinda sounds like the meaning changes depending on if we're cloning with a different branch or the same branch i.e. with a different branch, nothing you do with the cloned revertible will affect the original but with the same branch, they still have the same dependencies even though they can be owned by different components and disposed separately as well although i guess it would be somewhat similar to the case where a user explicitly does not dispose after revert and we call revert again on the same revertible and it just noops. which I still think it kinda strange.
  • I think having the parameter be optional is fine if the behavior around operating on the same branch is more clear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a set of clones of the same revertible associated with the same branch, the repair data will not be collected until all clones have called dispose(). This is the intended behavior, and I believe is the currently behavior, but if it's not covered by tests then we should add more tests.

So, yes, if component A and component B each have a clone of a revertible, and are on the same branch, then when A does a revert it will do something, and when B does a revert it will (probably) no-op. This is what I would expect - the reason you cloned in the first place is so that A and B don't have to know what each other are doing, therefore, if B does an undo but it was already undone by A, nothing will happen (because it "already happened"). The other option which you alluded to in your first bullet is for B's undo stack to be changed along with A's undo stack. But if that's what the user wants, then A and B are not isolated and the user shouldn't have done a clone in the first place. Undo-stacks are probably 1:1 with branches most of the time, is my guess. Being able to clone a revertible on the same branch is more of a ref-counting thing. Like you have some code in your application that wants to ensure it can revert some specific data at some point in the future, so it holds on to a particular revertible - regardless of whether or not a clone of that revertible might get reverted anyway by some other means. I think it's likely to be a pretty uncommon scenario, but it might be useful to have as an option, it's easy for us to implement, and it's the natural behavior (IMO) of a parameterless call to clone.

A review of potential scenarios (e.g. with Nick) to inform the documentation would probably not be a bad idea. If we find that it's just too hard to explain why you would do this, then that could be a sign that we should just leave it out.

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, "SharedTreeBranch for revertible not found.");
jikim-msft marked this conversation as resolved.
Show resolved Hide resolved
forkedCheckout.revertibleCommitBranches.set(revision, revertibleBranch.fork());
jikim-msft marked this conversation as resolved.
Show resolved Hide resolved

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<TRoot extends ImplicitFieldSchema | UnsafeUnknownSchema>(
config: TreeViewConfiguration<ReadSchema<TRoot>>,
Expand Down Expand Up @@ -792,7 +820,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);
Expand Down Expand Up @@ -890,7 +918,3 @@ export function runSynchronous(
? view.transaction.abort()
: view.transaction.commit();
}

interface DisposableRevertible extends Revertible {
jikim-msft marked this conversation as resolved.
Show resolved Hide resolved
dispose: () => void;
}
10 changes: 7 additions & 3 deletions packages/dds/tree/src/simple-tree/api/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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:
Expand All @@ -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;
}

/**
Expand Down
Loading
Loading