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

bug: angular, moving elements inside of ion-router-outlet causes dom tree mismatch during hydration #28534

Closed
3 tasks done
bashoogzaad opened this issue Nov 15, 2023 · 15 comments
Labels
package: angular @ionic/angular package type: bug a confirmed bug report

Comments

@bashoogzaad
Copy link

bashoogzaad commented Nov 15, 2023

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

When starting a new project with ionic start with the option Angular Standalone, then @angular/ssr is not working correctly. When adding @angular/ssr, the Angular CLI is adding files, such as app.config.server.ts. This file is referring to app.config.ts. This file is however not present, so you have to create is. Not a big deal.

However, after npm run dev:serve, the application is running, but gets an error:

main.ts:12 ERROR Error: NG0501: During hydration Angular expected more sibling nodes to be present.

Actual DOM is:

<ion-app>
  …
  <ion-router-outlet>…</ion-router-outlet>
  <!-- container -->  <-- AT THIS LOCATION
</ion-app>

To fix this problem:
  * check corresponding component for hydration-related issues
  * check to see if your template has valid HTML structure
  * or skip hydration by adding the 'ngSkipHydration' attribute to its host node in a template

 Find more at https://angular.io/errors/NG0501
    at validateSiblingNodeExists (core.mjs:18669:15)
    at siblingAfter (core.mjs:19187:22)
    at locateDehydratedViewsInContainer (core.mjs:19395:32)
    at populateDehydratedViewsInLContainerImpl (core.mjs:20174:44)
    at locateOrCreateAnchorNode (core.mjs:20189:10)
    at createContainerRef (core.mjs:20086:5)
    at injectViewContainerRef (core.mjs:19846:12)
    at core.mjs:4188:29
    at runInInjectorProfilerContext (core.mjs:867:9)
    at lookupTokenUsingNodeInjector (core.mjs:4187:17)

This is just a starter project with 'list' prefill.

Expected Behavior

The application should run correctly, and must be able to hydrate the components.

