ionic-team / ionic-framework

A powerful cross-platform UI toolkit for building native-quality iOS, Android, and Progressive Web Apps with HTML, CSS, and JavaScript.
https://ionicframework.com
MIT License
50.93k stars 13.52k forks source link

bug: Ionic / Angular Universal Prerender bugs on slides #21138

Closed rgolea closed 4 years ago

rgolea commented 4 years ago

Bug Report

Ionic version:

[x] 5.x

Current behavior:

The SSR prerender does not render properly. There is a problem when importing slider.js. It seems that the screen object is not defined. Expected behavior:

It should allow to prerender the html files for the templates that contain ion-select to be prerendered. Steps to reproduce:

It happens when running npm run prerender directly from the @nguniversal/express-engine schematics and the template has an ion-slides.

Related code:

A sample application via GitHub https://github.com/rgolea/ionic-ssr-errors

Other information:

This is a continuation of the #21063 since the original issue was fixed.

Ionic info:

Ionic:

   Ionic CLI : 6.6.0

Utility:

   cordova-res                          : not installed
   native-run (update available: 1.0.0) : 0.3.0

System:

   NodeJS : v13.12.0
   npm    : 6.14.4
   OS     : macOS Catalina

Errors:

Prerendering 1 route(s) to <path-to-project>/dist/ionic-ssr-errors/browser
TypeError: Cannot read property 'width' of undefined
    at Device (<path-to-project>/dist/ionic-ssr-errors/server/main.js:1:3026748)
    at hydrateAppClosure (<path-to-project>/dist/ionic-ssr-errors/server/main.js:1:3028625)
    at hydrateFactory (<path-to-project>/dist/ionic-ssr-errors/server/main.js:1:3107878)
    at render (<path-to-project>/dist/ionic-ssr-errors/server/main.js:1:3128770)
    at <path-to-project>/dist/ionic-ssr-errors/server/main.js:1:3133854
    at new ZoneAwarePromise (<path-to-project>/dist/ionic-ssr-errors/server/main.js:1:3206236)
    at hydrateDocument (<path-to-project>/dist/ionic-ssr-errors/server/main.js:1:3133500)
    at <path-to-project>/dist/ionic-ssr-errors/server/main.js:1:1287139
    at <path-to-project>/dist/ionic-ssr-errors/server/main.js:1:1212014
    at ZoneDelegate.invoke (<path-to-project>/dist/ionic-ssr-errors/server/main.js:1:3194265)
CREATE <path-to-project>/dist/ionic-ssr-errors/browser/index.html (3471 bytes)

The error seems to come from here: https://github.com/ionic-team/ionic/blob/6a167172ffa4802ff68b5ca0690586438f5ed744/core/src/components/slides/swiper/swiper.bundle.js#L2586 The thing is that it seems to be defined: ScreenShot

NikolaPeevski commented 4 years ago

@liamdebeasi Do you guys have a customised express engine implementation or is the ionic ssr using the universal express engine purely without any changes ? I can't seem to find an example.

NikolaPeevski commented 4 years ago

This is an old project of mine using a few modifications to mock instances such as win and document, which fixes said issues, it's on top of the regular ngexpress. https://gist.github.com/NikolaPeevski/5a016d9edb46ec92ad99803fcf9f4558

NikolaPeevski commented 4 years ago

Additionally there are some issues with inline css bootstrapping if one decides to use a crawler to generate facebook sharable thumbnails for example. I can provide an example that fixes that as well, it's a bit more clusterfunky tho.

rgolea commented 4 years ago

@NikolaPeevski This is not serving it and applying domino. This is prerender the app completely. See the github repo I added.

NikolaPeevski commented 4 years ago

@rgolea You can still mock the browser when pre-rendering, you need a prerender script similar to server.ts

rgolea commented 4 years ago

@NikolaPeevski would you be able to clone the provided git repo and show me a way to do that?

NikolaPeevski commented 4 years ago

@rgolea Check this out, https://github.com/Angular-RU/angular-universal-starter/blob/master/prerender.ts, I can also clone it and show you an example later. When you generate pages if I remember correctly it's the same principle as server side rendering them, as in you still have some javascript chunks that will be rendered in the browser, you can isolate them with the platform service. But in your case mocking it with a custom prerenderer is enough since you only care about the window.

rgolea commented 4 years ago

@NikolaPeevski So, the problem is not how to implement SSR. The problem is Ionic's incompatibility with SSR. Say for example I implement inside main.server.ts the domino library. I get to read the template, and create the window object and assign it to the global object. I'm still way too far from bug free issues:

const template = readFileSync(
  join(process.cwd(), 'dist', 'ionic-ssr-errors', 'browser', 'index.html')
).toString();
const win = domino.createWindow(template);
// tslint:disable no-string-literal
global['window'] = win;
// tslint:enable no-string-literal

After this I get the document is undefined, self is undefined, navigator is undefined, DocumentFragment is undefined to which my code extends to be the following:

const template = readFileSync(
  join(process.cwd(), 'dist', 'ionic-ssr-errors', 'browser', 'index.html')
).toString();
const win = domino.createWindow(template);
// tslint:disable no-string-literal
global['window'] = win;
global['document'] = win.document;
global['self'] = win;
global['navigator'] = win.navigator;
Object.assign(global, domino['impl']);
// tslint:enable no-string-literal

Afterwards, I still have issues with customElements which has me importing the webcomponents polyfill:

