fix(onBefore): Skip remaining hooks after the (#2)

first hook that modifies the transition.

Author: csvn

src/transition/transition.ts

@@ -473,6 +473,7 @@ export class Transition implements IHookRegistry {
    */
   run(): Promise<any> {
     let runSynchronousHooks = TransitionHook.runSynchronousHooks;
+    let runAllHooks = TransitionHook.runAllHooks;
     let hookBuilder = this.hookBuilder();
     let globals = <Globals> this.router.globals;
     globals.transitionHistory.enqueue(this);
@@ -503,15 +504,15 @@ export class Transition implements IHookRegistry {
       trace.traceSuccess(this.$to(), this);
       this.success = true;
       this._deferred.resolve(this.to());
-      runSynchronousHooks(hookBuilder.getOnSuccessHooks(), true);
+      runAllHooks(hookBuilder.getOnSuccessHooks());
     };
 
     const transitionError = (reason: any) => {
       trace.traceError(reason, this);
       this.success = false;
       this._deferred.reject(reason);
       this._error = reason;
-      runSynchronousHooks(hookBuilder.getOnErrorHooks(), true);
+      runAllHooks(hookBuilder.getOnErrorHooks());
     };
 
     trace.traceTransitionStart(this);

src/transition/transitionHook.ts

@@ -94,31 +94,44 @@ export class TransitionHook {
     return `${event} context: ${context}, ${maxLength(200, name)}`;
   }
 
+  /**
+   * Run all TransitionHooks, ignoring their return value.
+   */
+  static runAllHooks(hooks: TransitionHook[]): void {
+    hooks.forEach(hook => {
+      try {
+        hook.invokeHook();
+      } catch (exception) {
+        let errorHandler = hook.transition.router.stateService.defaultErrorHandler();
+        errorHandler(exception);
+      }
+    });
+  }
 
   /**
    * Given an array of TransitionHooks, runs each one synchronously and sequentially.
+   * Should any hook return a Rejection synchronously, the remaining hooks will not run.
    *
    * Returns a promise chain composed of any promises returned from each hook.invokeStep() call
    */
-  static runSynchronousHooks(hooks: TransitionHook[], swallowExceptions: boolean = false): Promise<any> {
+  static runSynchronousHooks(hooks: TransitionHook[]): Promise<any> {
     let results: Promise<HookResult>[] = [];
-    for (let i = 0; i < hooks.length; i++) {
-      let hook = hooks[i];
+
+    for (let hook of hooks) {
       try {
-        results.push(hook.invokeHook());
-      } catch (exception) {
-        if (!swallowExceptions) {
-          return Rejection.errored(exception).toPromise();
+        let hookResult = hook.invokeHook();
+        
+        if (Rejection.isTransitionRejectionPromise(hookResult)) {
+          // Break on first thrown error or false/TargetState
+          return hookResult;
         }
 
-        let errorHandler = hook.transition.router.stateService.defaultErrorHandler();
-        errorHandler(exception);
+        results.push(hookResult);
+      } catch (exception) {
+        return Rejection.errored(exception).toPromise();
       }
     }
 
-    let rejections = results.filter(Rejection.isTransitionRejectionPromise);
-    if (rejections.length) return rejections[0];
-
     return results
         .filter(isPromise)
         .reduce((chain: Promise<any>, promise: Promise<any>) => chain.then(val(promise)), services.$q.when());

test/transitionSpec.ts

@@ -152,6 +152,61 @@ describe('transition', function () {
         }), 20);
       }));
 
+      describe('.onBefore()', function() {
+        beforeEach(() => $state.defaultErrorHandler(() => {}));
+
+        it('should stop running remaining hooks if hook modifies transition synchronously', ((done) => {
+          let counter = 0;
+          let increment = (amount) => {
+            return () => {
+              counter += amount;
+              return false;
+            };
+          };
+
+          $transitions.onBefore({}, increment(1), { priority: 2 });
+          $transitions.onBefore({}, increment(100), { priority: 1 });
+
+          Promise.resolve()
+            .then(goFail('first', 'second'))
+            .then(() => expect(counter).toBe(1))
+            .then(goFail('second', 'third'))
+            .then(() => expect(counter).toBe(2))
+            .then(done);
+        }));
+
+        it('should stop running remaining hooks when synchronous result throws or returns false|TargetState', ((done) => {
+          let current = null;
+
+          $transitions.onBefore({}, (t) => { current = t.to().name; });
+          $transitions.onBefore({ to: 'first' }, () => {
+            throw Error('first-error');
+          }, { priority: 1 });
+          $transitions.onBefore({ to: 'second' }, () => false, { priority: 3 });
+          $transitions.onBefore({ to: 'third' }, () => $state.target('A'), { priority: 2 });
+
+          Promise.resolve()
+            .then(goFail('A', 'first'))
+            .then((res) => {
+              expect(res.type).toBe(RejectType.ERROR);
+              expect(current).toBe(null);
+            })
+            .then(goFail('A', 'second'))
+            .then((res) => {
+              expect(res.type).toBe(RejectType.ABORTED);
+              expect(current).toBe(null);
+            })
+            .then(goFail('A', 'third'))
+            .then((res) => {
+              expect(res.type).toBe(RejectType.SUPERSEDED);
+              expect(current).toBe(null);
+            })
+            .then(go('A', 'B'))
+            .then(() => expect(current).toBe('B'))
+            .then(done);
+        }));
+      });
+
       describe('.onStart()', function() {
         it('should fire matching events when transition starts', ((done) => {
           var t = null;
@@ -317,7 +372,9 @@ describe('transition', function () {
               .then(done);
         }));
 
-        it('should be called even if other .onSuccess() callbacks fail (throw errors, etc)', ((done) => {
+        it('should call all .onSuccess() even when callbacks fail (throw errors, etc)', ((done) => {
+          $transitions.onSuccess({ from: "*", to: "*" }, () => false);
+          $transitions.onSuccess({ from: "*", to: "*" }, () => $state.target('A'));
           $transitions.onSuccess({ from: "*", to: "*" }, function() { throw new Error("oops!"); });
           $transitions.onSuccess({ from: "*", to: "*" }, function(trans) { states.push(trans.to().name); });
 
@@ -364,6 +421,33 @@ describe('transition', function () {
               .then(() => expect(hooks).toEqual([ 'splatsplat', 'AD' ]))
               .then(done);
         }));
+
+        it('should call all error handlers when transition fails.', ((done) => {
+          let count = 0;
+
+          $state.defaultErrorHandler(() => {});
+          $transitions.onStart({}, () => false);
+          $transitions.onError({}, () => {
+            count += 1;
+            return false;
+          });
+          $transitions.onError({}, () => {
+            count += 10;
+            $state.target('A');
+          });
+          $transitions.onError({}, function() {
+            count += 100;
+            throw new Error("oops!");
+          });
+          $transitions.onError({}, () => {
+            count += 1000;
+          });
+
+          Promise.resolve()
+              .then(goFail("B", "C"))
+              .then(() => expect(count).toBe(1111))
+              .then(done);
+        }));
       });
 
       // Test for #2866