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: Ionic / Angular Universal Prerender bugs on slides #21138

Closed
rgolea opened this issue Apr 27, 2020 · 27 comments · Fixed by #21350
Closed

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

rgolea opened this issue Apr 27, 2020 · 27 comments · Fixed by #21350
Assignees
Milestone

Comments

@rgolea
Copy link

rgolea commented Apr 27, 2020

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
Copy link

@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
Copy link

NikolaPeevski commented Apr 29, 2020

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
Copy link

NikolaPeevski commented Apr 29, 2020

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
Copy link
Author

rgolea commented Apr 30, 2020

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

@NikolaPeevski
Copy link

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

@rgolea
Copy link
Author

rgolea commented May 4, 2020

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

@NikolaPeevski
Copy link

@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
Copy link
Author

rgolea commented May 5, 2020

@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
Copy link
Author

rgolea commented May 5, 2020

@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
Copy link

Waiting for this merge as well

@rgolea
Copy link
Author

rgolea commented May 7, 2020

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
Copy link
Contributor

liamdebeasi commented May 7, 2020

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 (#21138 (comment)) 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
Copy link
Author

rgolea commented May 7, 2020

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
Copy link
Contributor

liamdebeasi commented May 7, 2020

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
Copy link
Author

rgolea commented May 8, 2020

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
Copy link
Author

rgolea commented May 12, 2020

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
Copy link
Contributor

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
Copy link
Author

rgolea commented May 14, 2020

@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
Copy link
Author

rgolea commented May 16, 2020

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

@liamdebeasi
Copy link
Contributor

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.

@liamdebeasi liamdebeasi self-assigned this May 18, 2020
@rgolea
Copy link
Author

rgolea commented May 18, 2020

@liamdebeasi I will check with him.

@liamdebeasi
Copy link
Contributor

Thanks for the issue. The Swiper dev released Swiper 5.4.1 with the dependency update, and I have updated Ionic with this via #21350. This fix will be available in an upcoming release of Ionic Framework.

@liamdebeasi
Copy link
Contributor

liamdebeasi commented May 20, 2020

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

@rgolea
Copy link
Author

rgolea commented May 20, 2020

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

@KevinBassaDevelopment
Copy link

KevinBassaDevelopment commented May 21, 2020

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
Copy link

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
Copy link

ionitron-bot bot commented Jun 28, 2020

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 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.