Skip to content

Commit 970616b

Browse files
KyleAMathewsclaude
andauthored
fix(collection): fire status:change event before cleaning up event handlers (#714)
* fix(collection): fire status:change event before cleaning up event handlers Event handlers are now cleaned up after the status is changed to 'cleaned-up', allowing status:change listeners to properly detect the cleaned-up state. The cleanup process now: 1. Cleans up sync, state, changes, and indexes 2. Sets status to 'cleaned-up' (fires the event) 3. Finally cleans up event handlers This fixes the collection factory pattern where collections listen for the 'cleaned-up' status to remove themselves from the cache. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * style: format changeset with prettier 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d8ef559 commit 970616b

File tree

3 files changed

+88
-3
lines changed

3 files changed

+88
-3
lines changed
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@tanstack/db": patch
3+
---
4+
5+
Fix collection cleanup to fire status:change event with 'cleaned-up' status
6+
7+
Previously, when a collection was garbage collected, event handlers were removed before the status was changed to 'cleaned-up'. This prevented listeners from receiving the status:change event, breaking the collection factory pattern where collections listen for cleanup to remove themselves from a cache.
8+
9+
Now, the cleanup process:
10+
11+
1. Cleans up sync, state, changes, and indexes
12+
2. Sets status to 'cleaned-up' (fires the event)
13+
3. Finally cleans up event handlers
14+
15+
This enables the collection factory pattern:
16+
17+
```typescript
18+
const cache = new Map<string, ReturnType<typeof createCollection>>()
19+
20+
const getTodoCollection = (id: string) => {
21+
if (!cache.has(id)) {
22+
const collection = createCollection(/* ... */)
23+
24+
collection.on("status:change", ({ status }) => {
25+
if (status === "cleaned-up") {
26+
cache.delete(id) // This now works!
27+
}
28+
})
29+
30+
cache.set(id, collection)
31+
}
32+
return cache.get(id)!
33+
}
34+
```

packages/db/src/collection/lifecycle.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -250,8 +250,7 @@ export class CollectionLifecycleManager<
250250
!deadline || deadline.timeRemaining() > 0 || deadline.didTimeout
251251

252252
if (hasTime) {
253-
// Perform all cleanup operations
254-
this.events.cleanup()
253+
// Perform all cleanup operations except events
255254
this.sync.cleanup()
256255
this.state.cleanup()
257256
this.changes.cleanup()
@@ -265,8 +264,13 @@ export class CollectionLifecycleManager<
265264
this.hasBeenReady = false
266265
this.onFirstReadyCallbacks = []
267266

268-
// Set status to cleaned-up
267+
// Set status to cleaned-up after everything is cleaned up
268+
// This fires the status:change event to notify listeners
269269
this.setStatus(`cleaned-up`)
270+
271+
// Finally, cleanup event handlers after the event has been fired
272+
this.events.cleanup()
273+
270274
return true
271275
} else {
272276
// If we don't have time, reschedule for the next idle period

packages/db/tests/collection-lifecycle.test.ts

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -474,5 +474,52 @@ describe(`Collection Lifecycle Management`, () => {
474474

475475
subscription.unsubscribe()
476476
})
477+
478+
it(`should fire status:change event with 'cleaned-up' status before clearing event handlers`, () => {
479+
const collection = createCollection<{ id: string; name: string }>({
480+
id: `cleanup-event-test`,
481+
getKey: (item) => item.id,
482+
gcTime: 1000,
483+
sync: {
484+
sync: () => {},
485+
},
486+
})
487+
488+
// Track status changes
489+
const statusChanges: Array<{ status: string; previousStatus: string }> =
490+
[]
491+
492+
// Add event listener for status changes
493+
collection.on(`status:change`, ({ status, previousStatus }) => {
494+
statusChanges.push({ status, previousStatus })
495+
})
496+
497+
// Subscribe and unsubscribe to trigger GC
498+
const subscription = collection.subscribeChanges(() => {})
499+
subscription.unsubscribe()
500+
501+
expect(statusChanges).toHaveLength(1)
502+
expect(statusChanges[0]).toEqual({
503+
status: `loading`,
504+
previousStatus: `idle`,
505+
})
506+
507+
// Trigger GC timeout to schedule cleanup
508+
const gcTimerId = mockSetTimeout.mock.results[0]?.value
509+
if (gcTimerId) {
510+
triggerTimeout(gcTimerId)
511+
}
512+
513+
// Trigger all remaining timeouts to handle the idle callback
514+
triggerAllTimeouts()
515+
516+
// Verify that the listener received the 'cleaned-up' status change event
517+
expect(statusChanges).toHaveLength(2)
518+
expect(statusChanges[1]).toEqual({
519+
status: `cleaned-up`,
520+
previousStatus: `loading`,
521+
})
522+
expect(collection.status).toBe(`cleaned-up`)
523+
})
477524
})
478525
})

0 commit comments

Comments
 (0)