fix transaction sorting order and enforce them having an id (#230)

Author: samwillis

.changeset/khaki-cougars-give.md

@@ -0,0 +1,5 @@
+---
+"@tanstack/db": patch
+---
+
+add a sequence number to transactions to when sorting we can ensure that those created in the same ms are sorted in the correct order

.changeset/open-foxes-say.md

@@ -0,0 +1,5 @@
+---
+"@tanstack/db": patch
+---
+
+Ensure that all transactions are given an id, fixes a potential bug with direct mutations

packages/db/src/collection.ts

@@ -1,7 +1,8 @@
 import { Store } from "@tanstack/store"
 import { withArrayChangeTracking, withChangeTracking } from "./proxy"
-import { Transaction, getActiveTransaction } from "./transactions"
+import { createTransaction, getActiveTransaction } from "./transactions"
 import { SortedMap } from "./SortedMap"
+import type { Transaction } from "./transactions"
 import type {
   ChangeListener,
   ChangeMessage,
@@ -277,8 +278,8 @@ export class CollectionImpl<
       throw new Error(`Collection requires a sync config`)
     }
 
-    this.transactions = new SortedMap<string, Transaction<any>>(
-      (a, b) => a.createdAt.getTime() - b.createdAt.getTime()
+    this.transactions = new SortedMap<string, Transaction<any>>((a, b) =>
+      a.compareCreatedAt(b)
     )
 
     this.config = config
@@ -1242,7 +1243,7 @@ export class CollectionImpl<
       return ambientTransaction
     } else {
       // Create a new transaction with a mutation function that calls the onInsert handler
-      const directOpTransaction = new Transaction<T>({
+      const directOpTransaction = createTransaction<T>({
         mutationFn: async (params) => {
           // Call the onInsert handler with the transaction
           return this.config.onInsert!(params)
@@ -1444,7 +1445,7 @@ export class CollectionImpl<
 
     // If no changes were made, return an empty transaction early
     if (mutations.length === 0) {
-      const emptyTransaction = new Transaction({
+      const emptyTransaction = createTransaction({
         mutationFn: async () => {},
       })
       emptyTransaction.commit()
@@ -1464,7 +1465,7 @@ export class CollectionImpl<
     // No need to check for onUpdate handler here as we've already checked at the beginning
 
     // Create a new transaction with a mutation function that calls the onUpdate handler
-    const directOpTransaction = new Transaction<T>({
+    const directOpTransaction = createTransaction<T>({
       mutationFn: async (params) => {
         // Call the onUpdate handler with the transaction
         return this.config.onUpdate!(params)
@@ -1559,7 +1560,7 @@ export class CollectionImpl<
     }
 
     // Create a new transaction with a mutation function that calls the onDelete handler
-    const directOpTransaction = new Transaction<T>({
+    const directOpTransaction = createTransaction<T>({
       autoCommit: true,
       mutationFn: async (params) => {
         // Call the onDelete handler with the transaction

packages/db/src/transactions.ts

@@ -12,24 +12,13 @@ import type {
 const transactions: Array<Transaction<any>> = []
 let transactionStack: Array<Transaction<any>> = []
 
+let sequenceNumber = 0
+
 export function createTransaction<
   TData extends object = Record<string, unknown>,
 >(config: TransactionConfig<TData>): Transaction<TData> {
-  if (typeof config.mutationFn === `undefined`) {
-    throw `mutationFn is required when creating a transaction`
-  }
-
-  let transactionId = config.id
-  if (!transactionId) {
-    transactionId = crypto.randomUUID()
-  }
-  const newTransaction = new Transaction<TData>({
-    ...config,
-    id: transactionId,
-  })
-
+  const newTransaction = new Transaction<TData>(config)
   transactions.push(newTransaction)
-
   return newTransaction
 }
 
@@ -56,7 +45,7 @@ function removeFromPendingList(tx: Transaction<any>) {
   }
 }
 
-export class Transaction<
+class Transaction<
   T extends object = Record<string, unknown>,
   TOperation extends OperationType = OperationType,
 > {
@@ -67,20 +56,25 @@ export class Transaction<
   public isPersisted: Deferred<Transaction<T, TOperation>>
   public autoCommit: boolean
   public createdAt: Date
+  public sequenceNumber: number
   public metadata: Record<string, unknown>
   public error?: {
     message: string
     error: Error
   }
 
   constructor(config: TransactionConfig<T>) {
-    this.id = config.id!
+    if (typeof config.mutationFn === `undefined`) {
+      throw `mutationFn is required when creating a transaction`
+    }
+    this.id = config.id ?? crypto.randomUUID()
     this.mutationFn = config.mutationFn
     this.state = `pending`
     this.mutations = []
     this.isPersisted = createDeferred<Transaction<T, TOperation>>()
     this.autoCommit = config.autoCommit ?? true
     this.createdAt = new Date()
+    this.sequenceNumber = sequenceNumber++
     this.metadata = config.metadata ?? {}
   }
 
@@ -210,4 +204,21 @@ export class Transaction<
 
     return this
   }
+
+  /**
+   * Compare two transactions by their createdAt time and sequence number in order
+   * to sort them in the order they were created.
+   * @param other - The other transaction to compare to
+   * @returns -1 if this transaction was created before the other, 1 if it was created after, 0 if they were created at the same time
+   */
+  compareCreatedAt(other: Transaction<any>): number {
+    const createdAtComparison =
+      this.createdAt.getTime() - other.createdAt.getTime()
+    if (createdAtComparison !== 0) {
+      return createdAtComparison
+    }
+    return this.sequenceNumber - other.sequenceNumber
+  }
 }
+
+export type { Transaction }