Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(angular): views are not moved on mount #28544

Merged
merged 9 commits into from
Jan 9, 2024
Merged

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Nov 16, 2023

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
  • 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 bug: angular, moving elements inside of ion-router-outlet causes dom tree mismatch during hydration #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.

@github-actions github-actions bot added the package: angular @ionic/angular package label Nov 16, 2023
@liamdebeasi liamdebeasi changed the title fix(angular): avoid dom manipulation when mounting components perf(angular): views are not moved on mount Jan 4, 2024
@liamdebeasi liamdebeasi marked this pull request as ready for review January 4, 2024 18:14
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, verified that it fixes the issue. Suggestions given.

cmpRef = this.activated = this.location.createComponent(component, {
index: this.location.length,
/**
* View components need to be added as a child of ion-router-outlet.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it mention that it needs to be added as a child due to animations? It could make it easier for future debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Added in d32f66e

@liamdebeasi liamdebeasi merged commit 77a0640 into feature-8.0 Jan 9, 2024
44 checks passed
@liamdebeasi liamdebeasi deleted the FW-5609 branch January 9, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants