Repository: maxnowack/signaldb
Pull request #1299 (2025-01-13 09:58:23)

fix: wait until collections persistence was initialized before sync

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/maxnowack/signaldb/pull/1299?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) Report
Attention: Patch coverage is `69.23077%` with `4 lines` in your changes missing coverage. Please review.
> Project coverage is 99.76%. Comparing base [(`c637976`)](https://app.codecov.io/gh/maxnowack/signaldb/commit/c637976cb9319031d7ab8060d13700aa0fe42e16?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) to head [(`570839e`)](https://app.codecov.io/gh/maxnowack/signaldb/commit/570839ea6ca2a228de6817f1dc56ec3e7969a83a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack).

| [Files with missing lines](https://app.codecov.io/gh/maxnowack/signaldb/pull/1299?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) | Patch % | Lines |
|---|---|---|
| [packages/base/core/src/Collection/index.ts](https://app.codecov.io/gh/maxnowack/signaldb/pull/1299?src=pr&el=tree&filepath=packages%2Fbase%2Fcore%2Fsrc%2FCollection%2Findex.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack#diff-cGFja2FnZXMvYmFzZS9jb3JlL3NyYy9Db2xsZWN0aW9uL2luZGV4LnRz) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/maxnowack/signaldb/pull/1299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) |
| [packages/base/sync/src/SyncManager.ts](https://app.codecov.io/gh/maxnowack/signaldb/pull/1299?src=pr&el=tree&filepath=packages%2Fbase%2Fsync%2Fsrc%2FSyncManager.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack#diff-cGFja2FnZXMvYmFzZS9zeW5jL3NyYy9TeW5jTWFuYWdlci50cw==) | 81.81% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/maxnowack/signaldb/pull/1299?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) |

<details><summary>Additional details and impacted files</summary>


```diff
@@             Coverage Diff             @@
##              main    #1299      +/-   ##
===========================================
- Coverage   100.00%   99.76%   -0.24%     
===========================================
  Files           56       56              
  Lines         1700     1712      +12     
  Branches       376      378       +2     
===========================================
+ Hits          1700     1708       +8     
- Misses           0        3       +3     
- Partials         0        1       +1     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/maxnowack/signaldb/pull/1299?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack).


Patch:
diff --git a/packages/base/core/CHANGELOG.md b/packages/base/core/CHANGELOG.md
index f5e6d5c..1a97ff9 100644
--- a/packages/base/core/CHANGELOG.md
+++ b/packages/base/core/CHANGELOG.md
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+* `hasPersistence` method on `Collection` to check if a collection has persistence
+
 ### Changed
 
 * Don't load developer tools automatically in production
diff --git a/packages/base/core/src/Collection/index.ts b/packages/base/core/src/Collection/index.ts
index f649a16..ac923ea 100644
--- a/packages/base/core/src/Collection/index.ts
+++ b/packages/base/core/src/Collection/index.ts
@@ -394,6 +394,14 @@ export default class Collection<
     Collection.onCreationCallbacks.forEach(callback => callback(this))
   }
 
+  /**
+   * Checks whether the collection is persisted.
+   * @returns A boolean indicating if the collection is persisted.
+   */
+  public hasPersistence() {
+    return !!this.persistenceAdapter
+  }
+
   /**
    * Checks whether the collection is currently performing a pull operation
    * ⚡️ this function is reactive!
diff --git a/packages/base/sync/CHANGELOG.md b/packages/base/sync/CHANGELOG.md
index 8f85569..cbc97b3 100644
--- a/packages/base/sync/CHANGELOG.md
+++ b/packages/base/sync/CHANGELOG.md
@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+* Wait until collections persistence was initialized before starting sync
+
 ## [1.0.1] - 2025-01-10
 
 ### Changed
diff --git a/packages/base/sync/src/SyncManager.ts b/packages/base/sync/src/SyncManager.ts
index 524645c..61432d1 100644
--- a/packages/base/sync/src/SyncManager.ts
+++ b/packages/base/sync/src/SyncManager.ts
@@ -82,6 +82,7 @@ export default class SyncManager<
   protected collections: Map<string, [
     Collection<ItemType, IdType, any>,
     SyncOptions<CollectionOptions>,
+    readyPromise: Promise<void>,
   ]> = new Map()
 
   protected changes: Collection<Change<ItemType>, string>
@@ -222,6 +223,19 @@ export default class SyncManager<
     options: SyncOptions<CollectionOptions>,
   ) {
     if (this.isDisposed) throw new Error('SyncManager is disposed')
+
+    const readyPromise = new Promise<void>((resolve, reject) => {
+      if (!collection.hasPersistence()) {
+        resolve()
+        return
+      }
+      collection.once('persistence.error', (error) => {
+        if (this.options.onError) this.options.onError(options, error)
+        reject(error)
+      })
+      collection.once('persistence.init', resolve)
+    })
+
     if (this.options.registerRemoteChange) {
       this.options.registerRemoteChange(options, async (data) => {
         if (data == null) {
@@ -262,7 +276,7 @@ export default class SyncManager<
       })
     }
 
-    this.collections.set(options.name, [collection, options])
+    this.collections.set(options.name, [collection, options, readyPromise])
 
     const hasRemoteChange = (change: Omit<Change, 'id' | 'time'>) => {
       for (const remoteChange of this.remoteChanges) {
@@ -379,6 +393,8 @@ export default class SyncManager<
     await this.isReady()
     const entry = this.getCollection(name)
     const collectionOptions = entry[1]
+    const readyPromise = entry[2]
+    await readyPromise
 
     const hasActiveSyncs = this.syncOperations.find({
       collectionName: name,


Test patch:
diff --git a/packages/base/core/__tests__/Collection.spec.ts b/packages/base/core/__tests__/Collection.spec.ts
index aa21c3b..987b2bd 100644
--- a/packages/base/core/__tests__/Collection.spec.ts
+++ b/packages/base/core/__tests__/Collection.spec.ts
@@ -1,6 +1,7 @@
 import { vi, beforeEach, describe, it, expect } from 'vitest'
 import { Collection, createMemoryAdapter, createIndex } from '../src'
 import waitForEvent from './helpers/waitForEvent'
+import memoryPersistenceAdapter from './helpers/memoryPersistenceAdapter'
 
 const measureTime = (fn: () => void) => {
   const start = performance.now()
@@ -717,5 +718,15 @@ describe('Collection', () => {
         expect(col.find({ id: '2' }).fetch()).toEqual([{ id: '2', name: 'Jane' }])
       })
     })
+
+    it('should return if a collection is being persisted or not', () => {
+      const col1 = new Collection<{ id: string, name: string }>()
+      expect(col1.hasPersistence()).toBe(false)
+
+      const col2 = new Collection<{ id: string, name: string }>({
+        persistence: memoryPersistenceAdapter(),
+      })
+      expect(col2.hasPersistence()).toBe(true)
+    })
   })
 })
diff --git a/packages/base/sync/__tests__/SyncManager.spec.ts b/packages/base/sync/__tests__/SyncManager.spec.ts
index 537abd3..3baf558 100644
--- a/packages/base/sync/__tests__/SyncManager.spec.ts
+++ b/packages/base/sync/__tests__/SyncManager.spec.ts
@@ -1013,3 +1013,143 @@ it('should reset if syncmanager snapshot and collection are not in sync', async
   // @ts-expect-error - private property
   expect(syncManager.remoteChanges.length).toBe(0)
 })
+
+it('should start sync after internal collections are ready', async () => {
+  const persistenceAdapter = memoryPersistenceAdapter([], undefined, 100)
+  const mockPersistenceAdapter = createPersistenceAdapter({
+    register: vi.fn(persistenceAdapter.register),
+    load: vi.fn(persistenceAdapter.load),
+    save: vi.fn(persistenceAdapter.save),
+  })
+  const mockPull = vi.fn<() => Promise<LoadResponse<TestItem>>>().mockResolvedValue({
+    items: [{ id: '1', name: 'Test Item' }],
+  })
+
+  const mockPush = vi.fn<(options: any, pushParameters: any) => Promise<void>>()
+    .mockResolvedValue()
+
+  const syncManager = new SyncManager({
+    persistenceAdapter: () => mockPersistenceAdapter,
+    pull: mockPull,
+    push: mockPush,
+  })
+
+  let persistenceInitialized = false
+  void Promise.all([
+    new Promise((resolve) => {
+      // @ts-expect-error - private property
+      syncManager.syncOperations.once('persistence.init', resolve)
+    }),
+    new Promise((resolve) => {
+      // @ts-expect-error - private property
+      syncManager.changes.once('persistence.init', resolve)
+    }),
+    new Promise((resolve) => {
+      // @ts-expect-error - private property
+      syncManager.snapshots.once('persistence.init', resolve)
+    }),
+  ]).then(() => {
+    persistenceInitialized = true
+  })
+
+  const collection = new Collection<TestItem, string, any>()
+  syncManager.addCollection(collection, { name: 'test' })
+
+  expect(mockPersistenceAdapter.load).not.toBeCalled()
+  expect(mockPull).not.toBeCalled()
+  expect(persistenceInitialized).toBeFalsy()
+  await syncManager.sync('test')
+
+  expect(mockPull).toBeCalled()
+  expect(mockPersistenceAdapter.load).toHaveBeenCalledBefore(mockPull)
+  expect(persistenceInitialized).toBeTruthy()
+})
+
+it('should start sync after collection is ready', async () => {
+  const persistenceAdapter = memoryPersistenceAdapter([], undefined, 100)
+  const mockPersistenceAdapter = createPersistenceAdapter({
+    register: vi.fn(persistenceAdapter.register),
+    load: vi.fn(persistenceAdapter.load),
+    save: vi.fn(persistenceAdapter.save),
+  })
+  const mockPull = vi.fn<() => Promise<LoadResponse<TestItem>>>().mockResolvedValue({
+    items: [{ id: '1', name: 'Test Item' }],
+  })
+
+  const mockPush = vi.fn<(options: any, pushParameters: any) => Promise<void>>()
+    .mockResolvedValue()
+
+  const syncManager = new SyncManager({
+    pull: mockPull,
+    push: mockPush,
+  })
+
+  const collection = new Collection<TestItem, string, any>({
+    persistence: mockPersistenceAdapter,
+  })
+  let persistenceInitialized = false
+  void new Promise<void>((resolve) => {
+    collection.once('persistence.init', resolve)
+  }).then(() => {
+    persistenceInitialized = true
+  })
+
+  syncManager.addCollection(collection, { name: 'test' })
+
+  expect(mockPull).not.toBeCalled()
+  expect(mockPersistenceAdapter.load).not.toBeCalled()
+  expect(persistenceInitialized).toBeFalsy()
+  await syncManager.sync('test')
+
+  expect(mockPull).toBeCalled()
+  expect(mockPersistenceAdapter.load).toHaveBeenCalledBefore(mockPull)
+  expect(persistenceInitialized).toBeTruthy()
+})
+
+it('should fail if there was a persistence error during initialization', async () => {
+  const persistenceAdapter = memoryPersistenceAdapter([], undefined, 100)
+  const mockPersistenceAdapter = createPersistenceAdapter({
+    register: vi.fn(persistenceAdapter.register),
+    load: vi.fn(() => Promise.reject(new Error('Persistence error'))),
+    save: vi.fn(persistenceAdapter.save),
+  })
+  const mockPull = vi.fn<() => Promise<LoadResponse<TestItem>>>().mockResolvedValue({
+    items: [{ id: '1', name: 'Test Item' }],
+  })
+
+  const mockPush = vi.fn<(options: any, pushParameters: any) => Promise<void>>()
+    .mockResolvedValue()
+
+  const syncManager = new SyncManager({
+    pull: mockPull,
+    push: mockPush,
+  })
+
+  const collection = new Collection<TestItem, string, any>({
+    persistence: mockPersistenceAdapter,
+  })
+  let persistenceInitialized = false
+  void new Promise<void>((resolve) => {
+    collection.once('persistence.init', resolve)
+  }).then(() => {
+    persistenceInitialized = true
+  })
+  let persistenceError = false
+  void new Promise<void>((resolve) => {
+    collection.once('persistence.error', () => resolve())
+  }).then(() => {
+    persistenceError = true
+  })
+
+  syncManager.addCollection(collection, { name: 'test' })
+
+  expect(mockPull).not.toBeCalled()
+  expect(persistenceInitialized).toBeFalsy()
+  expect(persistenceError).toBeFalsy()
+
+  await expect(syncManager.sync('test')).rejects.toThrowError('Persistence error')
+
+  expect(mockPull).not.toBeCalled()
+  expect(persistenceInitialized).toBeFalsy()
+  expect(persistenceError).toBeTruthy()
+})