Merge pull request #133 from optimizely/jordan/fix-subsequent-dispatches

Allow subsequent dispatches to work after store throws error

Author: jordangarcia

src/reactor.js

@@ -117,12 +117,26 @@ class Reactor {
     }
 
     var prevState = this.state
-    this.state = this.__handleAction(prevState, actionType, payload)
+
+    try {
+      this.state = this.__handleAction(prevState, actionType, payload)
+    } catch (e) {
+      this.__isDispatching = false
+      throw e
+    }
+
 
     if (this.__batchDepth > 0) {
       this.__batchDispatchCount++
-    } else if (this.state !== prevState) {
-      this.__notify()
+    } else {
+      if (this.state !== prevState) {
+        try {
+          this.__notify()
+        } catch (e) {
+          this.__isDispatching = false
+          throw e
+        }
+      }
       this.__isDispatching = false
     }
   }
@@ -244,7 +258,6 @@ class Reactor {
     this.__changeObserver.notifyObservers(this.state)
   }
 
-
   /**
    * Reduces the current state to the new state given actionType / message
    * @param {string} actionType
@@ -257,22 +270,23 @@ class Reactor {
         logging.dispatchStart(actionType, payload)
       }
 
-      // let each core handle the message
+      // let each store handle the message
       this.__stores.forEach((store, id) => {
         var currState = state.get(id)
         var newState
-        var dispatchError
 
         try {
           newState = store.handle(currState, actionType, payload)
         } catch(e) {
-          dispatchError = e
+          // ensure console.group is properly closed
+          logging.dispatchError(e.message)
+          throw e
         }
 
-        if (this.debug && (newState === undefined || dispatchError)) {
-          var error = dispatchError || 'Store handler must return a value, did you forget a return statement'
-          logging.dispatchError(error)
-          throw new Error(error)
+        if (this.debug && newState === undefined) {
+          var errorMsg = 'Store handler must return a value, did you forget a return statement'
+          logging.dispatchError(errorMsg)
+          throw new Error(errorMsg)
         }
 
         state.set(id, newState)
@@ -299,7 +313,12 @@ class Reactor {
       if (this.__batchDispatchCount > 0) {
         // set to true to catch if dispatch called from observer
         this.__isDispatching = true
-        this.__notify()
+        try {
+          this.__notify()
+        } catch (e) {
+          this.__isDispatching = false
+          throw e
+        }
         this.__isDispatching = false
       }
       this.__batchDispatchCount = 0

tests/reactor-tests.js

@@ -29,6 +29,9 @@ describe('Reactor', () => {
         },
 
         initialize() {
+          this.on('storeError', (state, payload) => {
+            throw new Error('Store Error')
+          })
           this.on('addItem', (state, payload) => {
             return state.update('all', items => {
               return items.push(Map({
@@ -198,6 +201,36 @@ describe('Reactor', () => {
         expect(() => checkoutActions.setTaxPercent(5)).not.toThrow(
           new Error('Dispatch may not be called while a dispatch is in progress'))
       })
+
+      it('should allow subsequent dispatches if a store throws an error', () => {
+        try {
+          reactor.dispatch('storeError')
+        } catch (e) {} // eslint-disable-line
+
+        expect(() => reactor.dispatch('setTax', 5)).not.toThrow()
+      })
+
+      it('should allow subsequent dispatches if a dispatched action doesnt cause state change', () => {
+        reactor.dispatch('noop')
+
+        expect(() => reactor.dispatch('setTax', 5)).not.toThrow()
+      })
+
+      it('should allow subsequent dispatches if an observer throws an error', () => {
+        var unWatchFn = reactor.observe([], state => {
+          throw new Error('observer error')
+        })
+
+        try {
+          checkoutActions.setTaxPercent(1)
+        } catch (e) {} // eslint-disable-line
+
+        unWatchFn()
+
+        expect(() => {
+          checkoutActions.setTaxPercent(2)
+        }).not.toThrow()
+      })
     }) // when dispatching a relevant action
 
     describe('#observe', () => {
@@ -500,8 +533,8 @@ describe('Reactor', () => {
     it('should log and throw an error', function() {
       expect(function() {
         reactor.dispatch('set', 'foo')
-      }).toThrow()
-      expect(logging.dispatchError).toHaveBeenCalled()
+      }).toThrow(new Error('Error during action handling'))
+      expect(logging.dispatchError).toHaveBeenCalledWith('Error during action handling')
     })
   })
 
@@ -976,6 +1009,9 @@ describe('Reactor', () => {
           },
           initialize() {
             this.on('add', (state, item) => state.push(toImmutable(item)))
+            this.on('error', (state, payload) => {
+              throw new Error('store error');
+            })
           },
         }),
       })
@@ -1066,5 +1102,46 @@ describe('Reactor', () => {
       }).not.toThrow(
         new Error('Dispatch may not be called while a dispatch is in progress'))
     })
+
+    it('should allow subsequent dispatches if an error is raised by a store handler', () => {
+      expect(() => {
+        reactor.batch(() => {
+          reactor.dispatch('add', 'one')
+          reactor.dispatch('error')
+        })
+      }).toThrow(new Error('store error'))
+
+      expect(() => {
+        reactor.dispatch('add', 'three')
+      }).not.toThrow()
+    })
+
+    it('should allow subsequent dispatches if batched action doesnt cause state change', () => {
+      reactor.batch(() => {
+        reactor.dispatch('noop')
+      })
+
+      expect(() => reactor.dispatch('add', 'one')).not.toThrow()
+    })
+
+    it('should allow subsequent dispatches if an error is raised in an observer', () => {
+      var unWatchFn = reactor.observe([], state => {
+        throw new Error('observe error')
+      })
+
+      expect(() => {
+        reactor.batch(() => {
+          reactor.dispatch('add', 'one')
+          reactor.dispatch('add', 'two')
+        })
+      }).toThrow(
+        new Error('observe error'))
+
+      unWatchFn()
+
+      expect(() => {
+        reactor.dispatch('add', 'three')
+      }).not.toThrow()
+    })
   })
 })