From 77a0640e92dc4300b9e89221ef03e96206eca9ae Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 9 Jan 2024 09:17:01 -0500 Subject: [PATCH] perf(angular): views are not moved on mount (#28544) Issue number: resolves #28534 --------- ## What is the current behavior? Page views in Ionic need to be rendered as a child of `ion-router-outlet` in order for page transitions and swipe to go back to function correctly. However, Angular always inserts components as siblings of the insertion point. Previously, the insertion point was `ion-router-outlet` (the host element). This meant that page views were mounted as siblings of `ion-router-outlet`. We then added code to move the component inside of `ion-router-outlet`. This caused two issues: 1. A DOM tree mismatch during hydration (the linked issue) because hydration is expecting the page view to be a sibling of the router outlet, but Ionic moves the view around in the DOM. 2. A performance issue where all components effectively have `connectedCallback` fired twice. This callback runs when the component is added to the DOM. On initial mount, `connectedCallback` for each component runs. Once the page view is moved, the elements are removed from the DOM (thus causing `disconnectedCallback` to run), and then added to the correct location in the DOM which causes `connectedCallback` to run again. ## What is the new behavior? - IonRouterOutlet now renders a `ng-container`. This appears as a comment in the DOM inside of `ion-router-outlet`. This comment is used as the injection point for adding new views. The new views are added as siblings of the comment, but since the comment is inside of `ion-router-outlet` then the views themselves are inside of `ion-router-outlet` too. ## Does this introduce a breaking change? - [ ] Yes - [x] No This doesn't cause any known breaking changes. However, the placement of views is pretty critical to the functionality of Ionic, so I wanted to ship this in a major release so we have a solid public testing period before the code is considered stable. We already have test coverage that verifies page views are mounted in the correct order, so I did not add more tests for this. ## Other information Dev build: 7.6.2-dev.11704390532.1202188d Testing: 1. Clone and install dependencies for https://github.com/bashoogzaad/ionic-ssr-test 2. Run `npm run dev:ssr`. 3. Open app in a browser. Observe that error noted in https://github.com/ionic-team/ionic-framework/issues/28534#issue-1995002926 appears. 4. Install dev build. 5. Run `npm run dev:ssr`. Observe that the error noted in the original issue does not appear. Note: The Angular SSR package does not support Web Components. As a result, there are other errors you will encounter. However, I still think it's worth fixing this issue a) in the event that the Angular SSR package adds support for Web Components and b) to get the performance gain of not having to re-mount components. --- .../directives/navigation/router-outlet.ts | 13 ++++++++-- .../directives/navigation/stack-controller.ts | 3 --- .../navigation/ion-router-outlet.ts | 26 ++++++++++++++++--- .../src/navigation/router-outlet.ts | 26 ++++++++++++++++--- 4 files changed, 57 insertions(+), 11 deletions(-) diff --git a/packages/angular/common/src/directives/navigation/router-outlet.ts b/packages/angular/common/src/directives/navigation/router-outlet.ts index d0d6bb600c..556e49d606 100644 --- a/packages/angular/common/src/directives/navigation/router-outlet.ts +++ b/packages/angular/common/src/directives/navigation/router-outlet.ts @@ -116,6 +116,7 @@ export class IonRouterOutlet implements OnDestroy, OnInit { router: Router, zone: NgZone, activatedRoute: ActivatedRoute, + protected outletContent: ViewContainerRef, @SkipSelf() @Optional() readonly parentOutlet?: IonRouterOutlet ) { this.nativeEl = elementRef.nativeElement; @@ -276,8 +277,16 @@ export class IonRouterOutlet implements OnDestroy, OnInit { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const component = snapshot.routeConfig!.component ?? snapshot.component; - cmpRef = this.activated = this.location.createComponent(component, { - index: this.location.length, + /** + * View components need to be added as a child of ion-router-outlet + * for page transitions and swipe to go back. + * However, createComponent mounts components as siblings of the + * ViewContainerRef. As a result, outletContent must reference + * an ng-container inside of ion-router-outlet and not + * ion-router-outlet itself. + */ + cmpRef = this.activated = this.outletContent.createComponent(component, { + index: this.outletContent.length, injector, environmentInjector: environmentInjector ?? this.environmentInjector, }); diff --git a/packages/angular/common/src/directives/navigation/stack-controller.ts b/packages/angular/common/src/directives/navigation/stack-controller.ts index 8241afa864..0593219d3f 100644 --- a/packages/angular/common/src/directives/navigation/stack-controller.ts +++ b/packages/angular/common/src/directives/navigation/stack-controller.ts @@ -274,9 +274,6 @@ export class StackController { if (enteringEl && enteringEl !== leavingEl) { enteringEl.classList.add('ion-page'); enteringEl.classList.add('ion-page-invisible'); - if (enteringEl.parentElement !== containerEl) { - containerEl.appendChild(enteringEl); - } if ((containerEl as any).commit) { return containerEl.commit(enteringEl, leavingEl, { diff --git a/packages/angular/src/directives/navigation/ion-router-outlet.ts b/packages/angular/src/directives/navigation/ion-router-outlet.ts index c84f5ce23b..52cc2ebe62 100644 --- a/packages/angular/src/directives/navigation/ion-router-outlet.ts +++ b/packages/angular/src/directives/navigation/ion-router-outlet.ts @@ -1,13 +1,32 @@ import { Location } from '@angular/common'; -import { Directive, Attribute, Optional, SkipSelf, ElementRef, NgZone } from '@angular/core'; +import { + ViewChild, + ViewContainerRef, + Component, + Attribute, + Optional, + SkipSelf, + ElementRef, + NgZone, +} from '@angular/core'; import { Router, ActivatedRoute } from '@angular/router'; import { IonRouterOutlet as IonRouterOutletBase } from '@ionic/angular/common'; -@Directive({ +@Component({ selector: 'ion-router-outlet', + template: '', }) // eslint-disable-next-line @angular-eslint/directive-class-suffix export class IonRouterOutlet extends IonRouterOutletBase { + /** + * `static: true` must be set so the query results are resolved + * before change detection runs. Otherwise, the view container + * ref will be ion-router-outlet instead of ng-container, and + * the first view will be added as a sibling of ion-router-outlet + * instead of a child. + */ + @ViewChild('outletContent', { read: ViewContainerRef, static: true }) outletContent: ViewContainerRef; + /** * We need to pass in the correct instance of IonRouterOutlet * otherwise parentOutlet will be null in a nested outlet context. @@ -22,8 +41,9 @@ export class IonRouterOutlet extends IonRouterOutletBase { router: Router, zone: NgZone, activatedRoute: ActivatedRoute, + outletContent: ViewContainerRef, @SkipSelf() @Optional() readonly parentOutlet?: IonRouterOutlet ) { - super(name, tabs, commonLocation, elementRef, router, zone, activatedRoute, parentOutlet); + super(name, tabs, commonLocation, elementRef, router, zone, activatedRoute, outletContent, parentOutlet); } } diff --git a/packages/angular/standalone/src/navigation/router-outlet.ts b/packages/angular/standalone/src/navigation/router-outlet.ts index d5e6becab2..5919462b82 100644 --- a/packages/angular/standalone/src/navigation/router-outlet.ts +++ b/packages/angular/standalone/src/navigation/router-outlet.ts @@ -1,5 +1,14 @@ import { Location } from '@angular/common'; -import { Directive, Attribute, Optional, SkipSelf, ElementRef, NgZone } from '@angular/core'; +import { + ViewChild, + ViewContainerRef, + Component, + Attribute, + Optional, + SkipSelf, + ElementRef, + NgZone, +} from '@angular/core'; import { Router, ActivatedRoute } from '@angular/router'; import { IonRouterOutlet as IonRouterOutletBase, ProxyCmp } from '@ionic/angular/common'; import { defineCustomElement } from '@ionic/core/components/ion-router-outlet.js'; @@ -7,12 +16,22 @@ import { defineCustomElement } from '@ionic/core/components/ion-router-outlet.js @ProxyCmp({ defineCustomElementFn: defineCustomElement, }) -@Directive({ +@Component({ selector: 'ion-router-outlet', standalone: true, + template: '', }) // eslint-disable-next-line @angular-eslint/directive-class-suffix export class IonRouterOutlet extends IonRouterOutletBase { + /** + * `static: true` must be set so the query results are resolved + * before change detection runs. Otherwise, the view container + * ref will be ion-router-outlet instead of ng-container, and + * the first view will be added as a sibling of ion-router-outlet + * instead of a child. + */ + @ViewChild('outletContent', { read: ViewContainerRef, static: true }) outletContent: ViewContainerRef; + /** * We need to pass in the correct instance of IonRouterOutlet * otherwise parentOutlet will be null in a nested outlet context. @@ -27,8 +46,9 @@ export class IonRouterOutlet extends IonRouterOutletBase { router: Router, zone: NgZone, activatedRoute: ActivatedRoute, + outletContent: ViewContainerRef, @SkipSelf() @Optional() readonly parentOutlet?: IonRouterOutlet ) { - super(name, tabs, commonLocation, elementRef, router, zone, activatedRoute, parentOutlet); + super(name, tabs, commonLocation, elementRef, router, zone, activatedRoute, outletContent, parentOutlet); } }