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: ion-content fullscreen is not compatible with pre-rendering #27411

Closed
3 tasks done
WDrzewiecki opened this issue May 6, 2023 · 10 comments · Fixed by #27440
Closed
3 tasks done

bug: ion-content fullscreen is not compatible with pre-rendering #27411

WDrzewiecki opened this issue May 6, 2023 · 10 comments · Fixed by #27440
Assignees
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@WDrzewiecki
Copy link

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

SSR not working after upgrading to angular 16, even in ionic starter app. After starting dev server and opening the app, the page loads until the server throws out of memory.
With a production build server throws "NotYetImplemented" error and returns only original index.html file without rendered page.

Expected Behavior

SSR should work with angular 16.

Steps to Reproduce

  1. Start SSR dev server (npm run dev:ssr)
  2. Visit app located on localhost:4200
  3. Loading continues until the server throws out of memory
    OR
  4. Build app
  5. Run npm run serve:ssr
  6. Visit app with disabled javascript

Code Reproduction URL

https://github.com/WDrzewiecki/ionic-ssr

Ionic Info

Ionic:

Ionic CLI : 7.1.1 (/usr/local/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/angular 7.0.5
@angular-devkit/build-angular : 16.0.0
@angular-devkit/schematics : 16.0.0
@angular/cli : 16.0.0
@ionic/angular-toolkit : 9.0.0

Capacitor:

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

Utility:

cordova-res : 0.15.4
native-run : 1.7.2

System:

NodeJS : v18.16.0 (/usr/local/bin/node)
npm : 9.5.1
OS : macOS Unknown

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label May 6, 2023
@liamdebeasi liamdebeasi self-assigned this May 8, 2023
@liamdebeasi
Copy link
Contributor

Thanks for the report. I can reproduce the reported out of memory error, but I don't see anything that would indicate this is an Ionic bug. Have you tried filing an issue with the Angular team?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label May 8, 2023
@liamdebeasi liamdebeasi removed their assignment May 8, 2023
@ionitron-bot ionitron-bot bot removed the triage label May 8, 2023
@WDrzewiecki
Copy link
Author

Thanks for your reply. I haven't reported the issue to the Angular team yet. But after removing the IonicServerModule from the imports in app.server.module the page starts to render (obviously the ionic components have some visual issues, but all other added components render fine). Also Angular starter app works well with ssr.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels May 8, 2023
@liamdebeasi
Copy link
Contributor

liamdebeasi commented May 8, 2023

Are you able to identify which part of IonicServerModule is causing the issue? It's possible that the module is triggering a bug in Angular. It's also possible that there is an incompatibility with Angular 16, but there's not enough information here to determine that.

If you have a stack trace with the source coming from Ionic-related code, that would be helpful.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label May 8, 2023
@ionitron-bot ionitron-bot bot removed the triage label May 8, 2023
@WDrzewiecki
Copy link
Author

Unfortunately, the only logs I have currently come from the prod build, so they are not very readable, but I also attach the main.ts file related to reported logs (https://github.com/WDrzewiecki/ionic-ssr/blob/main/main.js)

And these are my log:

Error: NotYetImplemented
    at exports2.nyi (/Users/user/myApp/dist/app/server/main.js:133678:25)
    at hydrateApp (/Users/user/myApp/dist/app/server/main.js:19461:25)
    at hydrateAppClosure (/Users/user/myApp/dist/app/server/main.js:49424:15)
    at hydrateFactory (/Users/user/myApp/dist/app/server/main.js:49473:13)
    at render (/Users/user/myApp/dist/app/server/main.js:49869:16)
    at /Users/user/myApp/dist/app/server/main.js:49753:21
    at new Promise (<anonymous>)
    at hydrateDocument (/Users/user/myApp/dist/app/server/main.js:49733:13)
    at /Users/user/myApp/dist/app/server/main.js:161244:39
    at /Users/user/myApp/dist/app/server/main.js:150845:42

As you can see line 161244 is part of hydrateIonicComponents function so maybe this can help somehow.
I can try to get more logs soon if needed.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels May 8, 2023
@liamdebeasi
Copy link
Contributor

Is there a way I can reproduce this in a development build?

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label May 8, 2023
@ionitron-bot ionitron-bot bot removed the triage label May 8, 2023
@WDrzewiecki
Copy link
Author

Just change in angular.json file in serve-ssr from "browserTarget": "app:build:development", to "browserTarget": "app:build", and from "serverTarget": "app:server:development" to "serverTarget": "app:server" and after running npm run dev:ssr you should have this logs. It may not be the most elegant way, but it should work temporarily

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels May 8, 2023
@WDrzewiecki
Copy link
Author

And some more logs from debugger:
Zrzut ekranu 2023-05-8 o 16 50 12

@sean-perkins sean-perkins self-assigned this May 9, 2023
@sean-perkins
Copy link
Contributor

sean-perkins commented May 9, 2023

Hello @WDrzewiecki can you try with this dev-build and let me know if you experience any issues?

npm install @ionic/angular@7.0.6-dev.11683653232.1ddc5840 @ionic/angular-server@7.0.6-dev.11683653232.1ddc5840

The problematic UI was <ion-content [fullscreen]="true">. Our fullscreen implementation reads the element dimensions and force updates the element. This causes an issue in a SSR environment. The dev-build includes a change that only does this behavior in a browser environment.

Related Stencil bugs that should be closed with this:

@sean-perkins sean-perkins added the needs: reply the issue needs a response from the user label May 9, 2023
@ionitron-bot ionitron-bot bot removed the triage label May 9, 2023
@WDrzewiecki
Copy link
Author

Everything seems to be working fine now, thank you so much for your assistance

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels May 9, 2023
@sean-perkins sean-perkins changed the title bug: SSR not woking with angular 16 bug: ion-content fullscreen is not compatible with pre-rendering May 9, 2023
@sean-perkins sean-perkins added package: core @ionic/core package type: bug a confirmed bug report labels May 9, 2023
@ionitron-bot ionitron-bot bot removed the triage label May 9, 2023
sean-perkins added a commit that referenced this issue May 9, 2023
Issue number: Resolves #27411,
ionic-team/stencil#2429,
ionic-team/stencil#4076

---------

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

Rendering `<ion-content fullscreen="true">` in an Angular Universal
project will result in a javascript heap exception and the browser tab
timing out.

`forceUpdate` is not a compatible API with pre-rendering and results in
calling itself indefinitely.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- Updates the fullscreen implementation of `ion-content` to only call
`forceUpdate` and related functionality when running in a browser
environment.
- `<ion-content fullscreen="true">` is compatible with Angular Universal

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- 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.0.6-dev.11683653232.1ddc5840` ✅
@ionitron-bot
Copy link

ionitron-bot bot commented Jun 8, 2023

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 Jun 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants