feat(transition): Improve supersede logic: Do not supersede if the new trans is aborted before onStart

Previously, any calls to state.go would supersede the running transition.
Now, if the new transition is aborted before it reaches "run" phase (onStart hook),
it will not cause the running transition to be aborted.

Closes #41

Author: christopherthielen

src/globals.ts

@@ -41,10 +41,14 @@ export class UIRouterGlobals implements Disposable {
   $current: StateObject;
 
   /**
-   * The current transition (in progress)
+   * The current started/running transition.
+   * This transition has reached at least the onStart phase, but is not yet complete
    */
   transition: Transition;
 
+  /** @internalapi */
+  lastStartedTransitionId: number = -1;
+
   /** @internalapi */
   transitionHistory = new Queue<Transition>([], 1);
 

src/hooks/updateGlobals.ts

@@ -1,7 +1,8 @@
-/** @module hooks */ /** for typedoc */
-import { Transition } from "../transition/transition";
-import { copy } from "../common/common";
-import { TransitionService } from "../transition/transitionService";
+/** @module hooks */
+/** for typedoc */
+import { Transition } from '../transition/transition';
+import { copy } from '../common/common';
+import { TransitionService } from '../transition/transitionService';
 
 /**
  * A [[TransitionHookFn]] which updates global UI-Router state
@@ -17,25 +18,23 @@ import { TransitionService } from "../transition/transitionService";
  */
 const updateGlobalState = (trans: Transition) => {
   let globals = trans.router.globals;
-  globals.transition = trans;
-  globals.transitionHistory.enqueue(trans);
 
-  const updateGlobalState = () => {
+  const transitionSuccessful = () => {
     globals.successfulTransitions.enqueue(trans);
     globals.$current = trans.$to();
     globals.current = globals.$current.self;
+
     copy(trans.params(), globals.params);
   };
 
-  trans.onSuccess({}, updateGlobalState, {priority: 10000});
-
   const clearCurrentTransition = () => {
     // Do not clear globals.transition if a different transition has started in the meantime
     if (globals.transition === trans) globals.transition = null;
   };
 
+  trans.onSuccess({}, transitionSuccessful, { priority: 10000 });
   trans.promise.then(clearCurrentTransition, clearCurrentTransition);
 };
 
 export const registerUpdateGlobalState = (transitionService: TransitionService) =>
-    transitionService.onBefore({}, updateGlobalState);
+    transitionService.onCreate({}, updateGlobalState);

src/state/stateService.ts

@@ -309,9 +309,10 @@ export class StateService {
   transitionTo(to: StateOrName, toParams: RawParams = {}, options: TransitionOptions = {}): TransitionPromise {
     let router = this.router;
     let globals = router.globals;
-    let transHistory = globals.transitionHistory;
     options = defaults(options, defaultTransOpts);
-    options = extend(options, { current: transHistory.peekTail.bind(transHistory)});
+    const getCurrent = () =>
+        globals.transition;
+    options = extend(options, { current: getCurrent });
 
     let ref: TargetState = this.target(to, toParams, options);
     let currentPath = this.getCurrentPath();

src/transition/transition.ts

@@ -618,8 +618,6 @@ export class Transition implements IHookRegistry {
    */
   run(): Promise<any> {
     let runAllHooks = TransitionHook.runAllHooks;
-    let globals = this.router.globals;
-    globals.transitionHistory.enqueue(this);
 
     // Gets transition hooks array for the given phase
     const hooksFor = (phase: TransitionHookPhase) =>
@@ -630,6 +628,16 @@ export class Transition implements IHookRegistry {
     const chainFor = (phase: TransitionHookPhase) =>
         TransitionHook.chain(hooksFor(phase));
 
+    const startTransition = () => {
+      let globals = this.router.globals;
+
+      globals.lastStartedTransitionId = this.$id;
+      globals.transition = this;
+      globals.transitionHistory.enqueue(this);
+
+      trace.traceTransitionStart(this);
+    };
+
     // When the chain is complete, then resolve or reject the deferred
     const transitionSuccess = () => {
       trace.traceSuccess(this.$to(), this);
@@ -648,7 +656,7 @@ export class Transition implements IHookRegistry {
 
     services.$q.when()
         .then(() => chainFor(TransitionHookPhase.BEFORE))
-        .then(() => trace.traceTransitionStart(this))
+        .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))
@@ -657,10 +665,9 @@ export class Transition implements IHookRegistry {
     return this.promise;
   }
 
-  /**
-   * Checks if this transition is currently active/running.
-   */
-  isActive = () => this === this._options.current();
+  /** Checks if this transition is currently active/running. */
+  isActive = () =>
+      this.router.globals.transition === this;
 
   /**
    * Checks if the Transition is valid

src/transition/transitionHook.ts

@@ -3,7 +3,7 @@
  * @module transition
  */
 /** for typedoc */
-import { TransitionHookOptions, HookResult } from './interface';
+import { TransitionHookOptions, HookResult, TransitionHookPhase } from './interface';
 import { defaults, noop, silentRejection } from '../common/common';
 import { fnToString, maxLength } from '../common/strings';
 import { isPromise } from '../common/predicates';
@@ -73,7 +73,7 @@ export class TransitionHook {
   };
 
   private isSuperseded = () =>
-      !this.type.synchronous && this.options.current() !== this.options.transition;
+    this.type.hookPhase === TransitionHookPhase.RUN && !this.options.transition.isActive();
 
   logError(err): any {
     this.transition.router.stateService.defaultErrorHandler()(err);

test/stateServiceSpec.ts

@@ -241,11 +241,8 @@ describe('stateService', function () {
       });
 
       afterEach((done) => {
-        if (router.globals.transition) {
-          router.globals.transition.promise.then(done, done)
-        } else {
-          done();
-        }
+        router.dispose();
+        done();
       });
 
       describe('[ transition.dynamic() ]:', function() {
@@ -545,12 +542,15 @@ describe('stateService', function () {
       done();
     });
 
-    it('aborts pending transitions when superseded from callbacks', async(done) => {
+    fit('aborts pending transitions when superseded from callbacks', async(done) => {
+      // router.trace.enable();
       $state.defaultErrorHandler(() => null);
       $registry.register({
         name: 'redir',
         url: "redir",
-        onEnter: (trans) => { trans.router.stateService.go('A') }
+        onEnter: (trans) => {
+          trans.router.stateService.go('A')
+        }
       });
       let result = await $state.transitionTo('redir').catch(err => err);
       await router.globals.transition.promise;
@@ -561,6 +561,25 @@ describe('stateService', function () {
       done();
     });
 
+    it('does not abort pending transition when a new transition is cancelled by onBefore hook', (done) => {
+      router.transitionService.onBefore({}, (t) => {
+        if (t.$id === 1) return false;
+        return (new Promise(resolve => setTimeout(resolve, 100))) as any;
+      });
+
+      let promise1 = $state.transitionTo('A'); // takes 100 ms
+      let promise2 = $state.transitionTo('A'); // is cancelled
+      let promise2Error;
+
+      promise1.then(() => {
+        expect($state.current.name).toBe('A');
+        expect(promise2Error).toBeDefined();
+        done();
+      });
+
+      promise2.catch((err) => promise2Error = err);
+    });
+
     it('triggers onEnter and onExit callbacks', async(done) => {
       let log = "";
       await initStateTo(A);