Merge pull request #153 from sysdea-libs/fix-skipping-observers-alt

Iterate over copy of observers for notifyObservers [Fixes #151]

Author: jordangarcia

src/change-observer.js

@@ -27,7 +27,10 @@ class ChangeObserver {
     if (this.__observers.length > 0) {
       var currentValues = Immutable.Map()
 
-      this.__observers.forEach(entry => {
+      this.__observers.slice(0).forEach(entry => {
+        if (entry.unwatched) {
+          return
+        }
         var getter = entry.getter
         var code = hashCode(getter)
         var prevState = this.__prevState
@@ -65,13 +68,15 @@ class ChangeObserver {
     var entry = {
       getter: getter,
       handler: handler,
+      unwatched: false,
     }
     this.__observers.push(entry)
     // return unwatch function
     return () => {
       // TODO: untrack from change emitter
       var ind = this.__observers.indexOf(entry)
       if (ind > -1) {
+        entry.unwatched = true
         this.__observers.splice(ind, 1)
       }
     }

tests/change-observer-tests.js

@@ -78,6 +78,46 @@ describe('ChangeObserver', () => {
         expect(mockFn2.calls.count()).toBe(1)
       })
     })
+
+    it('should not skip observers when handler causes unobserve', () => {
+      var getter = ['foo', 'bar']
+      var mockFn = jasmine.createSpy()
+      var unreg = observer.onChange(getter, () => unreg())
+      observer.onChange(getter, mockFn)
+
+      observer.notifyObservers(initialState.updateIn(getter, x => 2))
+
+      expect(mockFn.calls.count()).toBe(1)
+    })
+
+    it('should not call unwatched observers when removed during notify', () => {
+      var getter = ['foo', 'bar']
+      var mockFn1 = jasmine.createSpy()
+      var mockFn2 = jasmine.createSpy()
+      observer.onChange(getter, () => {
+        mockFn1()
+        unreg()
+      })
+      var unreg = observer.onChange(getter, mockFn2)
+
+      observer.notifyObservers(initialState.updateIn(getter, x => 2))
+
+      expect(mockFn1.calls.count()).toBe(1)
+      expect(mockFn2.calls.count()).toBe(0)
+    })
+
+    it('should not call new observers when handlers attach them', () => {
+      var getter = ['foo', 'bar']
+      var mockFn1 = jasmine.createSpy()
+      var mockFn2 = jasmine.createSpy()
+      observer.onChange(getter, mockFn1)
+      observer.onChange(getter, () => observer.onChange(getter, mockFn2))
+
+      observer.notifyObservers(initialState.updateIn(getter, x => 2))
+
+      expect(mockFn1.calls.count()).toBe(1)
+      expect(mockFn2.calls.count()).toBe(0)
+    })
   })
   // TODO: test the prevValues and registering an observable
 })