From 1f06be4a31965f2a949b4866a585aee6af0af29d Mon Sep 17 00:00:00 2001 From: Sean Perkins Date: Wed, 28 Jun 2023 11:27:25 -0400 Subject: [PATCH] fix(nav): root component is mounted with root params (#27676) Issue number: Resolves #27146 --------- ## What is the current behavior? When an `ion-nav` is presented multiple times in an overlay, the user's `root` component will attempt to be attached twice. This results in the view being mounted without the `rootParams` being defined, causing an exception in a user's application. This behavior occurs due to the `root` watch callback firing twice. The second time the `ion-nav` is presented in an overlay, the watch callback will execute _before_ `componentDidLoad` fires. This results in the watch callback firing twice, once from the underlying change detection and the second time from [manually calling the function](https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/nav/nav.tsx#L115) from `componentDidLoad`. ## What is the new behavior? - `ion-nav` includes a new flag to track when `componentDidLoad` executes. This allows us to prevent the behavior of the `rootChanged` callback from happening when the component has not loaded. - `ion-nav` consistently attaches the `rootParams` to the `root` component. ## Does this introduce a breaking change? - [ ] Yes - [x] No ## Other information Dev-build: `7.0.15-dev.11687924603.10a1e477`. ### How to test You can install against the reproduction app in the linked issue to verify the behavior before and after. - Before, the app will throw an exception when presenting the modal the second time. - After, the app will not throw an exception when presenting the modal the second time. Information that you fill out on the main screen form will be rendered inside the modal content. --- .../ng16/e2e/src/modal-nav-params.spec.ts | 20 +++++++++ .../modal-nav-params.component.ts | 45 +++++++++++++++++++ .../modal-nav-params/nav-root.component.ts | 38 ++++++++++++++++ .../version-test-routing.module.ts | 7 ++- core/src/components/nav/nav.tsx | 26 +++++++++-- 5 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 angular/test/apps/ng16/e2e/src/modal-nav-params.spec.ts create mode 100644 angular/test/apps/ng16/src/app/version-test/modal-nav-params/modal-nav-params.component.ts create mode 100644 angular/test/apps/ng16/src/app/version-test/modal-nav-params/nav-root.component.ts diff --git a/angular/test/apps/ng16/e2e/src/modal-nav-params.spec.ts b/angular/test/apps/ng16/e2e/src/modal-nav-params.spec.ts new file mode 100644 index 0000000000..6a5556518f --- /dev/null +++ b/angular/test/apps/ng16/e2e/src/modal-nav-params.spec.ts @@ -0,0 +1,20 @@ +describe('Modal Nav Params', () => { + + beforeEach(() => { + cy.visit('/version-test/modal-nav-params'); + }); + + it('should assign the rootParams when presented in a modal multiple times', () => { + cy.contains('Open Modal').click(); + cy.get('ion-modal').should('exist').should('be.visible'); + cy.get('ion-modal').contains('OK'); + + cy.contains("Close").click(); + cy.get('ion-modal').should('not.be.visible'); + + cy.contains('Open Modal').click(); + cy.get('ion-modal').should('exist').should('be.visible'); + cy.get('ion-modal').contains('OK').should('exist'); + }); + +}); diff --git a/angular/test/apps/ng16/src/app/version-test/modal-nav-params/modal-nav-params.component.ts b/angular/test/apps/ng16/src/app/version-test/modal-nav-params/modal-nav-params.component.ts new file mode 100644 index 0000000000..5f820e1be4 --- /dev/null +++ b/angular/test/apps/ng16/src/app/version-test/modal-nav-params/modal-nav-params.component.ts @@ -0,0 +1,45 @@ +import { Component } from "@angular/core"; + +import { IonicModule } from "@ionic/angular"; + +import { NavRootComponent } from "./nav-root.component"; + +@Component({ + selector: 'app-modal-nav-params', + template: ` + + + Modal Nav Params + + + + Open Modal + + + + + + Close + + + + + + + + + + `, + standalone: true, + imports: [IonicModule, NavRootComponent] +}) +export class ModalNavParamsComponent { + + root = NavRootComponent; + rootParams = { + params: { + id: 123 + } + }; + +} diff --git a/angular/test/apps/ng16/src/app/version-test/modal-nav-params/nav-root.component.ts b/angular/test/apps/ng16/src/app/version-test/modal-nav-params/nav-root.component.ts new file mode 100644 index 0000000000..ae545093dc --- /dev/null +++ b/angular/test/apps/ng16/src/app/version-test/modal-nav-params/nav-root.component.ts @@ -0,0 +1,38 @@ +import { JsonPipe } from "@angular/common"; +import { Component } from "@angular/core"; + +import { IonicModule } from "@ionic/angular"; + +/** + * This is used to track if any occurences of + * the ion-nav root component being attached to + * the DOM result in the rootParams not being + * assigned to the component instance. + * + * https://github.com/ionic-team/ionic-framework/issues/27146 + */ +let rootParamsException = false; + +@Component({ + selector: 'app-modal-content', + template: ` + {{ hasException ? 'ERROR' : 'OK' }} + `, + standalone: true, + imports: [IonicModule, JsonPipe] +}) +export class NavRootComponent { + + params: any; + + ngOnInit() { + if (this.params === undefined) { + rootParamsException = true; + } + } + + get hasException() { + return rootParamsException; + } + +} diff --git a/angular/test/apps/ng16/src/app/version-test/version-test-routing.module.ts b/angular/test/apps/ng16/src/app/version-test/version-test-routing.module.ts index be335bd1af..7cd8857683 100644 --- a/angular/test/apps/ng16/src/app/version-test/version-test-routing.module.ts +++ b/angular/test/apps/ng16/src/app/version-test/version-test-routing.module.ts @@ -3,7 +3,12 @@ import { RouterModule } from "@angular/router"; @NgModule({ imports: [ - RouterModule.forChild([]) + RouterModule.forChild([ + { + path: 'modal-nav-params', + loadComponent: () => import('./modal-nav-params/modal-nav-params.component').then(m => m.ModalNavParamsComponent) + } + ]) ], exports: [RouterModule] }) diff --git a/core/src/components/nav/nav.tsx b/core/src/components/nav/nav.tsx index 733428e9f6..d6290dee71 100644 --- a/core/src/components/nav/nav.tsx +++ b/core/src/components/nav/nav.tsx @@ -2,6 +2,7 @@ import type { EventEmitter } from '@stencil/core'; import { Build, Component, Element, Event, Method, Prop, Watch, h } from '@stencil/core'; import { getTimeGivenProgression } from '@utils/animation/cubic-bezier'; import { assert } from '@utils/helpers'; +import { printIonWarning } from '@utils/logging'; import type { TransitionOptions } from '@utils/transition'; import { lifecycle, setPageHidden, transition } from '@utils/transition'; @@ -36,6 +37,7 @@ export class Nav implements NavOutlet { private destroyed = false; private views: ViewController[] = []; private gesture?: Gesture; + private didLoad = false; @Element() el!: HTMLElement; @@ -77,12 +79,25 @@ export class Nav implements NavOutlet { @Watch('root') rootChanged() { const isDev = Build.isDev; - if (this.root !== undefined) { - if (!this.useRouter) { + + if (this.root === undefined) { + return; + } + + if (this.didLoad === false) { + /** + * If the component has not loaded yet, we can skip setting up the root component. + * It will be called when `componentDidLoad` fires. + */ + return; + } + + if (!this.useRouter) { + if (this.root !== undefined) { this.setRoot(this.root, this.rootParams); - } else if (isDev) { - console.warn(' does not support a root attribute when using ion-router.'); } + } else if (isDev) { + printIonWarning(' does not support a root attribute when using ion-router.', this.el); } } @@ -112,6 +127,9 @@ export class Nav implements NavOutlet { } async componentDidLoad() { + // We want to set this flag before any watch callbacks are manually called + this.didLoad = true; + this.rootChanged(); this.gesture = (await import('../../utils/gesture/swipe-back')).createSwipeBackGesture(