feat(transition): Run hooks synchronously in current stack, when possible

Author: christopherthielen

src/state/stateService.ts

@@ -104,12 +104,18 @@ export class StateService {
       if (!(result instanceof TargetState)) {
         return;
       }
+
       let target = <TargetState> result;
       // Recreate the TargetState, in case the state is now defined.
       target = this.target(target.identifier(), target.params(), target.options());
 
-      if (!target.valid()) return Rejection.invalid(target.error()).toPromise();
-      if (latestThing() !== latest) return Rejection.superseded().toPromise();
+      if (!target.valid()) {
+        return Rejection.invalid(target.error()).toPromise();
+      }
+
+      if (latestThing() !== latest) {
+        return Rejection.superseded().toPromise();
+      }
 
       return this.transitionTo(target.identifier(), target.params(), target.options());
     };

src/transition/transition.ts

@@ -6,14 +6,14 @@
 import { trace } from '../common/trace';
 import { services } from '../common/coreservices';
 import {
-  map, find, extend, mergeR, tail, omit, toJson, arrayTuples, unnestR, identity, anyTrueR
+  map, find, extend, mergeR, tail, omit, toJson, arrayTuples, unnestR, identity, anyTrueR,
 } from '../common/common';
-import { isObject, isUndefined } from '../common/predicates';
+import { isObject } from '../common/predicates';
 import { prop, propEq, val, not, is } from '../common/hof';
 import { StateDeclaration, StateOrName } from '../state/interface';
 import {
   TransitionOptions, TreeChanges, IHookRegistry, TransitionHookPhase, RegisteredHooks, HookRegOptions,
-  HookMatchCriteria, TransitionStateHookFn, TransitionHookFn
+  HookMatchCriteria, TransitionStateHookFn, TransitionHookFn,
 } from './interface'; // has or is using
 import { TransitionHook } from './transitionHook';
 import { matchState, makeEvent, RegisteredHook } from './hookRegistry';
