Jordan/fix group by getter

src/reactor.js

@@ -195,29 +195,19 @@ class Reactor {
       return
     }
 
-    let observerIdsToNotify = Immutable.Set().withMutations(set => {
+    let gettersToNotify = Immutable.Set().withMutations(set => {
       // notify all observers
       set.union(this.observerState.get('any'))
 
       dirtyStores.forEach(id => {
         const entries = this.observerState.getIn(['stores', id])
-        if (!entries) {
-          return
+        if (entries) {
+          set.union(entries)
         }
-        set.union(entries)
       })
     })
 
-    observerIdsToNotify.forEach((observerId) => {
-      const entry = this.observerState.getIn(['observersMap', observerId])
-      if (!entry) {
-        // don't notify here in the case a handler called unobserve on another observer
-        return
-      }
-
-      const getter = entry.get('getter')
-      const handler = entry.get('handler')
-
+    gettersToNotify.forEach(getter => {
       const prevEvaluateResult = fns.evaluate(this.prevReactorState, getter)
       const currEvaluateResult = fns.evaluate(this.reactorState, getter)
 
@@ -228,7 +218,14 @@ class Reactor {
       const currValue = currEvaluateResult.result
 
       if (!Immutable.is(prevValue, currValue)) {
-        handler.call(null, currValue)
+        const observerIds = this.observerState.getIn(['gettersMap', getter], [])
+        observerIds.forEach(observerId => {
+          const handler = this.observerState.getIn(['observersMap', observerId, 'handler'])
+          // don't notify here in the case a handler called unobserve on another observer
+          if (handler) {
+            handler.call(null, currValue)
+          }
+        })
       }
     })
 

src/reactor/fns.js

@@ -171,18 +171,19 @@ exports.addObserver = function(observerState, getter, handler) {
     handler: handler,
   })
 
-  let updatedObserverState
+  let updatedObserverState = observerState.updateIn(['gettersMap', getter], observerIds => {
+    return observerIds ? observerIds.add(currId) : Immutable.Set.of(currId)
+  })
+
   if (storeDeps.size === 0) {
     // no storeDeps means the observer is dependent on any of the state changing
-    updatedObserverState = observerState.update('any', observerIds => observerIds.add(currId))
+    updatedObserverState = updatedObserverState.updateIn(['any'], getters => getters.add(getter))
   } else {
-    updatedObserverState = observerState.withMutations(map => {
+    updatedObserverState = updatedObserverState.withMutations(map => {
       storeDeps.forEach(storeId => {
-        let path = ['stores', storeId]
-        if (!map.hasIn(path)) {
-          map.setIn(path, Immutable.Set([]))
-        }
-        map.updateIn(['stores', storeId], observerIds => observerIds.add(currId))
+        map.updateIn(['stores', storeId], getters => {
+          return getters ? getters.add(getter) : Immutable.Set.of(getter)
+        })
       })
     })
   }
@@ -195,6 +196,7 @@ exports.addObserver = function(observerState, getter, handler) {
     observerState: updatedObserverState,
     entry: entry,
   }
+
 }
 
 /**
@@ -240,20 +242,34 @@ exports.removeObserver = function(observerState, getter, handler) {
 exports.removeObserverByEntry = function(observerState, entry) {
   return observerState.withMutations(map => {
     const id = entry.get('id')
+    const getter = entry.get('getter')
     const storeDeps = entry.get('storeDeps')
 
-    if (storeDeps.size === 0) {
-      map.update('any', anyObsevers => anyObsevers.remove(id))
-    } else {
-      storeDeps.forEach(storeId => {
-        map.updateIn(['stores', storeId], observers => {
-          if (observers) {
-            // check for observers being present because reactor.reset() can be called before an unwatch fn
-            return observers.remove(id)
-          }
-          return observers
+    const observerIds = map.getIn(['gettersMap', getter])
+    if (!observerIds) {
+      // getter doesn't exist if reactor.reset() is called before the unwatchFn()
+      return
+    }
+    const updatedObserverIds = observerIds.remove(id)
+    map.setIn(['gettersMap', getter], updatedObserverIds)
+
+    if (updatedObserverIds.size === 0) {
+      // all observers have been removed for this getter, remove other entries
+      if (storeDeps.size === 0) {
+        // no storeDeps means the observer is dependent on any of the state changing
+        map.update('any', getters => getters.remove(getter));
+      } else {
+        storeDeps.forEach(storeId => {
+          map.updateIn(['stores', storeId], getters => {
+            if (getters) {
+              // check to make sure the getters Set exists for this store,
+              // in the case of reactor.reset() is called before the unwatchFn()
+              return getters.remove(getter)
+            }
+            return getters
+          })
         })
-      })
+      }
     }
 
     map.removeIn(['observersMap', id])

src/reactor/records.js

@@ -12,11 +12,13 @@ const ReactorState = Immutable.Record({
 })
 
 const ObserverState = Immutable.Record({
-  // observers registered to any store change
+  // getters registered to any store change
   any: Immutable.Set([]),
-  // observers registered to specific store changes
+  // getters registered to specific store changes
   stores: Immutable.Map({}),
 
+  gettersMap: Immutable.Map({}),
+
   observersMap: Immutable.Map({}),
 
   nextId: 1,

tests/reactor-fns-tests.js

@@ -321,12 +321,12 @@ describe('reactor fns', () => {
         entry = result.entry
 
       })
-      it('should update the "any" observers', () => {
-        const expected = Set.of(1)
+      it('should update the "any" with getter reference', () => {
+        const expected = Set.of(getter)
         const result = nextObserverState.get('any')
         expect(is(expected, result)).toBe(true)
       })
-      it('should not update the "store" observers', () => {
+      it('should not update the "store" with getter reference', () => {
         const expected = Map({})
         const result = nextObserverState.get('stores')
         expect(is(expected, result)).toBe(true)
@@ -336,6 +336,11 @@ describe('reactor fns', () => {
         const result = nextObserverState.get('nextId')
         expect(is(expected, result)).toBe(true)
       })
+      it('should update the gettersMap with getter as ref, id as value', () => {
+        const expected = Set.of(1)
+        const result = nextObserverState.getIn(['gettersMap', getter])
+        expect(is(expected, result)).toBe(true)
+      })
       it('should update the observerMap', () => {
         const expected = Map([
           [1, Map({
@@ -375,20 +380,25 @@ describe('reactor fns', () => {
         nextObserverState = result.observerState
         entry = result.entry
       })
-      it('should not update the "any" observers', () => {
+      it('should not update the "any" getters', () => {
         const expected = Set.of()
         const result = nextObserverState.get('any')
         expect(is(expected, result)).toBe(true)
       })
-      it('should not update the "store" observers', () => {
+      it('should update the "store" with getter reference', () => {
         const expected = Map({
-          store1: Set.of(1),
-          store2: Set.of(1),
+          store1: Set.of(getter),
+          store2: Set.of(getter),
         })
 
         const result = nextObserverState.get('stores')
         expect(is(expected, result)).toBe(true)
       })
+      it('should update the gettersMap with getter as ref, id as value', () => {
+        const expected = Set.of(1)
+        const result = nextObserverState.getIn(['gettersMap', getter])
+        expect(is(expected, result)).toBe(true)
+      })
       it('should increment the nextId', () => {
         const expected = 2
         const result = nextObserverState.get('nextId')
@@ -448,12 +458,16 @@ describe('reactor fns', () => {
       it('should return a new ObserverState with all entries containing the getter removed', () => {
         nextObserverState = fns.removeObserver(initialObserverState, getter1)
         const expected = Map({
-          any: Set.of(3),
+          any: Set.of(getter2),
           stores: Map({
             store1: Set(),
             store2: Set(),
           }),
           nextId: 4,
+          gettersMap: Map([
+            [getter1, Set()],
+            [getter2, Set.of(3)]
+          ]),
           observersMap: Map([
             [3, Map({
               id: 3,
@@ -475,10 +489,14 @@ describe('reactor fns', () => {
         const expected = Map({
           any: Set(),
           stores: Map({
-            store1: Set.of(1, 2),
-            store2: Set.of(1, 2),
+            store1: Set.of(getter1),
+            store2: Set.of(getter1),
           }),
           nextId: 4,
+          gettersMap: Map([
+            [getter1, Set.of(1, 2)],
+            [getter2, Set()]
+          ]),
           observersMap: Map([
             [1, Map({
               id: 1,
@@ -500,6 +518,40 @@ describe('reactor fns', () => {
         expect(is(expected, result)).toBe(true)
       })
     })
+
+    it('should not remove the getter reference in store when there is still listeners for the getter', () => {
+      nextObserverState = fns.removeObserver(initialObserverState, getter1, handler2)
+      const expected = Map({
+        any: Set.of(getter2),
+        stores: Map({
+          store1: Set.of(getter1),
+          store2: Set.of(getter1),
+        }),
+        nextId: 4,
+        gettersMap: Map([
+          [getter1, Set.of(1)],
+          [getter2, Set.of(3)]
+        ]),
+        observersMap: Map([
+          [1, Map({
+            id: 1,
+            storeDeps: Set.of('store1', 'store2'),
+            getterKey: getter1,
+            getter: getter1,
+            handler: handler1,
+          })],
+          [3, Map({
+            id: 3,
+            storeDeps: Set(),
+            getterKey: getter2,
+            getter: getter2,
+            handler: handler3,
+          })]
+        ])
+      })
+      const result = nextObserverState
+      expect(is(expected, result)).toBe(true)
+    })
   })
 })
 /*eslint-enable one-var, comma-dangle*/