Skip to content

Remove Transactor.isInProgress #26535

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.

Sign up for GitHub

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

Merged
noencke merged 1 commit into from
Feb 25, 2026
Merged

Remove Transactor.isInProgress #26535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 12 additions & 22 deletions packages/dds/tree/src/shared-tree-core/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
Expand All @@ -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");
Expand All @@ -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");
Expand All @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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<TChange>[] = [];
Expand Down
2 changes: 1 addition & 1 deletion packages/dds/tree/src/shared-tree/schematizingTreeView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
);
Expand Down
6 changes: 3 additions & 3 deletions packages/dds/tree/src/shared-tree/sharedTree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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 */,
);
}
Expand All @@ -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) {
Expand Down
16 changes: 8 additions & 8 deletions packages/dds/tree/src/shared-tree/treeCheckout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.");
}

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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.");
}

Expand Down Expand Up @@ -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.",
);
Expand Down Expand Up @@ -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.");
}

Expand Down
56 changes: 26 additions & 30 deletions packages/dds/tree/src/test/shared-tree-core/transaction.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
},
};
},
Expand Down Expand Up @@ -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);
},
};
},
Expand Down Expand Up @@ -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);
},
};
});
Expand All @@ -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);
},
};
});
Expand Down Expand Up @@ -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(),
Expand Down
Loading
Loading