Steps to Reproduce

  1. ionic start -> Angular Standalone -> list prefill
  2. ng add @angular/ssr
  3. Create app.config.ts based on main.ts
  4. npm run dev:ssr

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 7.1.5 (/usr/local/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/angular 7.5.4
@angular-devkit/build-angular : 17.0.0
@angular-devkit/schematics : 17.0.0
@angular/cli : 17.0.0
@ionic/angular-toolkit : 9.0.0

Capacitor:

Capacitor CLI : 5.5.1
@capacitor/android : not installed
@capacitor/core : 5.5.1
@capacitor/ios : not installed

Utility:

cordova-res : not installed globally
native-run (update available: 2.0.0) : 1.7.4

System:

NodeJS : v18.18.2 (/usr/local/Cellar/node@18/18.18.2/bin/node)
npm : 9.8.1
OS : macOS Unknown

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Nov 15, 2023
@liamdebeasi liamdebeasi added the ionitron: needs reproduction a code reproduction is needed from the issue author label Nov 15, 2023
Copy link

ionitron-bot bot commented Nov 15, 2023

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Ionic starter application and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@ionitron-bot ionitron-bot bot removed the triage label Nov 15, 2023
@liamdebeasi
Copy link
Contributor

Can you provide a reproduction we can run? SSR issues can be a bit tricky, so having a full sample will save us a lot of time.

@bashoogzaad
Copy link
Author

Hi @liamdebeasi , thanks for your quick response! I have created a repo here: https://github.com/bashoogzaad/ionic-ssr-test

@liamdebeasi
Copy link
Contributor

Does the issue persist if you use @ionic/angular-universal? https://ionic.io/blog/ssr-with-angular-universal-and-ionic

@bashoogzaad
Copy link
Author

I am not able to use that, that one is based on ngModules and I am using Angular Standalone (as the preferred solution nowadays). And Angular SSR should work out of the box in v17 (maybe this is related: https://forum.ionicframework.com/t/angular-universal-update/213522/2)

@liamdebeasi
Copy link
Contributor

You should be able to use NgModules in a standalone application still: https://angular.io/guide/standalone-components#using-existing-ngmodules-in-a-standalone-component. The module would be passed to the imports array in your app component.

Also, I'm not able to reproduce the issue in your sample repo:

Error: src/app/app.config.server.ts:3:27 - error TS2307: Cannot find module './app.config' or its corresponding type declarations.

3 import { appConfig } from './app.config';

Can you make sure there's an app.config file in the repo?

@bashoogzaad
Copy link
Author

Excuse me, I have added it now!

@averyjohnston averyjohnston added triage and removed ionitron: needs reproduction a code reproduction is needed from the issue author labels Nov 15, 2023
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Nov 16, 2023

Thanks! So the problem appears to be that we are moving children inside of ion-router-outlet. By default, the current view's component is mounted as a sibling of ion-router-outlet. However, the component needs to be a child of ion-router-outlet so things like page transitions and swipe to go back can work properly.

To work around this, we move the element inside of ion-router-outlet:

This causes a DOM tree mismatch during hydration because Angular was expecting the view's component to be a sibling of ion-router-outlet. According to https://angular.io/guide/hydration#third-party-libraries-with-dom-manipulation, you can avoid this by setting ngSkipHydration on the component that renders ion-router-outlet.

However, this would mean setting ngSkipHydration on the root element which defeats the purpose of having hydration in the first place. To fix this, we likely need to update our implementation to ensure that view components are mounted in the correct place the first time so we don't need to move anything around in the DOM.

@liamdebeasi liamdebeasi changed the title bug: using @angular/ssr (v17) in Ionic 7 with Angular Standalone not working bug: angular, moving elements inside of ion-router-outlet causes dom tree mismatch during hydration Nov 16, 2023
@liamdebeasi liamdebeasi added package: angular @ionic/angular package type: bug a confirmed bug report labels Nov 16, 2023
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Nov 16, 2023

Also wanted to set appropriate expectations: @angular/ssr does not support Web Components at the moment, so even if we fix this issue (which we should as there are positive performance implications for apps if we address this) there will still be other issues. For example, Stencil renders comments inside of scoped components for style encapsulation. These comments cause @angular/ssr to break. A good example of this is the ion-header component.

Unfortunately, this is not something we can address as the Angular team needs to add support for Web Components to the @angular/ssr package. If this is something import for your use case, I recommend providing feedback to the Angular team on https://github.com/angular/angular/issues.

@stevebrowndotco
Copy link

Relevant link: angular/angular#52275

@stevebrowndotco
Copy link

@liamdebeasi might be worth noting that the Lit framework team, which also uses web components, is claiming that they are working on SSR support now, according to here. https://lit.dev/docs/ssr/overview/ Maybe the Ionic Team could consider a similar approach?

@liamdebeasi
Copy link
Contributor

liamdebeasi commented Nov 29, 2023

We'd probably still advocate for getting Web Component support in @angular/ssr. Even if Stencil had a Stencil-specific SSR solution, the Angular team (and React and Vue teams) would likely need to add Stencil-specific integrations to make that SSR solution compatible with each JS Framework. It's certainly possible, but it would be a sizable effort to build and maintain all of these different integrations.

@eideard-hm
Copy link

Does this mean that server side rendering is not yet possible?

liamdebeasi added a commit that referenced this issue Jan 9, 2024
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.
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Jan 9, 2024

Thanks for the issue. This has been resolved via #28544, and a fix will be available in an upcoming release of Ionic Framework.

We chose to ship this in a major release of Ionic Framework to account for any unexpected behavior changes. There should be no behavior changes, but how views are mounted are important to the functionality of page transitions and swipe to go back, so we'd like a public testing period.


Does this mean that server side rendering is not yet possible?

You can use the @ionic/angular-server package with Angular Universal for some SSR support. However, Angular's @angular/ssr package does not support Web Components yet and cannot be used with Ionic.

Copy link

ionitron-bot bot commented Feb 8, 2024

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Feb 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: angular @ionic/angular package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

5 participants