-
Notifications
You must be signed in to change notification settings - Fork 69
fix transaction sorting order and enforce them having an id #230
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
Conversation
🦋 Changeset detectedLatest commit: ef13a14 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@tanstack/db-example-react-todo
commit: |
Size Change: +169 B (+0.58%) Total Size: 29.2 kB
ℹ️ View Unchanged
|
Size Change: 0 B Total Size: 561 B ℹ️ View Unchanged
|
@@ -67,20 +56,25 @@ export class Transaction< | |||
public isPersisted: Deferred<Transaction<T, TOperation>> | |||
public autoCommit: boolean | |||
public createdAt: Date | |||
public sequenceNumber: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
On new Transaction
— we could just only export the type so people can't use it. It was definitely my mistake using new Transaction
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, I changed it to only export the type, and changed the direct mutations to use createTransaction.
While working on #204 I found two bugs:
direct insert/update/delete mutations where not settings an id on the transaction, this resulted in them being overwritten with further synchronous mutations as they had an id or
undefined
when being looked up. I have fixed that by ensuring that a transaction id will always fallback to a new uuid if not set.@KyleAMathews you used
new Transaction()
rather thancreateTransaction()
in these mutations, this means they are not added to the global list of transactions. Is this intentional or should then usecreateTransaction()
? If so I will fix that here as it's a very related bug, the id was enforced increateTransaction
, but not in the Transaction constructor.transactions were sorted by
createdAt
when rebasing over the latest synced data, however if two transactions happened in the same ms they would be ordered arbitrarily. This could result in the optimistic mutations happing in the wrong order - in localOnlyCollectionOptions collection for ephemeral local state #204 this manifested as a very strange situation where half the tests would fail, but if you only ran one of the failing tests it would work... I think this is also highly likely the cause of a flaky test I have seen when running the tests locally but never seen in CI.To fix this I have added an additional
sequenceNumber
to transaction, a monotonic counter in the current session - when we sort we compare bycreatedAt
first, then fall back tosequenceNumber
when they are created at the same time. I have leftcreatedAt
as this will be useful in future when we have some level of persistence for optimistic mutations.