-
Notifications
You must be signed in to change notification settings - Fork 114
Implement collection lifecycle PRD proposals #198
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
Implement collection lifecycle PRD proposals #198
Conversation
🦋 Changeset detectedLatest commit: 2708e73 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: +1.49 kB (+5.48%) 🔍 Total Size: 28.7 kB
ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 822 B ℹ️ View Unchanged
|
- Resolved merge conflicts in collection.ts, compiled-query.ts, and types.ts - Combined lifecycle management features with compare functionality - Added startSyncImmediate method for compiled queries to override lazy loading - Preserved all lifecycle features: lazy loading, GC timers, status tracking - Maintained compatibility with new compare option for sorted collections - All 384 tests passing
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.
Bug: Stale Promise Cache and Memory Leak
The preloadPromise is not reset after it resolves or rejects, leading to stale promise caching. This prevents subsequent preload() calls from retrying or initiating a fresh preload, even after cleanup or error recovery. Additionally, collections are added to a global collectionsStore in the constructor but are never removed during cleanup(), causing a memory leak.
packages/db/src/collection.ts#L338-L407
db/packages/db/src/collection.ts
Lines 338 to 407 in 2133e11
| */ | |
| public preload(): Promise<void> { | |
| if (this.preloadPromise) { | |
| return this.preloadPromise | |
| } | |
| this.preloadPromise = new Promise<void>((resolve, reject) => { | |
| if (this._status === `ready`) { | |
| resolve() | |
| return | |
| } | |
| if (this._status === `error`) { | |
| reject(new Error(`Collection is in error state`)) | |
| return | |
| } | |
| // Start sync if collection was cleaned up | |
| if (this._status === `cleaned-up`) { | |
| try { | |
| this.startSync() | |
| } catch (error) { | |
| reject(error) | |
| return | |
| } | |
| } | |
| // Wait for first commit | |
| this.onFirstCommit(() => { | |
| resolve() | |
| }) | |
| }) | |
| return this.preloadPromise | |
| } | |
| /** | |
| * Clean up the collection by stopping sync and clearing data | |
| * This can be called manually or automatically by garbage collection | |
| */ | |
| public async cleanup(): Promise<void> { | |
| // Clear GC timeout | |
| if (this.gcTimeoutId) { | |
| clearTimeout(this.gcTimeoutId) | |
| this.gcTimeoutId = null | |
| } | |
| // Stop sync | |
| if (this.syncCleanupFn) { | |
| this.syncCleanupFn() | |
| this.syncCleanupFn = null | |
| } | |
| // Clear data | |
| this.syncedData.clear() | |
| this.syncedMetadata.clear() | |
| this.derivedUpserts.clear() | |
| this.derivedDeletes.clear() | |
| this._size = 0 | |
| this.pendingSyncedTransactions = [] | |
| this.syncedKeys.clear() | |
| this.hasReceivedFirstCommit = false | |
| this.onFirstCommitCallbacks = [] | |
| this.preloadPromise = null | |
| // Update status | |
| this._status = `cleaned-up` | |
| return Promise.resolve() | |
| } |
Bug: Sorting Fails for Valid Index Zero
The toArray getter's sorting logic incorrectly checks for the truthiness of _orderByIndex on the first array item. This prevents sorting when _orderByIndex is 0 (a valid index) or when the first item lacks the property but other items in the array have it and require sorting.
packages/db/src/collection.ts#L1373-L1378
db/packages/db/src/collection.ts
Lines 1373 to 1378 in 2133e11
| // should be done by the query engine. | |
| if (array[0] && (array[0] as { _orderByIndex?: number })._orderByIndex) { | |
| return (array as Array<{ _orderByIndex: number }>).sort( | |
| (a, b) => a._orderByIndex - b._orderByIndex | |
| ) as Array<T> | |
| } |
Bug: Collection Methods Fail to Remove Subscribers
Memory leak in Collection's asStoreMap() and asStoreArray() methods. Both methods call addSubscriber() but lack a corresponding removeSubscriber() call. This keeps activeSubscribersCount above zero, preventing the Collection instance from being garbage collected and rendering its lifecycle management (GC timer) ineffective.
packages/db/src/collection.ts#L1510-L1517
db/packages/db/src/collection.ts
Lines 1510 to 1517 in 2133e11
| // Start sync and track subscriber - this subscription will never be removed | |
| // as the store is kept alive for the lifetime of the collection | |
| this.addSubscriber() | |
| this.changeListeners.add(() => { | |
| this._storeMap!.setState(() => new Map(this.entries())) | |
| }) |
packages/db/src/collection.ts#L1535-L1542
db/packages/db/src/collection.ts
Lines 1535 to 1542 in 2133e11
| // as the store is kept alive for the lifetime of the collection | |
| this.addSubscriber() | |
| this.changeListeners.add(() => { | |
| this._storeArray!.setState(() => this.toArray) | |
| }) | |
| } | |
| return this._storeArray |
BugBot free trial expires on July 22, 2025
You have used $0.00 of your $50.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
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.
Looks great!
We need some tests & implementing cleanup in electric/query.
|
@KyleAMathews this is ready for another review, I have added back the tests that were accidentally deleted. The electric sync now returns the unsubscribe function for cleanup: db/packages/db-collections/src/electric.ts Lines 300 to 301 in d7cdf3d
And the query sync already did: db/packages/db-collections/src/query.ts Line 253 in d7cdf3d
|
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 work!
Co-authored-by: Cursor Agent <[email protected]>
Implements and closes #195
Adds automatic lifecycle management for collections to optimize resource usage.
New Features:
startSyncoption (defaults totrue, set tofalsefor lazy loading)gcTime(default 5 minutes) of inactivitypreload()andcleanup()methods for lifecycle controlUsage: