diff --git a/packages/dds/tree/src/shared-tree-core/transaction.ts b/packages/dds/tree/src/shared-tree-core/transaction.ts index bd9677ecf9f5..c37ac49cfa6f 100644 --- a/packages/dds/tree/src/shared-tree-core/transaction.ts +++ b/packages/dds/tree/src/shared-tree-core/transaction.ts @@ -58,16 +58,11 @@ export interface Transactor { * Close this transaction and revert the state of the tree to what it was before this transaction began. */ abort(): void; - /** - * True if there is at least one transaction currently in progress on this view, otherwise false. - */ - isInProgress(): boolean; /** * The number of transactions currently in progress, including any nested transactions. * @remarks This is 0 when no transaction is in progress, 1 when a single transaction is in progress, 2 when a transaction is nested inside another, etc. - * TODO: This method may make {@link Transactor.isInProgress} redundant since it can be implemented as `size() > 0`. Consider whether both are necessary. */ - size(): number; + size: number; /** * Provides events for changes in transaction progress. */ @@ -77,17 +72,17 @@ export interface Transactor { export interface TransactionEvents { /** * Raised just after a transaction has begun. - * @remarks When this event fires, {@link Transactor.isInProgress} will be true because the transaction has already begun. + * @remarks When this event fires, {@link Transactor.size} will be greater than 0 because the transaction has already begun. */ started(): void; /** * Raised just before a transaction is aborted. - * @remarks When this event fires, {@link Transactor.isInProgress} will still be true because the transaction has not yet ended. + * @remarks When this event fires, {@link Transactor.size} will still be greater than 0 because the transaction has not yet ended. */ aborting(): void; /** * Raised just before a transaction is committed. - * @remarks When this event fires, {@link Transactor.isInProgress} will still be true because the transaction has not yet ended. + * @remarks When this event fires, {@link Transactor.size} will still be greater than 0 because the transaction has not yet ended. */ committing(): void; } @@ -115,13 +110,13 @@ export interface Callbacks { /** * A function that will be called when a transaction is pushed to the {@link TransactionStack | stack}. * @remarks This function may return other functions that will be called when the transaction is popped from the stack or a nested transaction is pushed onto the stack. - * This function runs just before the transaction begins, so if this is the beginning of an outermost (not nested) transaction then {@link Transactor.isInProgress} will be false during its execution. + * This function runs just before the transaction begins, so if this is the beginning of an outermost (not nested) transaction then {@link Transactor.size} will be 0 during its execution. */ export type OnPush = () => Callbacks | void; /** * A function that will be called when a transaction is popped from the {@link TransactionStack | stack}. - * @remarks This function runs just after the transaction ends, so if this is the end of an outermost (not nested) transaction then {@link Transactor.isInProgress} will be false during its execution. + * @remarks This function runs just after the transaction ends, so if this is the end of an outermost (not nested) transaction then {@link Transactor.size} will be 0 during its execution. */ export type OnPop = (result: TransactionResult) => void; @@ -159,12 +154,7 @@ export class TransactionStack implements Transactor, IDisposable { this.#onPush = onPush; } - public isInProgress(): boolean { - this.ensureNotDisposed(); - return this.#stack.length > 0; - } - - public size(): number { + public get size(): number { this.ensureNotDisposed(); return this.#stack.length; } @@ -182,7 +172,7 @@ export class TransactionStack implements Transactor, IDisposable { public commit(): void { this.ensureNotDisposed(); - if (!this.isInProgress()) { + if (this.size === 0) { throw new UsageError("No transaction to commit"); } this.#events.emit("committing"); @@ -191,7 +181,7 @@ export class TransactionStack implements Transactor, IDisposable { public abort(): void { this.ensureNotDisposed(); - if (!this.isInProgress()) { + if (this.size === 0) { throw new UsageError("No transaction to abort"); } this.#events.emit("aborting"); @@ -200,7 +190,7 @@ export class TransactionStack implements Transactor, IDisposable { public dispose(): void { this.ensureNotDisposed(); - while (this.isInProgress()) { + while (this.size > 0) { this.abort(); } this.#disposed = true; @@ -215,7 +205,7 @@ export class TransactionStack implements Transactor, IDisposable { /** * A function that will be called when a transaction is popped from the {@link SquashingTransactionStack | stack}. - * @remarks This function runs just after the transaction ends, so if this is the end of an outermost (not nested) transaction then {@link Transactor.isInProgress} will be false during its execution. + * @remarks This function runs just after the transaction ends, so if this is the end of an outermost (not nested) transaction then {@link Transactor.size} will be 0 during its execution. * @param result - The result of the transaction. * @param viewUpdate - The change that needs to be applied to the view to keep it up-to-date with the branch after the transaction ends. * This is needed in asynchronous transactions where new commits have been added to the branch while the transaction was in progress. @@ -318,7 +308,7 @@ export class SquashingTransactionStack< // Invoked when an outer transaction ends const onOuterTransactionPop: OnPop = (result) => { - assert(!this.isInProgress(), 0xcae /* The outer transaction should be ending */); + assert(this.size === 0, 0xcae /* The outer transaction should be ending */); transactionBranch.editor.exitTransaction(); const sourcePath: GraphCommit[] = []; diff --git a/packages/dds/tree/src/shared-tree/schematizingTreeView.ts b/packages/dds/tree/src/shared-tree/schematizingTreeView.ts index 4c1845fff73d..5a690ffd1a8e 100644 --- a/packages/dds/tree/src/shared-tree/schematizingTreeView.ts +++ b/packages/dds/tree/src/shared-tree/schematizingTreeView.ts @@ -332,7 +332,7 @@ export class SchematizingSimpleTreeView< private mountTransaction(params: RunTransactionParams | undefined, isAsync: boolean): void { this.ensureUndisposed(); const { checkout } = this; - if (isAsync && checkout.transaction.isInProgress()) { + if (isAsync && checkout.transaction.size > 0) { throw new UsageError( "An asynchronous transaction cannot be started while another transaction is already in progress.", ); diff --git a/packages/dds/tree/src/shared-tree/sharedTree.ts b/packages/dds/tree/src/shared-tree/sharedTree.ts index 9c005a574646..8e353cb0bb54 100644 --- a/packages/dds/tree/src/shared-tree/sharedTree.ts +++ b/packages/dds/tree/src/shared-tree/sharedTree.ts @@ -420,7 +420,7 @@ export class SharedTreeKernel public override didAttach(): void { for (const checkout of this.checkouts.values()) { - if (checkout.transaction.isInProgress()) { + if (checkout.transaction.size > 0) { // Attaching during a transaction is not currently supported. // At least part of of the system is known to not handle this case correctly - commit enrichment - and there may be others. throw new UsageError( @@ -438,7 +438,7 @@ export class SharedTreeKernel ): void { for (const checkout of this.checkouts.values()) { assert( - !checkout.transaction.isInProgress(), + checkout.transaction.size === 0, 0x674 /* Unexpected transaction is open while applying stashed ops */, ); } @@ -453,7 +453,7 @@ export class SharedTreeKernel ): void { const checkout = this.getCheckout(branchId); assert( - !checkout.transaction.isInProgress(), + checkout.transaction.size === 0, 0xaa6 /* Cannot submit a commit while a transaction is in progress */, ); if (isResubmit) { diff --git a/packages/dds/tree/src/shared-tree/treeCheckout.ts b/packages/dds/tree/src/shared-tree/treeCheckout.ts index 32ee7ebfef12..6a04699eaf5f 100644 --- a/packages/dds/tree/src/shared-tree/treeCheckout.ts +++ b/packages/dds/tree/src/shared-tree/treeCheckout.ts @@ -514,7 +514,7 @@ export class TreeCheckout implements ITreeCheckoutFork { if (viewUpdate !== undefined) { this.applyChange(viewUpdate, newHead.revision); } - if (!this.transaction.isInProgress()) { + if (this.transaction.size === 0) { // The changes in a transaction squash commit have already applied to the checkout and are known to be valid, so we can validate the squash commit automatically. this.validateCommit(newHead); } @@ -870,7 +870,7 @@ export class TreeCheckout implements ITreeCheckoutFork { "The parent branch has already been disposed and can no longer create new branches.", ); // Branching after an unfinished transaction would expose the application to a state where its invariants may be violated. - if (this.transaction.isInProgress()) { + if (this.transaction.size > 0) { throw new UsageError("A view cannot be forked while it has a pending transaction."); } @@ -902,7 +902,7 @@ export class TreeCheckout implements ITreeCheckoutFork { ): void { // TODO: Dispose old branch, if necessary assert( - !this.#transaction.isInProgress(), + this.#transaction.size === 0, 0xc55 /* Cannot switch branches during a transaction */, ); const diff = diffHistories( @@ -932,12 +932,12 @@ export class TreeCheckout implements ITreeCheckoutFork { ); this.editLock.checkUnlocked("Rebasing"); - if (this.transaction.isInProgress()) { + if (this.transaction.size > 0) { throw new UsageError( "Views cannot be rebased onto a view that has a pending transaction.", ); } - if (checkout.transaction.isInProgress()) { + if (checkout.transaction.size > 0) { throw new UsageError("A view cannot be rebased while it has a pending transaction."); } @@ -966,12 +966,12 @@ export class TreeCheckout implements ITreeCheckoutFork { "The source of the branch merge has been disposed and cannot be merged.", ); this.editLock.checkUnlocked("Merging"); - if (this.transaction.isInProgress()) { + if (this.transaction.size > 0) { throw new UsageError( "Views cannot be merged into a view while it has a pending transaction.", ); } - if (checkout.transaction.isInProgress()) { + if (checkout.transaction.size > 0) { throw new UsageError( "Views with an open transaction cannot be merged into another view.", ); @@ -1057,7 +1057,7 @@ export class TreeCheckout implements ITreeCheckoutFork { private revertRevertible(revision: RevisionTag, kind: CommitKind): RevertMetrics { this.editLock.checkUnlocked("Reverting a commit"); - if (this.transaction.isInProgress()) { + if (this.transaction.size > 0) { throw new UsageError("Undo is not yet supported during transactions."); } diff --git a/packages/dds/tree/src/test/shared-tree-core/transaction.spec.ts b/packages/dds/tree/src/test/shared-tree-core/transaction.spec.ts index 106a87c676fa..bf6507aa3b28 100644 --- a/packages/dds/tree/src/test/shared-tree-core/transaction.spec.ts +++ b/packages/dds/tree/src/test/shared-tree-core/transaction.spec.ts @@ -33,7 +33,7 @@ describe("TransactionStacks", () => { const transaction = new TransactionStack(); let started = false; transaction.events.on("started", () => { - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 1); started = true; }); transaction.start(); @@ -44,7 +44,7 @@ describe("TransactionStacks", () => { const transaction = new TransactionStack(); let aborting = false; transaction.events.on("aborting", () => { - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 1); aborting = true; }); transaction.start(); @@ -56,7 +56,7 @@ describe("TransactionStacks", () => { const transaction = new TransactionStack(); let committing = false; transaction.events.on("committing", () => { - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 1); committing = true; }); transaction.start(); @@ -66,39 +66,39 @@ describe("TransactionStacks", () => { it("report whether or not a transaction is in progress", () => { const transaction = new TransactionStack(); - assert.equal(transaction.isInProgress(), false); + assert.equal(transaction.size, 0); transaction.start(); - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 1); transaction.start(); - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 2); transaction.commit(); - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 1); transaction.abort(); - assert.equal(transaction.isInProgress(), false); + assert.equal(transaction.size, 0); }); it("report the number of transactions in progress", () => { const transaction = new TransactionStack(); - assert.equal(transaction.size(), 0); + assert.equal(transaction.size, 0); transaction.start(); - assert.equal(transaction.size(), 1); + assert.equal(transaction.size, 1); transaction.start(); - assert.equal(transaction.size(), 2); + assert.equal(transaction.size, 2); transaction.start(); - assert.equal(transaction.size(), 3); + assert.equal(transaction.size, 3); transaction.commit(); - assert.equal(transaction.size(), 2); + assert.equal(transaction.size, 2); transaction.abort(); - assert.equal(transaction.size(), 1); + assert.equal(transaction.size, 1); transaction.commit(); - assert.equal(transaction.size(), 0); + assert.equal(transaction.size, 0); }); it("run a function when a transaction begins", () => { let invoked = 0; const transaction = new TransactionStack((): void => { invoked += 1; - assert.equal(transaction.isInProgress(), false); + assert.equal(transaction.size, 0); }); transaction.start(); assert.equal(invoked, 1); @@ -108,7 +108,7 @@ describe("TransactionStacks", () => { let invoked = 0; const transaction = new TransactionStack((): void => { invoked += 1; - assert.equal(transaction.isInProgress(), invoked > 1); + assert.equal(transaction.size, invoked - 1); }); transaction.start(); assert.equal(invoked, 1); @@ -124,15 +124,15 @@ describe("TransactionStacks", () => { let invokedInner2 = 0; const transaction: TransactionStack = new TransactionStack(() => { invokedOuter += 1; - assert.equal(transaction.isInProgress(), false); + assert.equal(transaction.size, 0); return { onPush: () => { invokedInner1 += 1; - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 1); return { onPush: () => { invokedInner2 += 1; - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, invokedInner2 + 1); }, }; }, @@ -164,19 +164,19 @@ describe("TransactionStacks", () => { return { onPop: () => { invokedOuter += 1; - assert.equal(transaction.isInProgress(), false); + assert.equal(transaction.size, 0); }, onPush: () => { return { onPop: () => { invokedInner1 += 1; - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 1); }, onPush: () => { return { onPop: () => { invokedInner2 += 1; - assert.equal(transaction.isInProgress(), true); + assert.equal(transaction.size, 2); }, }; }, @@ -208,7 +208,7 @@ describe("TransactionStacks", () => { onPop: (result) => { invoked += 1; assert.equal(result, TransactionResult.Abort); - assert.equal(transaction.isInProgress(), false); + assert.equal(transaction.size, 0); }, }; }); @@ -225,7 +225,7 @@ describe("TransactionStacks", () => { onPop: (result) => { invoked += 1; assert.equal(result, TransactionResult.Commit); - assert.equal(transaction.isInProgress(), false); + assert.equal(transaction.size, 0); }, }; }); @@ -256,11 +256,7 @@ describe("TransactionStacks", () => { assert.equal(transaction.disposed, false); transaction.dispose(); assert.equal(transaction.disposed, true); - assert.throws( - () => transaction.isInProgress(), - validateAssertionError("Transactor is disposed"), - ); - assert.throws(() => transaction.size(), validateAssertionError("Transactor is disposed")); + assert.throws(() => transaction.size, validateAssertionError("Transactor is disposed")); assert.throws(() => transaction.start(), validateAssertionError("Transactor is disposed")); assert.throws( () => transaction.commit(), diff --git a/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts b/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts index 0c633312aeca..529686f5cdcd 100644 --- a/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts +++ b/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditGenerators.ts @@ -576,7 +576,7 @@ export const makeTransactionEditGenerator = ( boundary: "commit", }, opWeights.commit, - (state) => viewFromState(state).checkout.transaction.isInProgress(), + (state) => viewFromState(state).checkout.transaction.size > 0, ], [ { @@ -584,7 +584,7 @@ export const makeTransactionEditGenerator = ( boundary: "abort", }, opWeights.abort, - (state) => viewFromState(state).checkout.transaction.isInProgress(), + (state) => viewFromState(state).checkout.transaction.size > 0, ], ]); }; @@ -756,7 +756,7 @@ export function makeOpGenerator( [ () => makeConstraintEditGenerator(weights), constraintWeight, - (state: FuzzTestState) => viewFromState(state).checkout.transaction.isInProgress(), + (state: FuzzTestState) => viewFromState(state).checkout.transaction.size > 0, ], [ () => makeBranchEditGenerator(weights), diff --git a/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditReducers.ts b/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditReducers.ts index c75319d68cde..1641ce5ab26b 100644 --- a/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditReducers.ts +++ b/packages/dds/tree/src/test/shared-tree/fuzz/fuzzEditReducers.ts @@ -230,7 +230,7 @@ export function applyForkMergeOperation( : clientForkedViews[branchEdit.contents.baseBranch]; assert(forkBranchIndex !== undefined); const forkedBranch = clientForkedViews[forkBranchIndex]; - if (baseBranch.checkout.transaction.isInProgress() === true) { + if (baseBranch.checkout.transaction.size > 0) { return; } @@ -415,7 +415,7 @@ export function applyTransactionBoundary( } } - if (!checkout.transaction.isInProgress()) { + if (checkout.transaction.size === 0) { // Transaction is complete, so merge the changes into the root view and clean up the fork from the state. state.transactionViews.delete(state.client.channel); const rootView = viewFromState(state); diff --git a/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts b/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts index 51b59fd16efc..dbbefb90f990 100644 --- a/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts +++ b/packages/dds/tree/src/test/shared-tree/treeCheckout.spec.ts @@ -773,16 +773,16 @@ describe("sharedTreeView", () => { }); itView("statuses are reported correctly", ({ view }) => { - assert.equal(view.checkout.transaction.isInProgress(), false); + assert.equal(view.checkout.transaction.size, 0); view.runTransaction(() => { - assert.equal(view.checkout.transaction.isInProgress(), true); + assert.equal(view.checkout.transaction.size, 1); view.runTransaction(() => { - assert.equal(view.checkout.transaction.isInProgress(), true); + assert.equal(view.checkout.transaction.size, 2); }); - assert.equal(view.checkout.transaction.isInProgress(), true); + assert.equal(view.checkout.transaction.size, 1); return { rollback: true }; }); - assert.equal(view.checkout.transaction.isInProgress(), false); + assert.equal(view.checkout.transaction.size, 0); }); it("rejects async transactions within existing transactions", async () => {