angular-fullpage icon indicating copy to clipboard operation
angular-fullpage copied to clipboard

SSR not working properly

Open davidstellini opened this issue 6 years ago • 16 comments

The library is trying to import fullpage.js/dist/fullpage.extensions.min', no matter if it's running on platform browser or server. The fullpage extensions file in turn tries to do funny things with window, which breaks SSR rendering. I can open a PR, I think it should be a quick fix.

davidstellini avatar Jan 10 '19 15:01 davidstellini

I don't have much experience with angular-universal. Shouldn't it be fixed by using Server-side DOM such as domino in your server.js?

If you can open a PR, that would be great!

vh13294 avatar Jan 10 '19 22:01 vh13294

The fullpage extensions file in turn tries to do funny things with window, which breaks SSR rendering.

fullpage.js relies on the window object for many things, as you can see on the sourcecode, but if you think you can find a solution for this issue please feel free to open a PR.

alvarotrigo avatar Jan 11 '19 00:01 alvarotrigo

Sure. I don't know if you'll like it but the solution I found to work is not to import fullpage.js in the first place if it's running under server side code. I see there's already isPlatformBrowser checks to make sure it's not initialized if the platform is serverside, but it seems fullpage.js is using window as soon as it's required, not initialized, so the checks aren't really helping. I got it to work - I'll open a PR soon.

davidstellini avatar Jan 11 '19 07:01 davidstellini

https://github.com/alvarotrigo/angular-fullpage/pull/25

davidstellini avatar Jan 11 '19 07:01 davidstellini

I don't have much experience with angular-universal. Shouldn't it be fixed by using Server-side DOM such as domino in your server.js?

If you can open a PR, that would be great!

For some reason it still seems to fail even when domino is being used. I can maybe replicate it on a fork of this repo, which is also using domino in the server.ts: https://github.com/Angular-RU/angular-universal-starter/blob/master/server.ts I'm not sure why, I'm not very experienced with angular universal either, but adding in domino was one of the first things I tried.

davidstellini avatar Jan 11 '19 08:01 davidstellini

global['window'] = win;
Object.defineProperty(win.document.body.style, 'transform', {
  value: () => {
    return {
      enumerable: true,
      configurable: true,
    };
  },
});
global['document'] = win.document;
global['CSS'] = null;
// global['XMLHttpRequest'] = require('xmlhttprequest').XMLHttpRequest;
global['Prism'] = null;
global['DOMTokenList'] = win.DOMTokenList;
global['Node'] = win.Node;
global['NodeList'] = win.NodeList;
global['Text'] = win.Text;
global['HTMLElement'] = win.HTMLElement;
global['navigator'] = win.navigator;

these are the list of variables that you have to declare! Try this first to see, if it work.

vh13294 avatar Jan 11 '19 09:01 vh13294

Hi. First of all thank you for your reply and your time - and apologise for the long time taken to reply. I tried the above code on the server side - unfortunately I couldn't get it to work with the above code. I get the same error as before.

Also, defining the window object makes many libraries think that they are working in the browser if they use these checks: typeof window !== 'undefined' This can make other libraries behave erratically. I have managed to avoid it so far, and ideally, would really like to keep it this way.

I have pointed the project using @angular/fullpage to the commit for the PR I had opened: https://github.com/alvarotrigo/angular-fullpage/pull/25/commits/5c6c4b09897f44896dcc72ba2a1fe4d5cd4c7bf0 And it has been working like a charm since January. Do you have any other workaround in mind or would you be able to add the check to this library to give it SSR support?

Again, thank you so much for your time!

davidstellini avatar May 10 '19 09:05 davidstellini

@vh13294 any thoughts on that PR?

alvarotrigo avatar May 10 '19 10:05 alvarotrigo

Thanks for your quick reply - look forward to resolving this issue

davidstellini avatar May 14 '19 07:05 davidstellini

@alvarotrigo it's not the most elegant solution. But it's a good support for SSR.

vh13294 avatar May 14 '19 08:05 vh13294

@vh13294 should we merge it ?

@davidstellini I can not see your PR in this repository. Can you please create it here so we can merge it? Otherwise we will just make the changes by ourselves.

alvarotrigo avatar May 14 '19 09:05 alvarotrigo

If you can, as I'm abroad without access to my laptop for a few days. Otherwise I'll open another PR in a few days.

