Repository: maxnowack/signaldb
Pull request #177 (2023-09-20 09:40:30)

refactor: improve registering of event listeners from observer

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/maxnowack/signaldb/pull/177?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) Report
> Merging [#177](https://app.codecov.io/gh/maxnowack/signaldb/pull/177?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) (7ef831b) into [main](https://app.codecov.io/gh/maxnowack/signaldb/commit/7cbd88c500c2c77ed26993e3d3fd9c58cf943457?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) (7cbd88c) will **increase** coverage by `0.08%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #177      +/-   ##
==========================================
+ Coverage   97.10%   97.18%   +0.08%     
==========================================
  Files          20       20              
  Lines         725      746      +21     
  Branches      183      189       +6     
==========================================
+ Hits          704      725      +21     
  Misses         21       21              
```


| [Files Changed](https://app.codecov.io/gh/maxnowack/signaldb/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack) | Coverage Δ | |
|---|---|---|
| [src/Collection/Cursor.ts](https://app.codecov.io/gh/maxnowack/signaldb/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack#diff-c3JjL0NvbGxlY3Rpb24vQ3Vyc29yLnRz) | `100.00% <100.00%> (ø)` | |
| [src/Collection/Observer.ts](https://app.codecov.io/gh/maxnowack/signaldb/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack#diff-c3JjL0NvbGxlY3Rpb24vT2JzZXJ2ZXIudHM=) | `100.00% <100.00%> (ø)` | |
| [src/Collection/index.ts](https://app.codecov.io/gh/maxnowack/signaldb/pull/177?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Max+Nowack#diff-c3JjL0NvbGxlY3Rpb24vaW5kZXgudHM=) | `98.23% <100.00%> (-0.08%)` | :arrow_down: |



Patch:
diff --git a/src/Collection/Cursor.ts b/src/Collection/Cursor.ts
index ef7223f..08cb474 100644
--- a/src/Collection/Cursor.ts
+++ b/src/Collection/Cursor.ts
@@ -2,12 +2,20 @@ import type Selector from '../types/Selector'
 import sortItems from '../utils/sortItems'
 import project from '../utils/project'
 import match from '../utils/match'
+import type ReactivityAdapter from '../types/ReactivityAdapter'
 import type { BaseItem, FindOptions, Transform } from './types'
 import type { ObserveCallbacks } from './Observer'
 import Observer from './Observer'
 
+function isInReactiveScope(reactivity: ReactivityAdapter | undefined | false) {
+  if (!reactivity) return false // if reactivity is disabled we don't need to check
+  if (!reactivity.isInScope) return true // if reactivity is enabled and no isInScope method is provided we assume it is in scope
+  return reactivity.isInScope() // if reactivity is enabled and isInScope method is provided we check if it is in scope
+}
+
 export interface CursorOptions<T extends BaseItem, U = T> extends FindOptions<T> {
   transform?: Transform<T, U>,
+  bindEvents?: (requery: () => void) => () => void,
 }
 
 export default class Cursor<T extends BaseItem, U = T> {
@@ -57,6 +65,7 @@ export default class Cursor<T extends BaseItem, U = T> {
 
   private depend(changeEvents: { [P in keyof ObserveCallbacks<U>]?: true }) {
     if (!this.options.reactive) return
+    if (!isInReactiveScope(this.options.reactive)) return
     const signal = this.options.reactive.create()
     signal.depend()
     const notify = () => signal.notify()
@@ -134,11 +143,23 @@ export default class Cursor<T extends BaseItem, U = T> {
           },
         }
       }, {}) as ObserveCallbacks<T>
-    const observer = new Observer(transformedCallbacks, skipInitial)
+    const observer = new Observer(
+      transformedCallbacks,
+      () => {
+        const cleanup = this.options.bindEvents
+          && this.options.bindEvents(() => observer.check(this.getItems()))
+        return () => {
+          if (cleanup) cleanup()
+        }
+      },
+      skipInitial,
+    )
     this.observers.push(observer)
     observer.check(this.getItems())
+    this.onCleanup(() => observer.stop())
 
     return () => {
+      observer.stop()
       this.observers = this.observers.filter(o => o !== observer)
     }
   }
diff --git a/src/Collection/Observer.ts b/src/Collection/Observer.ts
index 85f724b..3d41685 100644
--- a/src/Collection/Observer.ts
+++ b/src/Collection/Observer.ts
@@ -13,10 +13,16 @@ export default class Observer<T extends { id: any }> {
   private callbacks: ObserveCallbacks<T>
   private skipInitial = false
   private isIinitial = true
+  private unbindEvents: () => void
 
-  constructor(callbacks: ObserveCallbacks<T>, skipInitial = false) {
+  constructor(
+    callbacks: ObserveCallbacks<T>,
+    bindEvents: () => () => void,
+    skipInitial = false,
+  ) {
     this.callbacks = callbacks
     this.skipInitial = skipInitial
+    this.unbindEvents = bindEvents()
   }
 
   // eslint-disable-next-line max-len
@@ -77,4 +83,8 @@ export default class Observer<T extends { id: any }> {
     this.previousItems = newItems
     this.isIinitial = false
   }
+
+  public stop() {
+    this.unbindEvents()
+  }
 }
diff --git a/src/Collection/index.ts b/src/Collection/index.ts
index 64c8ce2..6d162a3 100644
--- a/src/Collection/index.ts
+++ b/src/Collection/index.ts
@@ -33,12 +33,6 @@ interface CollectionEvents<T> {
   'persistence.received': () => void,
 }
 
-function isInReactiveScope(reactivity: ReactivityAdapter | undefined | false) {
-  if (!reactivity) return false // if reactivity is disabled we don't need to check
-  if (!reactivity.isInScope) return true // if reactivity is enabled and no isInScope method is provided we assume it is in scope
-  return reactivity.isInScope() // if reactivity is enabled and isInScope method is provided we check if it is in scope
-}
-
 // eslint-disable-next-line max-len
 export default class Collection<T extends BaseItem<I> = BaseItem, I = any, U = T> extends EventEmitter<CollectionEvents<T>> {
   private options: CollectionOptions<T, I, U>
@@ -135,27 +129,23 @@ export default class Collection<T extends BaseItem<I> = BaseItem, I = any, U = T
 
   public find<O extends FindOptions<T>>(selector?: Selector<T>, options?: O) {
     if (selector !== undefined && (!selector || typeof selector !== 'object')) throw new Error('Invalid selector')
-    const cursorOptions = {
+    return new Cursor<T, U>(() => this.getItems(), selector || {}, {
       reactive: this.options.reactivity,
       ...options,
       transform: this.transform.bind(this),
-    }
-    const cursor = new Cursor<T, U>(() => this.getItems(), selector || {}, cursorOptions)
-    if (isInReactiveScope(cursorOptions.reactive)) {
-      const requery = () => cursor.requery()
-      this.addListener('persistence.received', requery)
-      this.addListener('added', requery)
-      this.addListener('changed', requery)
-      this.addListener('removed', requery)
-      const cleanup = () => {
-        this.removeListener('persistence.received', requery)
-        this.removeListener('added', requery)
-        this.removeListener('changed', requery)
-        this.removeListener('removed', requery)
-      }
-      cursor.onCleanup(cleanup)
-    }
-    return cursor
+      bindEvents: (requery) => {
+        this.addListener('persistence.received', requery)
+        this.addListener('added', requery)
+        this.addListener('changed', requery)
+        this.addListener('removed', requery)
+        return () => {
+          this.removeListener('persistence.received', requery)
+          this.removeListener('added', requery)
+          this.removeListener('changed', requery)
+          this.removeListener('removed', requery)
+        }
+      },
+    })
   }
 
   public findOne<O extends Omit<FindOptions<T>, 'limit'>>(selector: Selector<T>, options?: O) {


Test patch:
diff --git a/__tests__/Collection.spec.ts b/__tests__/Collection.spec.ts
index b2267f5..bd55b13 100644
--- a/__tests__/Collection.spec.ts
+++ b/__tests__/Collection.spec.ts
@@ -206,6 +206,46 @@ describe('Collection', () => {
 
   describe('events', () => {
     it('shouldn\'t register any event listeners without reactivity', () => {
+      const col = new Collection<{ id: string, name: string }>()
+      expect(col.listenerCount('added')).toBe(0)
+      expect(col.listenerCount('changed')).toBe(0)
+      expect(col.listenerCount('removed')).toBe(0)
+
+      col.insert({ id: '1', name: 'John' })
+      col.updateOne({ id: '1' }, { $set: { name: 'Jane' } })
+      col.removeOne({ id: '1' })
+
+      expect(col.listenerCount('added')).toBe(0)
+      expect(col.listenerCount('changed')).toBe(0)
+      expect(col.listenerCount('removed')).toBe(0)
+
+      const cursor = col.find()
+      expect(col.listenerCount('added')).toBe(0)
+      expect(col.listenerCount('changed')).toBe(0)
+      expect(col.listenerCount('removed')).toBe(0)
+
+      cursor.fetch()
+      expect(col.listenerCount('added')).toBe(0)
+      expect(col.listenerCount('changed')).toBe(0)
+      expect(col.listenerCount('removed')).toBe(0)
+
+      cursor.observeChanges({})
+      expect(col.listenerCount('added')).toBe(1)
+      expect(col.listenerCount('changed')).toBe(1)
+      expect(col.listenerCount('removed')).toBe(1)
+
+      cursor.cleanup()
+      expect(col.listenerCount('added')).toBe(0)
+      expect(col.listenerCount('changed')).toBe(0)
+      expect(col.listenerCount('removed')).toBe(0)
+
+      col.find({}, { reactive: false })
+      expect(col.listenerCount('added')).toBe(0)
+      expect(col.listenerCount('changed')).toBe(0)
+      expect(col.listenerCount('removed')).toBe(0)
+    })
+
+    it('should register event listeners in reactive scope', () => {
       const col = new Collection<{ id: string, name: string }>({
         reactivity: {
           create: () => ({
@@ -234,13 +274,36 @@ describe('Collection', () => {
       expect(col.listenerCount('removed')).toBe(0)
 
       const cursor = col.find()
+      expect(col.listenerCount('added')).toBe(0)
+      expect(col.listenerCount('changed')).toBe(0)
+      expect(col.listenerCount('removed')).toBe(0)
+
+      cursor.fetch()
       expect(col.listenerCount('added')).toBe(1)
       expect(col.listenerCount('changed')).toBe(1)
       expect(col.listenerCount('removed')).toBe(1)
 
-      cursor.fetch()
-      cursor.cleanup()
+      cursor.map(item => item)
+      expect(col.listenerCount('added')).toBe(2)
+      expect(col.listenerCount('changed')).toBe(2)
+      expect(col.listenerCount('removed')).toBe(2)
+
+      cursor.forEach(item => item)
+      expect(col.listenerCount('added')).toBe(3)
+      expect(col.listenerCount('changed')).toBe(3)
+      expect(col.listenerCount('removed')).toBe(3)
 
+      cursor.count()
+      expect(col.listenerCount('added')).toBe(4)
+      expect(col.listenerCount('changed')).toBe(4)
+      expect(col.listenerCount('removed')).toBe(4)
+
+      cursor.observeChanges({})
+      expect(col.listenerCount('added')).toBe(5)
+      expect(col.listenerCount('changed')).toBe(5)
+      expect(col.listenerCount('removed')).toBe(5)
+
+      cursor.cleanup()
       expect(col.listenerCount('added')).toBe(0)
       expect(col.listenerCount('changed')).toBe(0)
       expect(col.listenerCount('removed')).toBe(0)
diff --git a/__tests__/Cursor.spec.ts b/__tests__/Cursor.spec.ts
index ccf95a0..ab69561 100644
--- a/__tests__/Cursor.spec.ts
+++ b/__tests__/Cursor.spec.ts
@@ -292,9 +292,9 @@ describe('Cursor', () => {
       expect(callbacks.removed).not.toHaveBeenCalled()
     })
 
-    it('should call the appropriate callbacks when items are added, moved, changed, or removed', async () => {
-      const col = new Collection<TestItem>()
-      items.forEach(item => col.insert(item))
+    it('should call the appropriate callbacks when items are added, moved, changed, or removed', () => {
+      const col = new Collection<TestItem & { count: number }>()
+      items.forEach((item, index) => col.insert({ ...item, count: index }))
 
       const callbacks: ObserveCallbacks<TestItem> = {
         added: vi.fn(),
@@ -304,28 +304,35 @@ describe('Cursor', () => {
         removed: vi.fn(),
       }
 
-      const cursor = col.find()
+      const cursor = col.find({}, {
+        sort: { count: 1 },
+      })
       cursor.observeChanges(callbacks, true)
 
       // Change data
-      col.insert({ id: 4, name: 'item4' }) // Add new item
-      col.updateOne({ id: 1 }, { $set: { name: 'item1_modified' } }) // Modify existing item
-      col.removeOne({ id: 2 }) // Remove item
-
-      cursor.requery()
-
-      await wait() // Wait for all async operations to finish
-
+      col.insert({ id: 4, name: 'item4', count: 99 }) // Add new item
       expect(callbacks.added).toHaveBeenCalledWith(expect.objectContaining({ id: 4, name: 'item4' }))
       expect(callbacks.addedBefore).toHaveBeenCalledWith(
         expect.objectContaining({ id: 4, name: 'item4' }),
         null,
       )
+
+      col.updateOne({ id: 1 }, { $set: { name: 'item1_modified' } }) // Modify existing item
       expect(callbacks.changed).toHaveBeenCalledWith(expect.objectContaining({ id: 1, name: 'item1_modified' }))
+
+      col.updateOne({ id: 1 }, { $set: { count: 42 } }) // Move existing item
       expect(callbacks.movedBefore).toHaveBeenCalledWith(
-        expect.objectContaining({ id: 3, name: 'Item 3' }),
-        expect.objectContaining({ id: 4, name: 'item4' }),
+        expect.objectContaining({ id: 1 }),
+        expect.objectContaining({ id: 4 }),
       )
+
+      col.updateOne({ id: 2 }, { $set: { count: 999 } }) // Move existing item
+      expect(callbacks.movedBefore).toHaveBeenCalledWith(
+        expect.objectContaining({ id: 2 }),
+        null,
+      )
+
+      col.removeOne({ id: 2 }) // Remove item
       expect(callbacks.removed).toHaveBeenCalledWith(expect.objectContaining({ id: 2, name: 'Item 2' }))
     })