From 5a4c6093a72059577544dafe372e06018ee1ed19 Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Sat, 25 Feb 2017 22:29:24 +0100 Subject: [PATCH] refactor(NavController): adds better error handling fixes #10090 --- src/components/app/app.ts | 2 - src/components/menu/menu-controller.ts | 2 +- .../popover/test/basic/app.module.ts | 14 ++++- src/components/popover/test/basic/main.html | 5 ++ src/navigation/nav-controller-base.ts | 57 ++++++++++++------- src/navigation/nav-util.ts | 7 ++- src/navigation/test/view-controller.spec.ts | 5 +- src/navigation/view-controller.ts | 32 +++++++---- src/util/mock-providers.ts | 4 +- src/util/util.ts | 2 +- 10 files changed, 85 insertions(+), 45 deletions(-) diff --git a/src/components/app/app.ts b/src/components/app/app.ts index 15ceaf33be..5c3fad45f9 100644 --- a/src/components/app/app.ts +++ b/src/components/app/app.ts @@ -220,8 +220,6 @@ export class App { present(enteringView: ViewController, opts: NavOptions, appPortal?: AppPortal): Promise { const portal = this._appRoot._getPortal(appPortal); - enteringView._setNav(portal); - opts.keyboardClose = false; opts.direction = DIRECTION_FORWARD; diff --git a/src/components/menu/menu-controller.ts b/src/components/menu/menu-controller.ts index 6b67dd9dd9..d9fd882907 100644 --- a/src/components/menu/menu-controller.ts +++ b/src/components/menu/menu-controller.ts @@ -313,7 +313,7 @@ export class MenuController { * @private */ _setActiveMenu(menu: Menu) { - assert(menu.enabled); + assert(menu.enabled, 'menu must be enabled'); assert(this._menus.indexOf(menu) >= 0, 'menu is not registered'); // if this menu should be enabled diff --git a/src/components/popover/test/basic/app.module.ts b/src/components/popover/test/basic/app.module.ts index b0a99271fb..4cdd29228e 100644 --- a/src/components/popover/test/basic/app.module.ts +++ b/src/components/popover/test/basic/app.module.ts @@ -1,5 +1,5 @@ import { Component, ViewChild, ElementRef, ViewEncapsulation, NgModule } from '@angular/core'; -import { IonicApp, IonicModule, PopoverController, NavParams, ViewController } from '../../../../../ionic-angular'; +import { IonicApp, IonicModule, PopoverController, NavParams, ToastController, ViewController } from '../../../../../ionic-angular'; @Component({ @@ -188,7 +188,10 @@ export class E2EPage { @ViewChild('popoverContent', {read: ElementRef}) content: ElementRef; @ViewChild('popoverText', {read: ElementRef}) text: ElementRef; - constructor(private popoverCtrl: PopoverController) {} + constructor( + private popoverCtrl: PopoverController, + private toastCtrl: ToastController, + ) { } presentListPopover(ev: UIEvent) { let popover = this.popoverCtrl.create(PopoverListPage); @@ -221,6 +224,13 @@ export class E2EPage { this.popoverCtrl.create(PopoverListPage).present(); } + presentToast() { + this.toastCtrl.create({ + message: 'Toast example', + duration: 1000 + }).present(); + } + } diff --git a/src/components/popover/test/basic/main.html b/src/components/popover/test/basic/main.html index 2aeb0a626a..21a2cebe22 100644 --- a/src/components/popover/test/basic/main.html +++ b/src/components/popover/test/basic/main.html @@ -62,6 +62,11 @@
Aenean rhoncus urna at interdum blandit. Donec ac massa nec libero vehicula tincidunt. Sed sit amet hendrerit risus. Aliquam vitae vestibulum ipsum, non feugiat orci. Vivamus eu rutrum elit. Nulla dapibus tortor non dignissim pretium. Nulla in luctus turpis. Etiam non mattis tortor, at aliquet ex. Nunc ut ante varius, auctor dui vel, volutpat elit. Nunc laoreet augue sit amet ultrices porta. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae; Vestibulum pellentesque lobortis est, ut tincidunt ligula mollis sit amet. In porta risus arcu, quis pellentesque dolor mattis non. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae;
+ + + diff --git a/src/navigation/nav-controller-base.ts b/src/navigation/nav-controller-base.ts index 17de3ac7f7..c379f0a1bb 100644 --- a/src/navigation/nav-controller-base.ts +++ b/src/navigation/nav-controller-base.ts @@ -202,8 +202,8 @@ export class NavControllerBase extends Ion implements NavController { }); } + // ti.resolve() is called when the navigation transition is finished successfully ti.resolve = (hasCompleted: boolean, isAsync: boolean, enteringName: string, leavingName: string, direction: string) => { - // transition has successfully resolved this._trnsId = null; this._init = true; resolve && resolve(hasCompleted, isAsync, enteringName, leavingName, direction); @@ -214,23 +214,22 @@ export class NavControllerBase extends Ion implements NavController { this._nextTrns(); }; - ti.reject = (rejectReason: any, trns: Transition) => { - // rut row raggy, something rejected this transition + // ti.reject() is called when the navigation transition fails. ie. it is rejected at some point. + ti.reject = (rejectReason: any, transition: Transition) => { this._trnsId = null; this._queue.length = 0; - while (trns) { - if (trns.enteringView && (trns.enteringView._state !== ViewState.LOADED)) { - // destroy the entering views and all of their hopes and dreams - this._destroyView(trns.enteringView); + // walk through the transition views so they are destroyed + while (transition) { + var enteringView = transition.enteringView; + if (enteringView && (enteringView._state === ViewState.ATTACHED)) { + this._destroyView(enteringView); } - if (!trns.parent) { + if (transition.isRoot()) { + this._trnsCtrl.destroy(transition.trnsId); break; } - } - - if (trns) { - this._trnsCtrl.destroy(trns.trnsId); + transition = transition.parent; } reject && reject(false, false, rejectReason); @@ -278,7 +277,19 @@ export class NavControllerBase extends Ion implements NavController { return false; } - // Get entering and leaving views + // ensure any of the inserted view are used + const insertViews = ti.insertViews; + if (insertViews) { + for (var i = 0; i < insertViews.length; i++) { + var nav = insertViews[i]._nav; + if (nav && nav !== this || insertViews[i]._state === ViewState.DESTROYED) { + ti.reject('leavingView and enteringView are null. stack is already empty'); + return false; + } + } + } + + // get entering and leaving views const leavingView = this.getActive(); const enteringView = this._getEnteringView(ti, leavingView); @@ -291,7 +302,7 @@ export class NavControllerBase extends Ion implements NavController { this.setTransitioning(true); // Initialize enteringView - if (enteringView && isBlank(enteringView._state)) { + if (enteringView && enteringView._state === ViewState.NEW) { // render the entering view, and all child navs and views // ******** DOM WRITE **************** this._viewInit(enteringView); @@ -303,7 +314,6 @@ export class NavControllerBase extends Ion implements NavController { // views have been initialized, now let's test // to see if the transition is even allowed or not return this._viewTest(enteringView, leavingView, ti); - } else { return this._postViewInit(enteringView, leavingView, ti); } @@ -419,7 +429,6 @@ export class NavControllerBase extends Ion implements NavController { // add the views to the for (i = 0; i < insertViews.length; i++) { view = insertViews[i]; - assert(view, 'view must be non null'); this._insertViewAt(view, ti.insertStart + i); } @@ -477,6 +486,9 @@ export class NavControllerBase extends Ion implements NavController { * DOM WRITE */ _viewInit(enteringView: ViewController) { + assert(enteringView, 'enteringView must be non null'); + assert(enteringView._state === ViewState.NEW, 'enteringView state must be NEW'); + // entering view has not been initialized yet const componentProviders = ReflectiveInjector.resolve([ { provide: NavController, useValue: this }, @@ -501,7 +513,7 @@ export class NavControllerBase extends Ion implements NavController { // render the component ref instance to the DOM // ******** DOM WRITE **************** viewport.insert(componentRef.hostView, viewport.length); - view._state = ViewState.PRE_RENDERED; + view._state = ViewState.ATTACHED; if (view._cssClass) { // the ElementRef of the actual ion-page created @@ -604,14 +616,12 @@ export class NavControllerBase extends Ion implements NavController { } }); - if (enteringView && enteringView._state === ViewState.INITIALIZED) { + if (enteringView && (enteringView._state === ViewState.INITIALIZED)) { // render the entering component in the DOM // this would also render new child navs/views // which may have their very own async canEnter/Leave tests // ******** DOM WRITE **************** this._viewAttachToDOM(enteringView, enteringView._cmp, this._viewport); - } else { - console.debug('enteringView state is not INITIALIZED', enteringView); } if (!transition.hasChildren) { @@ -762,9 +772,10 @@ export class NavControllerBase extends Ion implements NavController { if (existingIndex > -1) { // this view is already in the stack!! // move it to its new location + assert(view._nav === this, 'view is not part of the nav'); this._views.splice(index, 0, this._views.splice(existingIndex, 1)[0]); - } else { + assert(!view._nav, 'nav is used'); // this is a new view to add to the stack // create the new entering view view._setNav(this); @@ -781,6 +792,8 @@ export class NavControllerBase extends Ion implements NavController { } _removeView(view: ViewController) { + assert(view._state === ViewState.ATTACHED || view._state === ViewState.DESTROYED, 'view state should be loaded or destroyed'); + const views = this._views; const index = views.indexOf(view); assert(index > -1, 'view must be part of the stack'); @@ -1056,7 +1069,7 @@ export class NavControllerBase extends Ion implements NavController { dismissPageChangeViews() { for (let view of this._views) { if (view.data && view.data.dismissOnPageChange) { - view.dismiss(); + view.dismiss().catch(null); } } } diff --git a/src/navigation/nav-util.ts b/src/navigation/nav-util.ts index d5e42b2ff2..dffdd2f09b 100644 --- a/src/navigation/nav-util.ts +++ b/src/navigation/nav-util.ts @@ -193,9 +193,10 @@ export interface TransitionInstruction { } export enum ViewState { - INITIALIZED, - PRE_RENDERED, - LOADED, + NEW, // New created ViewController + INITIALIZED, // Initialized by the NavController + ATTACHED, // Loaded to the DOM + DESTROYED // Destroyed by the NavController } export const INIT_ZINDEX = 100; diff --git a/src/navigation/test/view-controller.spec.ts b/src/navigation/test/view-controller.spec.ts index e4ed040d47..5fccae6018 100644 --- a/src/navigation/test/view-controller.spec.ts +++ b/src/navigation/test/view-controller.spec.ts @@ -1,5 +1,5 @@ import { mockNavController, mockView, mockViews } from '../../util/mock-providers'; - +import { ViewState } from '../nav-util'; describe('ViewController', () => { @@ -16,6 +16,7 @@ describe('ViewController', () => { }); // act + viewController._state = ViewState.ATTACHED; viewController._willEnter(); }, 10000); }); @@ -33,6 +34,7 @@ describe('ViewController', () => { }); // act + viewController._state = ViewState.ATTACHED; viewController._didEnter(); }, 10000); }); @@ -50,6 +52,7 @@ describe('ViewController', () => { }); // act + viewController._state = ViewState.ATTACHED; viewController._willLeave(false); }, 10000); }); diff --git a/src/navigation/view-controller.ts b/src/navigation/view-controller.ts index 49a94746cc..5b98ac9025 100644 --- a/src/navigation/view-controller.ts +++ b/src/navigation/view-controller.ts @@ -1,7 +1,7 @@ import { ComponentRef, ElementRef, EventEmitter, Output, Renderer } from '@angular/core'; import { Footer, Header } from '../components/toolbar/toolbar'; -import { isPresent } from '../util/util'; +import { isPresent, assert } from '../util/util'; import { Navbar } from '../components/navbar/navbar'; import { NavController } from './nav-controller'; import { NavOptions, ViewState } from './nav-util'; @@ -46,7 +46,7 @@ export class ViewController { _cmp: ComponentRef; _nav: NavController; _zIndex: number; - _state: ViewState; + _state: ViewState = ViewState.NEW; _cssClass: string; /** @@ -227,7 +227,7 @@ export class ViewController { * @private */ get name(): string { - return this.component ? this.component.name : ''; + return (this.component ? this.component.name : ''); } /** @@ -261,14 +261,12 @@ export class ViewController { // _hidden value of '' means the hidden attribute will be added // _hidden value of null means the hidden attribute will be removed // doing checks to make sure we only update the DOM when actually needed - if (this._cmp) { - // if it should render, then the hidden attribute should not be on the element - if (shouldShow === this._isHidden) { - this._isHidden = !shouldShow; - let value = (shouldShow ? null : ''); - // ******** DOM WRITE **************** - renderer.setElementAttribute(this.pageRef().nativeElement, 'hidden', value); - } + // if it should render, then the hidden attribute should not be on the element + if (this._cmp && shouldShow === this._isHidden) { + this._isHidden = !shouldShow; + let value = (shouldShow ? null : ''); + // ******** DOM WRITE **************** + renderer.setElementAttribute(this.pageRef().nativeElement, 'hidden', value); } } @@ -411,6 +409,7 @@ export class ViewController { } _preLoad() { + assert(this._state === ViewState.INITIALIZED, 'view state must be INITIALIZED'); this._lifecycle('PreLoad'); } @@ -420,6 +419,7 @@ export class ViewController { * This event is fired before the component and his children have been initialized. */ _willLoad() { + assert(this._state === ViewState.INITIALIZED, 'view state must be INITIALIZED'); this._lifecycle('WillLoad'); } @@ -432,6 +432,7 @@ export class ViewController { * recommended method to use when a view becomes active. */ _didLoad() { + assert(this._state === ViewState.ATTACHED, 'view state must be ATTACHED'); this._lifecycle('DidLoad'); } @@ -440,6 +441,8 @@ export class ViewController { * The view is about to enter and become the active view. */ _willEnter() { + assert(this._state === ViewState.ATTACHED, 'view state must be ATTACHED'); + if (this._detached && this._cmp) { // ensure this has been re-attached to the change detector this._cmp.changeDetectorRef.reattach(); @@ -456,6 +459,8 @@ export class ViewController { * will fire, whether it was the first load or loaded from the cache. */ _didEnter() { + assert(this._state === ViewState.ATTACHED, 'view state must be ATTACHED'); + this._nb && this._nb.didEnter(); this.didEnter.emit(null); this._lifecycle('DidEnter'); @@ -510,6 +515,8 @@ export class ViewController { * DOM WRITE */ _destroy(renderer: Renderer) { + assert(this._state !== ViewState.DESTROYED, 'view state must be ATTACHED'); + if (this._cmp) { if (renderer) { // ensure the element is cleaned up for when the view pool reuses this element @@ -524,6 +531,7 @@ export class ViewController { } this._nav = this._cmp = this.instance = this._cntDir = this._cntRef = this._leavingOpts = this._hdrDir = this._ftrDir = this._nb = this._onDidDismiss = this._onWillDismiss = null; + this._state = ViewState.DESTROYED; } /** @@ -534,7 +542,7 @@ export class ViewController { const methodName = 'ionViewCan' + lifecycle; if (instance && instance[methodName]) { try { - let result = instance[methodName](); + var result = instance[methodName](); if (result === false) { return false; } else if (result instanceof Promise) { diff --git a/src/util/mock-providers.ts b/src/util/mock-providers.ts index 00223ec668..1bd1d33c69 100644 --- a/src/util/mock-providers.ts +++ b/src/util/mock-providers.ts @@ -348,7 +348,9 @@ export function mockView(component?: any, data?: any) { export function mockViews(nav: NavControllerBase, views: ViewController[]) { nav._views = views; - views.forEach(v => v._setNav(nav)); + views.forEach(v => { + v._setNav(nav); + }); } export function mockComponentRef(): ComponentRef { diff --git a/src/util/util.ts b/src/util/util.ts index bfbe21c6f1..b2fb6488e8 100644 --- a/src/util/util.ts +++ b/src/util/util.ts @@ -170,7 +170,7 @@ function _runInDev(fn: Function) { /** @private */ -function _assert(actual: any, reason?: string) { +function _assert(actual: any, reason: string) { if (!actual && ASSERT_ENABLED === true) { let message = 'IONIC ASSERT: ' + reason; console.error(message);