On Tue, May 14, 2019 at 11:54 AM Álvaro [email protected] wrote:

@vh13294 https://github.com/vh13294 should we merge it ?

@davidstellini https://github.com/davidstellini I can not see your PR in this repository https://github.com/alvarotrigo/angular-fullpage/pulls. Can you please create it here so we can merge it? Otherwise we will just make the changes by ourselves.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/alvarotrigo/angular-fullpage/issues/24?email_source=notifications&email_token=ABA7YQ3CFNPSUDAL6AAVLGTPVKD53A5CNFSM4GPG6DJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVK6ZQI#issuecomment-492170433, or mute the thread https://github.com/notifications/unsubscribe-auth/ABA7YQ7BZMMRY4P4C67RGR3PVKD53ANCNFSM4GPG6DJA .

davidstellini avatar May 14 '19 15:05 davidstellini

@alvarotrigo I will try to find a proper solution.

@davidstellini

Is it possible to check for platform-server or browser at your end? Then do this

<ng-container *ngIf="isBrowser">
   fullpage.js
</ng-container>
In ts,

isBrowser;
constructor(@Inject(PLATFORM_ID) private platformId) { 
   this.isBrowser = isPlatformBrowser(platformId);
}

...

if (this.isBrowser) { 
  // put your code which is access window variable 
} 

Ref: https://github.com/angular/universal/issues/830#issuecomment-338359090

vh13294 avatar May 15 '19 03:05 vh13294

Hi @vh13294 - I have these checks all over the app;

if (this.isBrowser) { 
  // put your code which is access window variable 
} 

The issue is that:

  1. As soon as if the AngularFullpageModule is imported in a module;
  2. This line is executed straight away: import fullpage from 'fullpage.js/dist/fullpage.extensions.min';
  3. Then almost immediately, the fullpage extensions tries to access the window variable. So the this.isBrowser check should either be in the place that is accessing the window variable (fullpage extensions, which seems to not be possible) - or we can stop the chain that causes the issue somewhere before.

Anyone who uses your library with SSR, will have their app break as soon as they import your module from the server side.

I see three options:

  1. Doing it the JS way - Detect if you are running in a node environment inside fullpage extensions using: if (typeof window !== 'undefined), and then not loading the module if it's not there, logging something like "Sorry, fullpageJS can only work on a client side JS environment". This is super common, but I'm not sure what the implications are: https://github.com/search?q=if+typeof+window+undefined&type=Code
  2. Doing it the angular way - Detect if you are running serverside inside angular-fullpage, and not importing the module, using the check you gave in the comment above;
  3. Delegating this responsibility to who uses your library - which I have tried to do, and have not managed so far. Basically, if you use SSR you can have a server.module.ts which includes modules that are only imported server side, and the same for the client side - so you can prevent this module from being imported. This is tricky because you then don't know if the directive exists during runtime, and might have to load a different parent component for client and server side - and it's super complicated. I have tried this for a few hours, and so far have been unsuccessful. I will provide you with further details about this soon.

davidstellini avatar May 15 '19 08:05 davidstellini

@alvarotrigo This is how react-fullpage solves the SSR issue,

https://github.com/alvarotrigo/react-fullpage/blob/5a11fb5211e5706da81f2874516f023dc6e5aa88/components/index.js#L17

It's similar, but more refined. I think it's okay to merge.

vh13294 avatar May 20 '19 00:05 vh13294

@vh13294 thanks for the info! Could you please confirm the commit I just made should solve this issue? https://github.com/alvarotrigo/angular-fullpage/commit/edc8953b8d8b96dc7f806f03658fe9cd60e192f4

I'll then create a new version, merge it and publish it to npm.

alvarotrigo avatar May 20 '19 15:05 alvarotrigo

the latest version not have import fullpage.js/dist/fullpage.extensions.min.js on directive the import its already on import angular.json

and also we already have check for ssr or no on directive

ngAfterViewInit() {
  if (isPlatformBrowser(this.platformId)) {
    this.initFullpage();
  }
  if (isPlatformServer(this.platformId)) {
    // server side code
  }
}

alidihaw avatar Dec 16 '23 02:12 alidihaw

@alidihaw, thanks for clarifying it! 👍

alvarotrigo avatar Dec 16 '23 16:12 alvarotrigo