Support loading "mousetrap" in AOT mode when using SystemJS

[mlc-mlapis]

I'm submitting a ...

[x] Bug report

Current behavior

Actual loading of "mousetrap" is done using the following code because of the comment in the code: To prevent issues with angular-universal on server-side ...

if (typeof window !== 'undefined') { require('mousetrap'); }

which means that require('mousetrap') is called outside of the header of CommonJS module:

(function (global, factory) { ... } (this, (function (exports,core,common) { ... })));

and it leads to a run-time error require is not a function when SystemJS is used as a loader.

Expected behavior

Support loading of angular-modal-gallery.umd.js using SystemJS loader on a client side in a browser in AOT mode ... probably by using correct import and not a direct using of require('mousetrap'); in the code.

When the code if (typeof window !== 'undefined') { require('mousetrap'); } is eliminated (manual removing in the actual code in angular-modal-gallery.umd.js) all works fine in a client browser because loading is done by imports in the main app module:

import 'hammerjs';
import 'mousetrap';

What is the motivation / use case for changing the behavior?

Using Angular application in AOT mode when SystemJS loader is used.

- SystemJS: 0.20.19
- I'm using Server Side Rendering with angular-universal: NO
- I'm compiling with mode: PROD with AOT
- angular-modal-gallery: 5.7.1

[Ks89]

I cannot change it otherwise ssr won't work. Obviously I have to support all users but I particolar everyone that are using official tools like angular cli.

When Google will release a real support for ssr with window object on server side I'll really happy to remove it (it's a work in progress feature and I hope for a release with angular 7...i hope). Now I can't, because without this workaround no one could use ssr.

[mlc-mlapis]

@Ks89 ... hmm, that's sad. Isn't possible to use ModalGalleryModule.forRoot() to pass an option that would say it's running only in a browser? Like: ModalGalleryModule.forRoot({ssr: false})

So you would be able to skip the lines for SSR when not needed.

[Ks89]

I'll investigate. Please be patient because this week I'm really busy. I'll update you ASAP.

[Ks89]

I don't know how to implement this, because right now I must use require to support SSR with angular-cli and universal. If I add an else with import inside It isn't working, because I cannot use import conditionally (not supported by Angular itself).

So the only way is to use require.

Also, If I try to add another parameter to forRoot, this will be available inside the service in the constructor, but I need it before, outside the service class (at the beginning).

I don't think that it is feasible right now. We should wait for a real support by Angular.

When SSR will be supported in the right way, without workarounds, I'll update the library to remove the 'require'.

[Ks89]

PS: If you have other ideas, feel free to share their

[mlc-mlapis]

@Ks89 ... hm, dynamic import() expression has been working since TS 2.4 but only for --module esnext or --module commonjs and not for --module es2015 because that module format uses only static import declarations and they have to be only at the top-level of a file. So you are right that the only way is to use require.

What I don't understand is ...

but I need it before, outside the service class (at the beginning)

because you can call require('mousetrap'); in any place. Or I am missing something?

If I missed something then there is always a global possibility on window level because you are also using it as:

if (typeof window !== 'undefined') {
    require('mousetrap');
}

So we can create a global property ... for example window.AngularModalGallery = {ssr: false}; and you can check it and react as necessary.

[Ks89]

Mmm I'll try it

[Ks89]

because you can call require('mousetrap'); in any place. Or I am missing something?

You are right. I moved the require inside the constructor in this way:

// To prevent issues with angular-universal on server-side
    if (typeof window !== 'undefined') {
      require('mousetrap');
      this.mousetrap = new (<any>Mousetrap)();
    }

It seems to be ok.

Also, I added a new property into forRoot, however this is a huge workaround and I don't want to support this adding it to the documentation website. I decided to add it only to help you and It should be considered as a 'private' and 'undocumented' api that will be removed in upcoming versions (when Google will add a window implementation on server-side) without a real breaking change warning (so be careful and help me to test this in future releases).

ModalGalleryModule.forRoot({shortcuts: ['ctrl+s', 'meta+s'], disableSsrWorkaround: true})

You must specify both params.

What do you think? I'll release it this week as part of version 6.0.0-beta.2 under the new package name @ks89/angular-modal-gallery and with this migration tutorial https://ks89.github.io/angular-modal-gallery-2018-v6.github.io/migration

I cannot promise that it will be perfect, however we could try with this workaroud.

[mlc-mlapis]

@Ks89 ... super. Thanks a lot for your effort! I appreciate it. I'll help you with the testing in any future version.

[Ks89]

Beta2 released https://github.com/Ks89/angular-modal-gallery/releases/tag/v.6.0.0-beta.2

PS: please check the doc website https://ks89.github.io/angular-modal-gallery-2018-v6.github.io/migration site because SystemJS users must add the additional param to forRoot

If the website is still not updated change forRoot() with forRoot({ shortcuts: ['ctrl+s', 'meta+s'], disableSsrWorkaround: true })

[Ks89]

Hi @mlc-mlapis is it working now? If yes please close the issue, so I can release the final version this weekend.

thanks :)

[mlc-mlapis]

@Ks89 ... sorry for delay, I was out of my office ...but I'll report the result today. 🚗

[mlc-mlapis]

@Ks89 ... Hi, I finally did the tests with v.6.0.0-beta.2. All is working fine in JIT and AOT.

• I used Angular 6.0.4.
• Finally I had to move to Node 10.2.1 and use only NPM.
• YARN has some problems with running with Node 10.2.1.

[Ks89]

Ok perfect.

I'll do some tests and this weekend I'll release it as final version.

Thank u

Issue closed

[Ks89]

Hi @mlc-mlapis

Are you still using SystemJs? If yes, do you have a starter/template project to create an empty Angular9 project with Systemjs? Because I want to add it as official systemjs example to the next major release of the library. At the moment, I'm not able to use systemjs and Angular9 together.

thanks

[mlc-mlapis]

@Ks89 Hi, I don't have it. We are using CLI for new projects and old ones ... still not enough time to migrate or just try to switch to the latest one using SystemJS.

But with UMDs from Angular 9, it should probably be very close to the previous ones. The key is still ngc compiler. Of course, the tree-shaking is out of the game with SystemJS Builder.

What did you try, and what is the problem that is blocking you?

[Ks89]

Oooo damn! I forgot to import reflect-metadata. Fixed 👍

[Ks89]

I created a demo here https://github.com/Ks89/systemjs-angular-starter/tree/master Now I have to try to upgrade SystemJs to the latest version

[mlc-mlapis]

@Ks89 Not sure if it is a good idea to switch to the latest version of SystemJS. They totally re-structuralized the code and changed many things -> a lot of breaking changes. For what you need, it's better to keep the previous one, I think.

[Ks89]

Probably you are right. I didn't find a good tutorial and apis are totally different.

[mlc-mlapis]

@Ks89 Fine. Btw, ... are you OK? I mean the actual situation in Italy and how it affects you and your family.

[Ks89]

Yes everything is OK :) Thank u.