-
Notifications
You must be signed in to change notification settings - Fork 99
Fix a bug that could result in a duplicate delete event for a row #621
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: b41e365 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Size Change: +36 B (+0.05%) Total Size: 74.1 kB
ℹ️ View Unchanged
|
Size Change: 0 B Total Size: 1.44 kB ℹ️ View Unchanged
|
completedOp.type === `delete` && | ||
previousVisibleValue !== undefined && | ||
newVisibleValue === undefined && | ||
deepEquals(completedOp.value, previousVisibleValue) |
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.
I don't love really that we do all these deepEquals — they're fairly expensive — but I suppose cheaper than the UI framework re-rendering the same bits of UI 🤷
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.
In this particular case, we don't actually need a deepEqual right? Since a delete is a delete is a delete. If the optimistic mutation got rid of it already, we can just immediately assume the synced delete is the same?
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.
The deepEquals are not really about stoping re-renders, they are deduplicating events in the same batch that could result in an invalid state. This is particularly important when feeding into the d2 graph as aggregates could become wrong, or the multiplicity could and we then track inserts/deletes later incorrectly.
I agree, I would prefer not to have them, but I think we should approach that in a general redesign of this reconciliation process - I don't particularly like how it currently works.
In this particular case, we don't actually need a deepEqual right?
Maybe, but I don't really want to bet on it. It's still about removing duplicates and there could be multiple deletes for the same key in the same batch. We just want to ensure the state inside d2 is correct. I don't want to make an assumption, I would prefer to take a belt and braces at this point before we further refactor.
Do note that these deepEquals
are only happening on optimistic mutations, not on every change that is synced, and not on every emitted initial state into the live query engine. As overhead it's tiny compared to the other things happening in D2.
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.
Do note that these deepEquals are only happening on optimistic mutations, not on every change that is synced
Ah ok, so very little generally 👍
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.
fixes #609
We had code in place that filtered out redundant change events from sync operations, but it failed to filter delete events, only filtering insert/update.
CI run with test failure showing the repo before the fix is applied: https://github.com/TanStack/db/actions/runs/18163302240/job/51698976831?pr=621