refactor(nav-controller-base): cleanup some logic

NavControllerBase is the core of ionic 2 navigation. It handles all the transitions and it is complicated code to follow.
I am refactoring it to allow future developers and contributors to follow it better.

!node.parent now becomes node.isRoot()

ViewController does not remove itself from the stack, but two new auxiliar function in nav controller: _insertView() and _removeView() are used to add a view to the stack.

And so on...

All e2e and unit tests passing...

src/animations/animation.ts

@@ -34,6 +34,7 @@ export class Animation {
 
   parent: Animation;
   opts: AnimationOptions;
+  hasChildren: boolean = false;
   isPlaying: boolean = false;
   hasCompleted: boolean = false;
 
@@ -81,6 +82,7 @@ export class Animation {
    */
   add(childAnimation: Animation): Animation {
     childAnimation.parent = this;
+    this.hasChildren = true;
     this._cL = (this._c = this._c || []).push(childAnimation);
     return this;
   }

src/components/tabs/tab.ts

@@ -298,14 +298,14 @@ export class Tab extends NavControllerBase {
   /**
    * @private
    */
-  _viewInsert(viewCtrl: ViewController, componentRef: ComponentRef<any>, viewport: ViewContainerRef) {
+  _viewAttachToDOM(viewCtrl: ViewController, componentRef: ComponentRef<any>, viewport: ViewContainerRef) {
     const isTabSubPage = (this.parent._subPages && viewCtrl.index > 0);
 
     if (isTabSubPage) {
       viewport = this.parent.portal;
     }
 
-    super._viewInsert(viewCtrl, componentRef, viewport);
+    super._viewAttachToDOM(viewCtrl, componentRef, viewport);
 
     if (isTabSubPage) {
       // add the .tab-subpage css class to tabs pages that should act like subpages

src/navigation/nav-controller-base.ts

@@ -193,9 +193,11 @@ export class NavControllerBase extends Ion implements NavController {
       while (trns) {
         if (trns.enteringView && (trns.enteringView._state !== ViewState.LOADED)) {
           // destroy the entering views and all of their hopes and dreams
-          trns.enteringView._destroy(this._renderer);
+          this._destroyView(trns.enteringView);
+        }
+        if (!trns.parent) {
+          break;
         }
-        if (!trns.parent) break;
       }
 
       if (trns) {
@@ -253,6 +255,8 @@ export class NavControllerBase extends Ion implements NavController {
     const leavingView = this.getActive();
     const enteringView = this._getEnteringView(ti, leavingView);
 
+    assert(leavingView || enteringView, 'Both leavingView and enteringView are null');
+
     // Initialize enteringView
     if (enteringView && isBlank(enteringView._state)) {
       // render the entering view, and all child navs and views
@@ -327,17 +331,23 @@ export class NavControllerBase extends Ion implements NavController {
     const opts = ti.opts || {};
     const insertViews = ti.insertViews;
     const removeStart = ti.removeStart;
-
+    let view;
     let destroyQueue: ViewController[] = [];
 
+    // there are views to remove
     if (isPresent(removeStart)) {
       for (var i = 0; i < ti.removeCount; i++) {
-        destroyQueue.push(this._views[i + removeStart]);
+        view = this._views[i + removeStart];
+        assert(view, 'internal error, view in destroyQueue can not be null');
+        if (view && view !== enteringView && view !== leavingView) {
+          destroyQueue.push(view);
+        }
       }
       // default the direction to "back"
       opts.direction = opts.direction || DIRECTION_BACK;
     }
 
+    // there are views to insert
     if (insertViews) {
       // manually set the new view's id if an id was passed in the options
       if (isPresent(opts.id)) {
@@ -346,25 +356,8 @@ export class NavControllerBase extends Ion implements NavController {
 
       // add the views to the
       for (var i = 0; i < insertViews.length; i++) {
-        var view = insertViews[i];
-
-        var existingIndex = this._views.indexOf(view);
-        if (existingIndex > -1) {
-          // this view is already in the stack!!
-          // move it to its new location
-          this._views.splice(ti.insertStart + i, 0, this._views.splice(existingIndex, 1)[0]);
-
-        } else {
-          // this is a new view to add to the stack
-          // create the new entering view
-          view._setNav(this);
-
-          // give this inserted view an ID
-          view.id = this.id + '-' + (++this._ids);
-
-          // insert the entering view into the correct index in the stack
-          this._views.splice(ti.insertStart + i, 0, view);
-        }
+        view = insertViews[i];
+        this._insertViewAt(view, ti.insertStart + i);
       }
 
       if (ti.enteringRequiresTransition) {
@@ -372,27 +365,20 @@ export class NavControllerBase extends Ion implements NavController {
         opts.direction = opts.direction || DIRECTION_FORWARD;
       }
     }
-
     // if the views to be removed are in the beginning or middle
     // and there is not a view that needs to visually transition out
     // then just destroy them and don't transition anything
-    for (var i = 0; i < destroyQueue.length; i++) {
     // batch all of lifecycles together
-      var view = destroyQueue[i];
-      if (view && view !== enteringView && view !== leavingView) {
+    for (view of destroyQueue) {
       this._willLeave(view);
       this._didLeave(view);
       this._willUnload(view);
     }
+
+    // once all lifecycle events has been delivered, we can safely detroy the views
+    for (view of destroyQueue) {
+      this._destroyView(view);
     }
-    for (var i = 0; i < destroyQueue.length; i++) {
-      // batch all of the destroys together
-      var view = destroyQueue[i];
-      if (view && view !== enteringView && view !== leavingView) {
-        view._destroy(this._renderer);
-      }
-    }
-    destroyQueue.length = 0;
 
     if (ti.enteringRequiresTransition || ti.leavingRequiresTransition && enteringView !== leavingView) {
       // set which animation it should use if it wasn't set yet
@@ -434,6 +420,27 @@ export class NavControllerBase extends Ion implements NavController {
     this._willLoad(enteringView);
   }
 
+  _viewAttachToDOM(view: ViewController, componentRef: ComponentRef<any>, viewport: ViewContainerRef) {
+    // successfully finished loading the entering view
+    // fire off the "didLoad" lifecycle events
+    this._didLoad(view);
+
+    // render the component ref instance to the DOM
+    // ******** DOM WRITE ****************
+    viewport.insert(componentRef.hostView, viewport.length);
+    view._state = ViewState.PRE_RENDERED;
+
+    if (view._cssClass) {
+      // the ElementRef of the actual ion-page created
+      var pageElement = componentRef.location.nativeElement;
+
+      // ******** DOM WRITE ****************
+      this._renderer.setElementClass(pageElement, view._cssClass, true);
+    }
+
+    componentRef.changeDetectorRef.detectChanges();
+  }
+
   _viewTest(enteringView: ViewController, leavingView: ViewController, ti: TransitionInstruction) {
     const promises: Promise<any>[] = [];
     const reject = ti.reject;
@@ -442,47 +449,38 @@ export class NavControllerBase extends Ion implements NavController {
     if (leavingView) {
       const leavingTestResult = leavingView._lifecycleTest('Leave');
 
-      if (isPresent(leavingTestResult) && leavingTestResult !== true) {
-        if (leavingTestResult instanceof Promise) {
-          // async promise
-          promises.push(leavingTestResult);
-
-        } else {
+      if (leavingTestResult === false) {
         // synchronous reject
         reject((leavingTestResult !== false ? leavingTestResult : `ionViewCanLeave rejected`));
         return false;
-        }
+      } else if (leavingTestResult instanceof Promise) {
+        // async promise
+        promises.push(leavingTestResult);
       }
     }
 
     if (enteringView) {
       const enteringTestResult = enteringView._lifecycleTest('Enter');
 
-      if (isPresent(enteringTestResult) && enteringTestResult !== true) {
-        if (enteringTestResult instanceof Promise) {
-          // async promise
-          promises.push(enteringTestResult);
-
-        } else {
+      if (enteringTestResult === false) {
         // synchronous reject
         reject((enteringTestResult !== false ? enteringTestResult : `ionViewCanEnter rejected`));
         return false;
-        }
+      } else if (enteringTestResult instanceof Promise) {
+        // async promise
+        promises.push(enteringTestResult);
       }
     }
 
     if (promises.length) {
       // darn, async promises, gotta wait for them to resolve
-      Promise.all(promises).then(() => {
-        // all promises resolved! let's continue
-        this._postViewInit(enteringView, leavingView, ti, resolve);
-
-      }).catch(reject);
-      return true;
-    }
-
+      Promise.all(promises)
+        .then(() => this._postViewInit(enteringView, leavingView, ti, resolve))
+        .catch(reject);
+    } else {
       // synchronous and all tests passed! let's move on already
       this._postViewInit(enteringView, leavingView, ti, resolve);
+    }
     return true;
   }
 
@@ -508,27 +506,21 @@ export class NavControllerBase extends Ion implements NavController {
 
     // create the transition animation from the TransitionController
     // this will either create the root transition, or add it as a child transition
-    const trns = this._trnsCtrl.get(this._trnsId, enteringView, leavingView, animationOpts);
+    const transition = this._trnsCtrl.get(this._trnsId, enteringView, leavingView, animationOpts);
 
     // ensure any swipeback transitions are cleared out
     this._sbTrns && this._sbTrns.destroy();
+    this._sbTrns = null;
 
-    if (trns.parent) {
-      // this is important for later to know if there
-      // are any more child tests to check for
-      trns.parent.hasChildTrns = true;
-
-    } else {
-      // this is the root transition
-      if (opts.progressAnimation) {
-        this._sbTrns = trns;
-      }
+    // swipe to go back root transition
+    if (transition.isRoot() && opts.progressAnimation) {
+      this._sbTrns = transition;
     }
 
-    trns.registerStart(() => {
-      this._trnsStart(trns, enteringView, leavingView, opts, resolve);
-      if (trns.parent) {
-        trns.parent.start();
+    transition.registerStart(() => {
+      this._trnsStart(transition, enteringView, leavingView, opts, resolve);
+      if (transition.parent) {
+        transition.parent.start();
       }
     });
 
@@ -537,37 +529,16 @@ export class NavControllerBase extends Ion implements NavController {
       // this would also render new child navs/views
       // which may have their very own async canEnter/Leave tests
       // ******** DOM WRITE ****************
-      this._viewInsert(enteringView, enteringView._cmp, this._viewport);
+      this._viewAttachToDOM(enteringView, enteringView._cmp, this._viewport);
     }
 
-    if (!trns.hasChildTrns) {
+    if (!transition.hasChildren) {
       // lowest level transition, so kick it off and let it bubble up to start all of them
-      trns.start();
+      transition.start();
     }
   }
 
-  _viewInsert(view: ViewController, componentRef: ComponentRef<any>, viewport: ViewContainerRef) {
-    // successfully finished loading the entering view
-    // fire off the "didLoad" lifecycle events
-    this._didLoad(view);
-
-    // render the component ref instance to the DOM
-    // ******** DOM WRITE ****************
-    viewport.insert(componentRef.hostView, viewport.length);
-    view._state = ViewState.PRE_RENDERED;
-
-    if (view._cssClass) {
-      // the ElementRef of the actual ion-page created
-      var pageElement = componentRef.location.nativeElement;
-
-      // ******** DOM WRITE ****************
-      this._renderer.setElementClass(pageElement, view._cssClass, true);
-    }
-
-    componentRef.changeDetectorRef.detectChanges();
-  }
-
-  _trnsStart(trns: Transition, enteringView: ViewController, leavingView: ViewController, opts: NavOptions, resolve: TransitionResolveFn) {
+  _trnsStart(transition: Transition, enteringView: ViewController, leavingView: ViewController, opts: NavOptions, resolve: TransitionResolveFn) {
     this._trnsId = null;
 
     // set the correct zIndex for the entering and leaving views
@@ -578,43 +549,46 @@ export class NavControllerBase extends Ion implements NavController {
     // ******** DOM WRITE ****************
     enteringView && enteringView._domShow(true, this._renderer);
 
-    if (leavingView) {
     // always ensure the leaving view is viewable
     // ******** DOM WRITE ****************
-      leavingView._domShow(true, this._renderer);
-    }
+    leavingView && leavingView._domShow(true, this._renderer);
 
     // initialize the transition
-    trns.init();
+    transition.init();
 
-    if ((!this._init && this._views.length === 1 && !this._isPortal) || this.config.get('animate') === false) {
-      // the initial load shouldn't animate, unless it's a portal
+    // we should animate (duration > 0) if the pushed page is not the first one (startup)
+    // or if it is a portal (modal, actionsheet, etc.)
+    let isFirstPage = !this._init && this._views.length === 1;
+    let shouldNotAnimate = isFirstPage && !this._isPortal;
+    let canNotAnimate = this.config.get('animate') === false;
+    if (shouldNotAnimate || canNotAnimate) {
       opts.animate = false;
     }
+
     if (opts.animate === false) {
       // if it was somehow set to not animation, then make the duration zero
-      trns.duration(0);
+      transition.duration(0);
     }
 
     // create a callback that needs to run within zone
     // that will fire off the willEnter/Leave lifecycle events at the right time
-    trns.beforeAddRead(() => {
+    transition.beforeAddRead(() => {
       this._zone.run(this._viewsWillLifecycles.bind(this, enteringView, leavingView));
     });
 
     // create a callback for when the animation is done
-    trns.onFinish(() => {
+    transition.onFinish(() => {
       // transition animation has ended
-      this._zone.run(this._trnsFinish.bind(this, trns, opts, resolve));
+      this._zone.run(this._trnsFinish.bind(this, transition, opts, resolve));
     });
 
     // get the set duration of this transition
-    const duration = trns.getDuration();
+    const duration = transition.getDuration();
 
     // set that this nav is actively transitioning
     this.setTransitioning(true, duration);
 
-    if (!trns.parent) {
+    if (transition.isRoot()) {
       // this is the top most, or only active transition, so disable the app
       // add XXms to the duration the app is disabled when the keyboard is open
 
@@ -622,19 +596,21 @@ export class NavControllerBase extends Ion implements NavController {
         // if this transition has a duration and this is the root transition
         // then set that the app is actively disabled
         this._app.setEnabled(false, duration + ACTIVE_TRANSITION_OFFSET);
+      } else {
+        console.debug('transition is running but app has not been disabled');
       }
 
       // cool, let's do this, start the transition
       if (opts.progressAnimation) {
         // this is a swipe to go back, just get the transition progress ready
         // kick off the swipe animation start
-        trns.progressStart();
+        transition.progressStart();
 
       } else {
         // only the top level transition should actually start "play"
         // kick it off and let it play through
         // ******** DOM WRITE ****************
-        trns.play();
+        transition.play();
       }
     }
   }
@@ -645,32 +621,30 @@ export class NavControllerBase extends Ion implements NavController {
     leavingView && this._willLeave(leavingView);
   }
 
-  _trnsFinish(trns: Transition, opts: NavOptions, resolve: TransitionResolveFn) {
-    const hasCompleted = trns.hasCompleted;
-
+  _trnsFinish(transition: Transition, opts: NavOptions, resolve: TransitionResolveFn) {
     // mainly for testing
     let enteringName: string;
     let leavingName: string;
 
-    if (hasCompleted) {
+    if (transition.hasCompleted) {
       // transition has completed (went from 0 to 1)
-      if (trns.enteringView) {
-        enteringName = trns.enteringView.name;
-        this._didEnter(trns.enteringView);
+      if (transition.enteringView) {
+        enteringName = transition.enteringView.name;
+        this._didEnter(transition.enteringView);
       }
 
-      if (trns.leavingView) {
-        leavingName = trns.leavingView.name;
-        this._didLeave(trns.leavingView);
+      if (transition.leavingView) {
+        leavingName = transition.leavingView.name;
+        this._didLeave(transition.leavingView);
       }
 
-      this._cleanup(trns.enteringView);
+      this._cleanup(transition.enteringView);
     }
 
-    if (!trns.parent) {
+    if (transition.isRoot()) {
       // this is the root transition
       // it's save to destroy this transition
-      this._trnsCtrl.destroy(trns.trnsId);
+      this._trnsCtrl.destroy(transition.trnsId);
 
       // it's safe to enable the app again
       this._app.setEnabled(true);
@@ -681,7 +655,7 @@ export class NavControllerBase extends Ion implements NavController {
         this._linker.navChange(opts.direction);
       }
 
-      if (opts.keyboardClose !== false && this._keyboard.isOpen()) {
+      if (opts.keyboardClose !== false) {
         // the keyboard is still open!
         // no problem, let's just close for them
         this._keyboard.close();
@@ -689,7 +663,40 @@ export class NavControllerBase extends Ion implements NavController {
     }
 
     // congrats, we did it!
-    resolve(hasCompleted, true, enteringName, leavingName, opts.direction);
+    resolve(transition.hasCompleted, true, enteringName, leavingName, opts.direction);
+  }
+
+  _insertViewAt(view: ViewController, index: number) {
+    var existingIndex = this._views.indexOf(view);
+    if (existingIndex > -1) {
+      // this view is already in the stack!!
+      // move it to its new location
+      this._views.splice(index, 0, this._views.splice(existingIndex, 1)[0]);
+
+    } else {
+      // this is a new view to add to the stack
+      // create the new entering view
+      view._setNav(this);
+
+      // give this inserted view an ID
+      view.id = this.id + '-' + (++this._ids);
+
+      // insert the entering view into the correct index in the stack
+      this._views.splice(index, 0, view);
+    }
+  }
+
+  _removeView(view: ViewController) {
+    const views = this._views;
+    const index = views.indexOf(view);
+    if (index > -1) {
+      views.splice(index, 1);
+    }
+  }
+
+  _destroyView(view: ViewController) {
+    view._destroy(this._renderer);
+    this._removeView(view);
   }
 
   /**
@@ -707,7 +714,7 @@ export class NavControllerBase extends Ion implements NavController {
         // this view comes after the active view
         // let's unload it
         this._willUnload(view);
-        view._destroy(this._renderer);
+        this._destroyView(view);
 
       } else if (i < activeViewIndex && !this._isPortal) {
         // this view comes before the active view
@@ -800,9 +807,10 @@ export class NavControllerBase extends Ion implements NavController {
   }
 
   destroy() {
-    for (var i = this._views.length - 1; i >= 0; i--) {
-      this._views[i]._willUnload();
-      this._views[i]._destroy(this._renderer);
+    let view;
+    for (view of this._views) {
+      view._willUnload();
+      view._destroy(this._renderer);
     }
     this._views.length = 0;
 

src/navigation/view-controller.ts

@@ -528,26 +528,25 @@ export class ViewController {
       this._cmp.destroy();
     }
 
-    if (this._nav) {
-      // remove it from the nav
-      const index = this._nav.indexOf(this);
-      if (index > -1) {
-        this._nav._views.splice(index, 1);
-      }
-    }
-
     this._nav = this._cmp = this.instance = this._cntDir = this._cntRef = this._hdrDir = this._ftrDir = this._nb = this._onWillDismiss = null;
   }
 
   /**
    * @private
    */
-  _lifecycleTest(lifecycle: string): boolean | string | Promise<any> {
+  _lifecycleTest(lifecycle: string): boolean | Promise<any> {
     let instance = this.instance;
     let methodName = 'ionViewCan' + lifecycle;
     if (instance && instance[methodName]) {
       try {
-        return instance[methodName]();
+        let result = instance[methodName]();
+        if (result === false) {
+          return false;
+        } else if (result instanceof Promise) {
+          return result;
+        } else {
+          return true;
+        }
 
       } catch (e) {
         console.error(`${this.name} ${methodName} error: ${e.message}`);
@@ -572,7 +571,6 @@ export class ViewController {
 
 }
 
-
 export function isViewController(viewCtrl: any) {
   return !!(viewCtrl && (<ViewController>viewCtrl)._didLoad && (<ViewController>viewCtrl)._willUnload);
 }

src/transitions/transition-controller.ts

@@ -34,7 +34,7 @@ export class TransitionController {
     return this._ids++;
   }
 
-  get(trnsId: number, enteringView: ViewController, leavingView: ViewController, opts: AnimationOptions) {
+  get(trnsId: number, enteringView: ViewController, leavingView: ViewController, opts: AnimationOptions): Transition {
     const trns = createTransition(this._config, opts.animation, enteringView, leavingView, opts);
     trns.trnsId = trnsId;
 

src/transitions/transition-registry.ts

@@ -1,4 +1,5 @@
 import { Config } from '../config/config';
+import { Transition } from './transition';
 import { IOSTransition } from './transition-ios';
 import { MDTransition } from './transition-md';
 import { WPTransition } from './transition-wp';
@@ -62,7 +63,7 @@ export function registerTransitions(config: Config) {
 }
 
 
-export function createTransition(config: Config, transitionName: string, enteringView: any, leavingView: any, opts: any) {
+export function createTransition(config: Config, transitionName: string, enteringView: any, leavingView: any, opts: any): Transition {
   let TransitionClass: any = config.getTransition(transitionName);
   if (!TransitionClass) {
     // didn't find a transition animation, default to ios-transition

src/transitions/transition.ts

@@ -21,10 +21,8 @@ export class Transition extends Animation {
   _trnsStart: Function;
 
   parent: Transition;
-  hasChildTrns: boolean = false;
   trnsId: number;
 
-
   constructor(public enteringView: ViewController, public leavingView: ViewController, opts: AnimationOptions, raf?: Function) {
     super(null, opts, raf);
   }
@@ -35,6 +33,10 @@ export class Transition extends Animation {
     this._trnsStart = trnsStart;
   }
 
+  isRoot(): boolean {
+    return !this.parent;
+  }
+
   start() {
     this._trnsStart && this._trnsStart();
     this._trnsStart = null;

src/util/mock-providers.ts

@@ -275,9 +275,9 @@ export const mockNavController = function(): NavControllerBase {
     enteringView._state = ViewState.INITIALIZED;
   };
 
-  (<any>nav)._orgViewInsert = nav._viewInsert;
+  (<any>nav)._orgViewInsert = nav._viewAttachToDOM;
 
-  nav._viewInsert = function(view: ViewController, componentRef: ComponentRef<any>, viewport: ViewContainerRef) {
+  nav._viewAttachToDOM = function(view: ViewController, componentRef: ComponentRef<any>, viewport: ViewContainerRef) {
     let mockedViewport: any = {
       insert: () => { }
     };