-
Notifications
You must be signed in to change notification settings - Fork 99
feat: Add flexible matching strategies for electric-db-collection (#402) #499
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
base: main
Are you sure you want to change the base?
Conversation
- Add three matching strategies for client-server synchronization: 1. Txid strategy (existing, backward compatible) 2. Custom match function strategy (new) 3. Void/timeout strategy (new, 3-second default) - New types: MatchFunction<T>, MatchingStrategy<T> - Enhanced ElectricCollectionConfig to support all strategies - New utility: awaitMatch(matchFn, timeout?) - Export isChangeMessage and isControlMessage helpers - Remove deprecated error classes (beta compatibility not required) - Comprehensive tests for all strategies including timeout behavior - Updated documentation with detailed examples and migration guide Benefits: - Backward compatibility maintained - Architecture flexibility for different backend capabilities - Progressive enhancement path - No forced backend API changes 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
🦋 Changeset detectedLatest commit: 89e7cce The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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: +14 B (+0.02%) Total Size: 75.2 kB
ℹ️ View Unchanged
|
Size Change: 0 B Total Size: 1.47 kB ℹ️ View Unchanged
|
…nd API consistency Critical fixes based on thorough code review: **🔧 Commit Semantics Fix:** - awaitMatch now waits for up-to-date after finding match (like awaitTxId) - Ensures consistent behavior between txid and custom match strategies - Prevents race conditions where mutations marked "persisted" before actual commit **🧠 Memory Leak Fixes:** - Properly cleanup pendingMatches on timeout and abort - Add abort listener to cleanup all pending matches on stream abort - Use cross-platform ReturnType<typeof setTimeout> instead of NodeJS.Timeout **🎯 API Consistency:** - Unified MatchingStrategy type used across all handler return types - Support configurable timeout for void strategy: { timeout: 1500 } - Remove unused discriminator type field for cleaner duck-typed unions **🧪 Enhanced Test Coverage:** - Test memory cleanup after timeout (no lingering handlers) - Test commit semantics (awaitMatch waits for up-to-date) - Test configurable void timeout functionality - All edge cases now properly covered **📦 Version Bump:** - Changeset updated to minor (removed exported error classes) All feedback addressed while maintaining backward compatibility. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Based on engineering feedback, this commit addresses several critical edge cases: **Memory Safety & Error Handling:** - Fix timeout cleanup memory leak in awaitMatch - pending matchers now properly removed on timeout - Add try/catch around matchFn calls to prevent user code from crashing stream loop - Add proper abort semantics with StreamAbortedError for pending matches - Add TimeoutWaitingForMatchError following codebase error class conventions **Race Condition Fix:** - Implement up-to-date bounded message buffer to handle race where messages arrive before matcher registration - Buffer is safely bounded to current transaction batch, eliminating stale data matching risks - Messages cleared on each up-to-date to maintain transaction boundaries **Test Reliability:** - Replace timing-based assertions with fake timers using vi.runOnlyPendingTimersAsync() - Eliminates CI flakiness while testing the same void strategy functionality **Cross-platform Compatibility:** - Confirmed ReturnType<typeof setTimeout> usage for browser compatibility - API shape consistency already matches runtime behavior The core matching strategy design (txid/custom/void) remains unchanged - these are lifecycle polish fixes for production readiness. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Resolved merge conflicts in electric.ts by combining: - Custom match function support from match-stream branch - Snapshot support from origin/main Both features are now integrated together. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The interface was extending BaseCollectionConfig with a strict handler return type of { txid: ... }, but our new matching strategies support broader return types including matchFn and void strategies. Removed the extends constraint and manually included needed properties to allow handlers to return any MatchingStrategy type. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
*/ | ||
id?: string | ||
schema?: TSchema | ||
getKey: CollectionConfig<T, string | number, TSchema>[`getKey`] |
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.
Most of these properties and methods (getKey
, onInsert
, etc.) are defined in the BaseCollectionConfig
. Let's not introduce them here but extend the BaseCollectionConfig
class as it used to do and only add Electric-specific things that are not part of the base config.
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.
This PR originally predates the refactor when BaseCollectionConfig
was introduced - all these props were here and modified in the first commit to this PR. I think Claude has not caught that context when addressing the merge conflict when merging in main.
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.
Yeah i was expecting that it had rebased correctly. Anyways, it will have to do that :)
directOpTransaction.commit().catch(() => undefined) | ||
|
||
// Add the transaction to the collection's transactions store | ||
state.transactions.set(directOpTransaction.id, directOpTransaction) |
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.
If the commit fails, do we still want to store this TX ID in the state.transactions
?
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.
Other than the comment on removing the duplicate interface we needs addressing before being merged, all look ok.
Question I do have is on why have two different methods for awaiting a match. We have this returned object with a txid or match function, as well as the utility functions to do the same. Could we standardise on just one way to do this?
The advantages of the unity methods is that you can compose multiple together, awaiting both/either a txid or another match, where with the returned value you can't. If a mutation functions calls multiple backend apis that all do inserts/udates, there could be multiple txids to await, the utility functions handle that well.
I think it also makes it conceptional easer to understand, your mutation function returns when the transaction is over, rather than it returning a value that tells the system when the transaction is over.
I'm approving as I know you are keen to get this out, but wanted to flag those thoughts.
Each handler can return: | ||
- `{ txid: number | number[] }` - Txid strategy (recommended) | ||
- `{ matchFn: (message) => boolean, timeout?: number }` - Custom match function strategy | ||
- `{}` - Void strategy (3-second timeout) |
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.
Could we let a user return the timeout on the void strategy?
- `{}` - Void strategy (3-second timeout) | |
- `{ timeout?: number }` - Void strategy (default 3-second timeout) |
await awaitTxId(txid) | ||
// Check against current batch messages first to handle race conditions | ||
for (const message of currentBatchMessages.state) { | ||
if (checkMatch(message)) { |
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.
Why is checkMatch
looking up the match and setting match.matched
to true
if we are also doing that here on L465? Isn't L465 going to override whatever we set on L449?
if (`txid` in result) { | ||
// Handle both single txid and array of txids | ||
if (Array.isArray(result.txid)) { | ||
await Promise.all(result.txid.map((id) => awaitTxId(id))) |
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.
nit: we can shorten result.txid.map((id) => awaitTxId(id))
to result.txid.map(awaitTxId)
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.
Good work, this is almost ready to be merged. We just need to fix the rebase that went a bit wrong (i.e. use the BaseCollectionConfig
to inherit common properties) and extract some of the logic to helper functions.
|
||
// Check pending matches against this message | ||
// Note: matchFn will mark matches internally, we don't resolve here | ||
const matchesToRemove: Array<string> = [] |
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.
It's worth extracting this logic to clean up matches into a helper function and call the helper function here in order to keep the code readable and concise.
}) | ||
|
||
// Resolve all matched pending matches on up-to-date | ||
const matchesToResolve: Array<string> = [] |
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.
It's worth also extracting this logic to clean up matches into a helper function and call the helper function here in order to keep the code readable and concise.
Summary
Implements GitHub issue #402 by adding flexible matching strategies for electric-db-collection synchronization.
• Txid Strategy (existing, backward compatible) - Uses PostgreSQL transaction IDs for precise matching
• Custom Match Function Strategy (new) - Allows heuristic-based matching with custom logic
• Void/Timeout Strategy (new, 3-second default) - Simple timeout for prototyping
Key Features
MatchFunction<T>
,MatchingStrategy<T>
ElectricCollectionConfig
to support all strategiesawaitMatch(matchFn, timeout?)
isChangeMessage
andisControlMessage
helpers for custom match functionsBenefits
Test plan
🤖 Generated with Claude Code