Skip to content

Commit

Permalink
perf(angular): views are not moved on mount (#28544)
Browse files Browse the repository at this point in the history
Issue number: resolves #28534

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

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?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- 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.

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

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
#28534 (comment)
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.
  • Loading branch information
liamdebeasi committed Jan 9, 2024
1 parent 15e368c commit 77a0640
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 11 deletions.
13 changes: 11 additions & 2 deletions packages/angular/common/src/directives/navigation/router-outlet.ts
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
});
Expand Down
Expand Up @@ -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, {
Expand Down
26 changes: 23 additions & 3 deletions 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: '<ng-container #outletContent><ng-content></ng-content></ng-container>',
})
// 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.
Expand All @@ -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);
}
}
26 changes: 23 additions & 3 deletions packages/angular/standalone/src/navigation/router-outlet.ts
@@ -1,18 +1,37 @@
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';

@ProxyCmp({
defineCustomElementFn: defineCustomElement,
})
@Directive({
@Component({
selector: 'ion-router-outlet',
standalone: true,
template: '<ng-container #outletContent><ng-content></ng-content></ng-container>',
})
// 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.
Expand All @@ -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);
}
}

0 comments on commit 77a0640

Please sign in to comment.