import '@webcomponents/webcomponentsjs';

After a couple of hours of hitting every dead end possible, I still get:

UnhandledPromiseRejectionWarning: ReferenceError: DOMParser is not defined

If you have any new ideas on how to get it to work when I run npm run prerender I am all ears.

rgolea commented 4 years ago

@liamdebeasi @NikolaPeevski Okay, so... I found the issue. The swiper.js library uses ssr-window as a dependency. This library uses the window object or the one simulated by them. Somewhere along the angular universal module, or maybe ionic server, or somewhere along the builds, before getting to the swiper bundle, there is a window object that gets injected but does not have window.screen. Since the library only checks that the objects exists, it doesn't add properties that need to exist in order to make swiper.js work. That is why I'm adding to the global window all parts that do not exist.

Please check out this PR#4 and, if you can, please encourage the team to merge it so this issue can be solved.

agustinhaller commented 4 years ago

Waiting for this merge as well

rgolea commented 4 years ago

In the meanwhile, anyone who's interested, can just do the following:

//main.server.ts
import 'ssr-window';
// If the above line doesn't work you can do
import { window as win, document as doc } from 'ssr-window';
global['window'] = window;
global['document'] = document;

This is theoretical, I didn't test it. If someone does, please leave a reply.

liamdebeasi commented 4 years ago

Hmm so it sounds like this is not a bug in Ionic but rather a bug in that ssr-window dependency. Am I understanding your comment (https://github.com/ionic-team/ionic/issues/21138#issuecomment-623864247) correctly, @rgolea?

Looks like the developer is working with you on getting the fix merged in. When that fix has been merged and released I will update Ionic to pull in the latest version of swiper/ssr-window.

rgolea commented 4 years ago

Yes! Thank you @liamdebeasi. Does Ionic use ssr-window directly? I believe it's a dependency of swiper, and swiper has to get the latest version as well.

liamdebeasi commented 4 years ago

Yeah, I think you are correct -- ssr-window is a dependency of swiper: https://github.com/nolimits4web/swiper/blob/master/package.json#L77. Not sure what the dev's release process is, but we'll get the fix into Ionic one way or another.

rgolea commented 4 years ago

Hey guys! The PR has merged with some extra features. If you want to, please try over the weekend the following: https://www.npmjs.com/package/ssr-window/v/2.0.0-beta.6 I will try it myself too.

rgolea commented 4 years ago

Quick update! v.2.0.0-beta.8 works. Confirmed! Check the changes here I hope the author merges it soon to 2.0.0 and releases it to swiper but it needs testing. Help would be appreciated. Thanks!

liamdebeasi commented 4 years ago

The author released 2.0.0 stable 2 days ago. Just need to wait for Swiper to get updated with the ssr-window dependency, and I will update Ionic.

In the meantime, I created a dev build of Ionic with a custom build of Swiper. This build has Ionic Framework v5.1.1 and Swiper 5.3.8 + ssr-window 2.0.0.

Here is a dev build if anyone would like to test: npm i @ionic/angular@5.2.0-dev.202005141344.39bfaea

I tested this using the repo in the original post, and it seems like this fixes the issue.

rgolea commented 4 years ago

@liamdebeasi you're the best! Thank you! I will keep you posted and tell you when swiper gets the latest update. It was a hard bug to catch since it was an incompatibility with domino and window, but that raises the question: does @ionic/angular-server has tests regarding implementing domino? That should have failed in the tests. 😳

rgolea commented 4 years ago

@liamdebeasi swiper has updated to the latest version. I will test tomorrow and confirm it works.

liamdebeasi commented 4 years ago

Looks like the dependency in the package directory still points to ssr-window@1.0.1: https://github.com/nolimits4web/swiper/blob/master/package/package.json#L54. The main package.json points to 2.0.0. This seems to cause the old version of ssr-window to still get pulled in when I bundle it with Ionic, so I'll need to wait for the developer to update that dependency as well.

rgolea commented 4 years ago

@liamdebeasi I will check with him.

liamdebeasi commented 4 years ago

Thanks for the issue. The Swiper dev released Swiper 5.4.1 with the dependency update, and I have updated Ionic with this via https://github.com/ionic-team/ionic/pull/21350. This fix will be available in an upcoming release of Ionic Framework.

liamdebeasi commented 4 years ago

Also a big "thank you" to @rgolea for helping with testing and getting this issue fixed! 🎉

rgolea commented 4 years ago

Thank you @liamdebeasi. Any time! Hope it helps others as well!

KevinBassaDevelopment commented 4 years ago

Also thank you 👍 ! Now I will finally be able to upgrade to 5.1.x and make use of the new keyboard functionality. (Or 5.2 it seems)

kotuadam commented 4 years ago

The author released 2.0.0 stable 2 days ago. Just need to wait for Swiper to get updated with the ssr-window dependency, and I will update Ionic.

In the meantime, I created a dev build of Ionic with a custom build of Swiper. This build has Ionic Framework v5.1.1 and Swiper 5.3.8 + ssr-window 2.0.0.

Here is a dev build if anyone would like to test: npm i @ionic/angular@5.2.0-dev.202005141344.39bfaea

I tested this using the repo in the original post, and it seems like this fixes the issue.

I confirm that this fixes the issue. In addition, ion-menu problem on angular universal is also fixed.. Now waiting this version to be released... Thanks.

ionitron-bot[bot] commented 4 years ago

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.