fix(router): transition race condition

fixes #14873
fixes #15090

angular/src/directives/navigation/ion-router-outlet.ts

@@ -162,11 +162,11 @@ export class IonRouterOutlet implements OnDestroy, OnInit {
 }
 
 function emitEvent(el: HTMLElement) {
-  const event = new CustomEvent('ionRouterOutletActivated', {
+  const ev = new CustomEvent('ionRouterOutletActivated', {
     bubbles: true,
     cancelable: true,
   });
-  el.dispatchEvent(event);
+  el.dispatchEvent(ev);
 }
 
 class OutletInjector implements Injector {

angular/src/directives/navigation/router-controller.ts

@@ -39,7 +39,6 @@ export class StackController {
     const leavingView = this.getActive();
     this.insertView(enteringView, direction);
     await this.transition(enteringView, leavingView, direction, animated, this.canGoBack(1));
-
     this.cleanup();
   }
 

angular/src/ionic-module.ts

@@ -1,4 +1,5 @@
 import { APP_INITIALIZER, ModuleWithProviders, NgModule } from '@angular/core';
+import { IonicConfig } from '@ionic/core';
 import { CommonModule } from '@angular/common';
 
 import { appInitialize } from './app-initialize';
@@ -133,7 +134,7 @@ const PROVIDERS = [
   imports: [CommonModule]
 })
 export class IonicModule {
-  static forRoot(config?: { [key: string]: any }): ModuleWithProviders {
+  static forRoot(config?: IonicConfig): ModuleWithProviders {
     return {
       ngModule: IonicModule,
       providers: [

angular/src/providers/config.ts

@@ -1,11 +1,11 @@
-import { Config as CoreConfig } from '@ionic/core';
+import { Config as CoreConfig, IonicConfig } from '@ionic/core';
 import { InjectionToken } from '@angular/core';
 import { IonicWindow } from '../types/interfaces';
 
 
 export class Config {
 
-  get(key: string, fallback?: any): any {
+  get(key: keyof IonicConfig, fallback?: any): any {
     const c = getConfig();
     if (c) {
       return c.get(key, fallback);
@@ -13,7 +13,7 @@ export class Config {
     return null;
   }
 
-  getBoolean(key: string, fallback?: boolean): boolean {
+  getBoolean(key: keyof IonicConfig, fallback?: boolean): boolean {
     const c = getConfig();
     if (c) {
       return c.getBoolean(key, fallback);
@@ -21,7 +21,7 @@ export class Config {
     return false;
   }
 
-  getNumber(key: string, fallback?: number): number {
+  getNumber(key: keyof IonicConfig, fallback?: number): number {
     const c = getConfig();
     if (c) {
       return c.getNumber(key, fallback);
@@ -29,7 +29,7 @@ export class Config {
     return 0;
   }
 
-  set(key: string, value?: any) {
+  set(key: keyof IonicConfig, value?: any) {
     const c = getConfig();
     if (c) {
       c.set(key, value);

core/src/components/router-outlet/route-outlet.tsx

@@ -11,9 +11,9 @@ import { attachComponent, detachComponent } from '../../utils/framework-delegate
 })
 export class RouterOutlet implements NavOutlet {
 
-  private isTransitioning = false;
   private activeEl: HTMLElement | undefined;
   private activeComponent: any;
+  private waitPromise?: Promise<void>;
 
   mode!: Mode;
 
@@ -36,7 +36,6 @@ export class RouterOutlet implements NavOutlet {
     if (this.animated === undefined) {
       this.animated = this.config.getBoolean('animate', true);
     }
-
     this.ionNavWillLoad.emit();
   }
 
@@ -49,20 +48,19 @@ export class RouterOutlet implements NavOutlet {
    */
   @Method()
   async setRoot(component: ComponentRef, params?: ComponentProps, opts?: RouterOutletOptions): Promise<boolean> {
-    if (this.isTransitioning || this.activeComponent === component) {
+    if (this.activeComponent === component) {
       return false;
     }
-    this.activeComponent = component;
 
     // attach entering view to DOM
-    const enteringEl = await attachComponent(this.delegate, this.el, component, ['ion-page', 'ion-page-invisible'], params);
     const leavingEl = this.activeEl;
+    const enteringEl = await attachComponent(this.delegate, this.el, component, ['ion-page', 'ion-page-invisible'], params);
+
+    this.activeComponent = component;
+    this.activeEl = enteringEl;
 
     // commit animation
     await this.commit(enteringEl, leavingEl, opts);
-
-    // remove leaving view
-    this.activeEl = enteringEl;
     detachComponent(this.delegate, leavingEl);
 
     return true;
@@ -71,35 +69,15 @@ export class RouterOutlet implements NavOutlet {
   /** @hidden */
   @Method()
   async commit(enteringEl: HTMLElement, leavingEl: HTMLElement | undefined, opts?: RouterOutletOptions): Promise<boolean> {
-    // isTransitioning acts as a lock to prevent reentering
-    if (this.isTransitioning || leavingEl === enteringEl) {
-      return false;
+    const unlock = await this.lock();
+    let changed = false;
+    try {
+      changed = await this.transition(enteringEl, leavingEl, opts);
+    } catch (e) {
+      console.error(e);
     }
-    this.isTransitioning = true;
-
-    // emit nav will change event
-    this.ionNavWillChange.emit();
-
-    opts = opts || {};
-
-    const { mode, queue, animated, animationCtrl, win, el } = this;
-    await transition({
-      mode,
-      queue,
-      animated,
-      animationCtrl,
-      window: win,
-      enteringEl,
-      leavingEl,
-      baseEl: el,
-
-      ...opts
-    });
-    this.isTransitioning = false;
-
-    // emit nav changed event
-    this.ionNavDidChange.emit();
-    return true;
+    unlock();
+    return changed;
   }
 
   /** @hidden */
@@ -125,6 +103,48 @@ export class RouterOutlet implements NavOutlet {
     } : undefined;
   }
 
+  private async lock() {
+    const p = this.waitPromise;
+    let resolve!: () => void;
+    this.waitPromise = new Promise(r => resolve = r);
+
+    if (p) {
+      await p;
+    }
+    return resolve;
+  }
+
+  async transition(enteringEl: HTMLElement, leavingEl: HTMLElement | undefined, opts?: RouterOutletOptions): Promise<boolean> {
+    // isTransitioning acts as a lock to prevent reentering
+    if (leavingEl === enteringEl) {
+      return false;
+    }
+
+    // emit nav will change event
+    this.ionNavWillChange.emit();
+
+    opts = opts || {};
+
+    const { mode, queue, animated, animationCtrl, win, el } = this;
+    await transition({
+      mode,
+      queue,
+      animated,
+      animationCtrl,
+      window: win,
+      enteringEl,
+      leavingEl,
+      baseEl: el,
+
+      ...opts
+    });
+
+    // emit nav changed event
+    this.ionNavDidChange.emit();
+
+    return true;
+  }
+
   render() {
     return [
       this.mode === 'ios' && <div class="nav-decor"/>,

core/src/components/router-outlet/test/basic/index.html

@@ -75,7 +75,28 @@
       <ion-route url="/two" component="page-two"> </ion-route>
       <ion-route url="/page-3" component="page-three"> </ion-route>
     </ion-router>
-    <ion-router-outlet></ion-router-outlet>
+
+    <ion-split-pane>
+      <ion-menu>
+        <ion-header>
+          <ion-toolbar>
+            <ion-title>Menu</ion-title>
+          </ion-toolbar>
+        </ion-header>
+        <ion-content>
+          <ion-item href="#/">
+            <ion-label>Page 1</ion-label>
+          </ion-item>
+          <ion-item href="#/two">
+            <ion-label>Page 2</ion-label>
+          </ion-item>
+          <ion-item href="#/page-3">
+            <ion-label>Page 3</ion-label>
+          </ion-item>
+        </ion-content>
+      </ion-menu>
+      <ion-router-outlet main></ion-router-outlet>
+    </ion-split-pane>
   </ion-app>
 </body>
 

core/src/components/router/router.tsx

@@ -19,6 +19,7 @@ export class Router {
   private busy = false;
   private state = 0;
   private lastState = 0;
+  private waitPromise?: Promise<void>;
 
   @Element() el!: HTMLElement;
 
@@ -82,10 +83,10 @@ export class Router {
   /** Navigate to the specified URL */
   @Method()
   push(url: string, direction: RouterDirection = 'forward') {
-    const path = parsePath(url);
-    const intent = DIRECTION_TO_INTENT[direction];
     console.debug('[ion-router] URL pushed -> updating nav', url, direction);
 
+    const path = parsePath(url);
+    const intent = DIRECTION_TO_INTENT[direction];
     this.setPath(path, intent);
     return this.writeNavStateRoot(path, intent);
   }
@@ -103,6 +104,7 @@ export class Router {
   @Method()
   async navChanged(intent: number): Promise<boolean> {
     if (this.busy) {
+      console.warn('[ion-router] router is busy, navChanged was cancelled');
       return false;
     }
     const { ids, outlet } = readNavState(this.win.document.body);
@@ -122,7 +124,7 @@ export class Router {
     console.debug('[ion-router] nav changed -> update URL', ids, path);
     this.setPath(path, intent);
 
-    await this.writeNavState(outlet, chain, RouterIntent.None, path, null, ids.length);
+    await this.safeWriteNavState(outlet, chain, RouterIntent.None, path, null, ids.length);
     return true;
   }
 
@@ -157,9 +159,6 @@ export class Router {
   }
 
   private async writeNavStateRoot(path: string[] | null, intent: RouterIntent): Promise<boolean> {
-    if (this.busy) {
-      return false;
-    }
     if (!path) {
       console.error('[ion-router] URL is not part of the routing set');
       return false;
@@ -184,7 +183,34 @@ export class Router {
     }
 
     // write DOM give
-    return this.writeNavState(this.win.document.body, chain, intent, path, redirectFrom);
+    return this.safeWriteNavState(this.win.document.body, chain, intent, path, redirectFrom);
+  }
+
+  private async safeWriteNavState(
+    node: HTMLElement | undefined, chain: RouteChain, intent: RouterIntent,
+    path: string[], redirectFrom: string[] | null,
+    index = 0
+  ): Promise<boolean> {
+    const unlock = await this.lock();
+    let changed = false;
+    try {
+      changed = await this.writeNavState(node, chain, intent, path, redirectFrom, index);
+    } catch (e) {
+      console.error(e);
+    }
+    unlock();
+    return changed;
+  }
+
+  private async lock() {
+    const p = this.waitPromise;
+    let resolve!: () => void;
+    this.waitPromise = new Promise(r => resolve = r);
+
+    if (p) {
+      await p;
+    }
+    return resolve;
   }
 
   private async writeNavState(
@@ -193,6 +219,7 @@ export class Router {
     index = 0
   ): Promise<boolean> {
     if (this.busy) {
+      console.warn('[ion-router] router is busy, transition was cancelled');
       return false;
     }
     this.busy = true;

core/src/global/config.ts

@@ -1,5 +1,12 @@
+import { Mode } from '../interface';
 
 export interface IonicConfig {
+  /**
+   * The mode determines which platform styles to use.
+   * Possible values are: `"ios"` or `"md"`.
+   */
+  mode?: Mode;
+
   isDevice?: boolean;
   statusbarPadding?: boolean;
   inputShims?: boolean;
@@ -12,7 +19,6 @@ export interface IonicConfig {
   pickerSpinner?: string;
   refreshingIcon?: string;
   refreshingSpinner?: string;
-  mode?: string;
   menuType?: string;
   scrollPadding?: string;
   inputBlurring?: string;

core/src/utils/transition.ts

@@ -70,6 +70,8 @@ async function animation(animationBuilder: AnimationBuilder, opts: TransitionOpt
   fireWillEvents(opts.window, opts.enteringEl, opts.leavingEl);
   await playTransition(trns, opts);
 
+  markVisible(opts);
+
   if (trns.hasCompleted) {
     fireDidEvents(opts.window, opts.enteringEl, opts.leavingEl);
   }
@@ -79,17 +81,25 @@ async function animation(animationBuilder: AnimationBuilder, opts: TransitionOpt
 async function noAnimation(opts: TransitionOptions): Promise<null> {
   const enteringEl = opts.enteringEl;
   const leavingEl = opts.leavingEl;
+
+  await waitForReady(opts, false);
+
+  markVisible(opts);
+
+  fireWillEvents(opts.window, enteringEl, leavingEl);
+  fireDidEvents(opts.window, enteringEl, leavingEl);
+  return null;
+}
+
+async function markVisible(opts: TransitionOptions) {
+  const enteringEl = opts.enteringEl;
+  const leavingEl = opts.leavingEl;
   if (enteringEl) {
     enteringEl.classList.remove('ion-page-invisible');
   }
   if (leavingEl) {
     leavingEl.classList.remove('ion-page-invisible');
   }
-  await waitForReady(opts, false);
-
-  fireWillEvents(opts.window, enteringEl, leavingEl);
-  fireDidEvents(opts.window, enteringEl, leavingEl);
-  return null;
 }
 
 async function waitForReady(opts: TransitionOptions, defaultDeep: boolean) {