@@ -94,21 +94,21 @@ export class Transition implements IHookRegistry {
 
 
   /** @hidden */
-  onBefore(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return }
+  onBefore(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return; }
   /** @inheritdoc */
-  onStart(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return }
+  onStart(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return; }
   /** @inheritdoc */
-  onExit(criteria: HookMatchCriteria, callback: TransitionStateHookFn, options?: HookRegOptions): Function { return }
+  onExit(criteria: HookMatchCriteria, callback: TransitionStateHookFn, options?: HookRegOptions): Function { return; }
   /** @inheritdoc */
-  onRetain(criteria: HookMatchCriteria, callback: TransitionStateHookFn, options?: HookRegOptions): Function { return }
+  onRetain(criteria: HookMatchCriteria, callback: TransitionStateHookFn, options?: HookRegOptions): Function { return; }
   /** @inheritdoc */
-  onEnter(criteria: HookMatchCriteria, callback: TransitionStateHookFn, options?: HookRegOptions): Function { return }
+  onEnter(criteria: HookMatchCriteria, callback: TransitionStateHookFn, options?: HookRegOptions): Function { return; }
   /** @inheritdoc */
-  onFinish(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return }
+  onFinish(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return; }
   /** @inheritdoc */
-  onSuccess(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return }
+  onSuccess(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return; }
   /** @inheritdoc */
-  onError(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return }
+  onError(criteria: HookMatchCriteria, callback: TransitionHookFn, options?: HookRegOptions): Function { return; }
 
   /** @hidden
    * Creates the transition-level hook registration functions
@@ -511,7 +511,7 @@ export class Transition implements IHookRegistry {
    */
   redirect(targetState: TargetState): Transition {
     let redirects = 1, trans: Transition = this;
-    while((trans = trans.redirectedFrom()) != null) {
+    while ((trans = trans.redirectedFrom()) != null) {
       if (++redirects > 20) throw new Error(`Too many consecutive Transition redirects (20+)`);
     }
 
@@ -620,14 +620,9 @@ export class Transition implements IHookRegistry {
     let runAllHooks = TransitionHook.runAllHooks;
 
     // Gets transition hooks array for the given phase
-    const hooksFor = (phase: TransitionHookPhase) =>
+    const getHooksFor = (phase: TransitionHookPhase) =>
         this._hookBuilder.buildHooksForPhase(phase);
 
-    // Builds a chain of transition hooks for the given phase
-    // Each hook is invoked after the previous one completes
-    const chainFor = (phase: TransitionHookPhase) =>
-        TransitionHook.chain(hooksFor(phase));
-
     const startTransition = () => {
       let globals = this.router.globals;
 
@@ -636,31 +631,39 @@ export class Transition implements IHookRegistry {
       globals.transitionHistory.enqueue(this);
 
       trace.traceTransitionStart(this);
+
+      return services.$q.when(undefined);
     };
 
+
     // When the chain is complete, then resolve or reject the deferred
     const transitionSuccess = () => {
       trace.traceSuccess(this.$to(), this);
       this.success = true;
       this._deferred.resolve(this.to());
-      runAllHooks(hooksFor(TransitionHookPhase.SUCCESS));
+      runAllHooks(getHooksFor(TransitionHookPhase.SUCCESS));
     };
 
     const transitionError = (reason: any) => {
       trace.traceError(reason, this);
       this.success = false;
       this._deferred.reject(reason);
       this._error = reason;
-      runAllHooks(hooksFor(TransitionHookPhase.ERROR));
+      runAllHooks(getHooksFor(TransitionHookPhase.ERROR));
+    };
+
+    // This waits to build the RUN hook chain until after the "BEFORE" hooks complete
+    // This allows a BEFORE hook to dynamically add RUN hooks via the Transition object.
+    const runTransition = () => {
+      let allRunHooks = getHooksFor(TransitionHookPhase.RUN);
+      let done = () => services.$q.when(undefined);
+      TransitionHook.invokeHooks(allRunHooks, done)
+          .then(transitionSuccess, transitionError);
     };
 
-    services.$q.when()
-        .then(() => chainFor(TransitionHookPhase.BEFORE))
-        .then(startTransition)
-        // This waits to build the RUN hook chain until after the "BEFORE" hooks complete
-        // This allows a BEFORE hook to dynamically add RUN hooks via the Transition object.
-        .then(() => chainFor(TransitionHookPhase.RUN))
-        .then(transitionSuccess, transitionError);
+    let allBeforeHooks = getHooksFor(TransitionHookPhase.BEFORE);
+    TransitionHook.invokeHooks(allBeforeHooks, startTransition)
+        .then(runTransition);
 
     return this.promise;
   }

src/transition/transitionHook.ts

@@ -15,7 +15,7 @@ import { TargetState } from '../state/targetState';
 import { Transition } from './transition';
 import { TransitionEventType } from './transitionEventType';
 import { RegisteredHook } from './hookRegistry';
-import { StateDeclaration } from '../state/interface'; // has or is using
+import { StateDeclaration } from '../state/interface';
 
 let defaultOptions: TransitionHookOptions = {
   current: noop,
@@ -79,7 +79,7 @@ export class TransitionHook {
     this.transition.router.stateService.defaultErrorHandler()(err);
   }
 
-  invokeHook(): Promise<HookResult> {
+  invokeHook(): Promise<HookResult> | void {
     let hook = this.registeredHook;
     if (hook._deregistered) return;
 
@@ -101,18 +101,19 @@ export class TransitionHook {
     const handleResult = result =>
         hook.eventType.getResultHandler(this)(result);
 
-    if (this.type.synchronous) {
-      try {
-        return handleResult(invokeCallback());
-      } catch (err) {
-        return handleError(Rejection.normalize(err));
+    try {
+      let result = invokeCallback();
+
+      if (isPromise(result)) {
+        return result.catch(normalizeErr)
+            .then(handleResult, handleError);
+      } else {
+        return handleResult(result);
       }
+    } catch (err) {
+      // If callback throws (synchronously)
+      return handleError(Rejection.normalize(err));
     }
-
-    return services.$q.when()
-        .then(invokeCallback)
-        .catch(normalizeErr)
-        .then(handleResult, handleError);
   }
 
   /**
@@ -205,6 +206,30 @@ export class TransitionHook {
   }
 
 
+  /**
+   * Invokes all the provided TransitionHooks, in order.
+   * Each hook's return value is checked.
+   * If any hook returns a promise, then the rest of the hooks are chained off that promise, and the promise is returned.
+   * If no hook returns a promise, then all hooks are processed synchronously.
+   *
+   * @param hooks the list of TransitionHooks to invoke
+   * @param done a callback that is invoked after all the hooks have successfully completed
+   *
+   * @returns a promise for the async result, or the result of the callback
+   */
+  static invokeHooks<T>(hooks: TransitionHook[], done: () => T): Promise<any> | T {
+    for (let idx = 0; idx < hooks.length; idx++) {
+      let hookResult = hooks[idx].invokeHook();
+
+      if (isPromise(hookResult)) {
+        let remainingHooks = hooks.slice(idx + 1);
+        return TransitionHook.chain(remainingHooks, hookResult).then(done);
+      }
+    }
+
+    return done();
+  }
+
   /**
    * Run all TransitionHooks, ignoring their return value.
    */

test/stateServiceSpec.ts

@@ -542,8 +542,8 @@ describe('stateService', function () {
       done();
     });
 
-    fit('aborts pending transitions when superseded from callbacks', async(done) => {
-      // router.trace.enable();
+    it('aborts pending transitions when superseded from callbacks', async(done) => {
+      // router.trace.enable(1);
       $state.defaultErrorHandler(() => null);
       $registry.register({
         name: 'redir',

tslint.json

@@ -17,7 +17,7 @@
     "no-any": false,
     "no-arg": true,
     "no-bitwise": true,
-    "no-conditional-assignment": true,
+    "no-conditional-assignment": false,
     "no-console": [true, "debug", "info", "time", "timeEnd", "trace" ],
     "no-construct": true,
     "no-constructor-vars": false,