Angular: Use ngtools/webpack instead of at-loader #7231

wmarques · 2018-03-05T13:42:56Z

It's better to lazy load modules to reduce the main bundle size and avoid giving the user data he will probably never need.

In order to make the lazy loading work with webpack we have two choices:

Use the angular-router-loader for the dev config and @ngtools/webpack for prod (with AOT)
Use @ngtools/webpack everywhere

For consistency and simplification I chose to use @ngtools/webpack everywhere except for the test configuration, I couldn't make it work... But I saw that we were using a fork of the "official" istanbul-instrumenter-loaderso I put in our dependencies the official loader and not the fork which doesn't seems maintained anymore.

We can discuss this of course, I didn't see much performance issues using @ngtools/webpack in development mode.

Please make sure the below checklist is followed for Pull Requests.
Travis tests are green
Tests are added where necessary
Documentation is added/updated where necessary
Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

wmarques · 2018-03-05T14:29:27Z

If you have any hints on the SyntaxError: Unexpected token importI'm having with protractor... It doesn't seems related to the PR

EDIT: fixed, had to create a tsconfig especially for protractor 😢

sendilkumarn · 2018-03-05T17:43:55Z

generators/client/templates/angular/webpack/webpack.common.js.ejs

@@ -40,6 +41,10 @@ module.exports = (options) => ({
    },
    module: {
        rules: [
+            {
+                test: /(?:\.ngfactory\.js|\.ngstyle\.js|\.ts)$/,
+                use: [ '@ngtools/webpack' ]


brave move 😉 😜

Haha yes ! Please try if you see any performance issues I'll rollback of course 😄

Yeah sure 👍
Did you find any good performance change?
This also means that we will have to wait for angular-cli release cycle. v6 - ivy promises a lot let us see

I'll do some tests to see the build time but it seemed pretty much the same...

what matters is the time to compile when in hot reload mode for any changes to a file

sendilkumarn

🎉 awesome so we do have lazy loading in V5

🎉 🎆

sendilkumarn · 2018-03-05T17:44:19Z

generators/client/templates/angular/webpack/webpack.common.js.ejs

@@ -64,6 +69,11 @@ module.exports = (options) => ({
        ]
    },
    plugins: [
+        new AngularCompilerPlugin({
+            mainPath: utils.root('src/main/webapp/app/app.main.ts'),


<%= MAIN_SRC_DIR %>

sendilkumarn · 2018-03-05T17:45:55Z

generators/client/templates/angular/webpack/webpack.test.js.ejs

@@ -61,7 +61,7 @@ module.exports = (WATCH) => ({
                test: /src[/|\\]main[/|\\]webapp[/|\\].+\.ts$/,
                enforce: 'post',
                exclude: /node_modules/,
-                loader: 'sourcemap-istanbul-instrumenter-loader?force-sourcemap=true'
+                loader: 'istanbul-instrumenter-loader?force-sourcemap=true&esModules=true'


can you change this to use syntax ?

sendilkumarn · 2018-03-05T17:47:04Z

generators/client/templates/angular/.angular-cli.json.ejs

-    "lint": [{
-            "project": "../../../tsconfig.json"
-        },
+    "lint": [
        {


can you try to lint this like the deleted line above ?

sendilkumarn · 2018-03-05T17:48:19Z

generators/client/templates/angular/package.json.ejs

@@ -78,6 +78,7 @@
    "html-loader": "0.5.0",
    "html-webpack-plugin": "3.0.0",
    "husky": "0.14.3",
+    "istanbul-instrumenter-loader": "3.0.0",


3.0.0 doesn't support webpack 4 right ?

I didn't have issue about this...

sendilkumarn · 2018-03-05T17:50:23Z

generators/client/templates/angular/webpack/webpack.dev.js.ejs

-        {
-            test: /\.ts$/,
-            loaders: [
-                'angular2-template-loader',


is this dependency also removed from package.json ?

Nope because we need it in the webpack.test :(

sendilkumarn · 2018-03-05T17:51:03Z

test/utils/expected-files.js

@@ -363,7 +363,6 @@ const expectedFiles = {
        `${CLIENT_TEST_SRC_DIR}spec/helpers/mock-state-storage.service.ts`,
        `${CLIENT_TEST_SRC_DIR}spec/helpers/spyobject.ts`,
        `${CLIENT_TEST_SRC_DIR}spec/test.module.ts`,
-        'tsconfig-aot.json',
        'tsconfig.json',


tsconfig.e2e ?

wmarques · 2018-03-05T18:14:23Z

Seems that I have an issue with the interceptors who are not shared in the lazy loaded module. I think the cause is maybe the ng-jhipster library who exports the HttpClientModule, but this module should be imported only once in the app

sendilkumarn · 2018-03-05T18:33:53Z

@wmarques WDYT about this?

Split this PR into 2, one to migrate to ngtools-webpack other one for lazy loading

The first one will fix the yarn start issue.

wmarques · 2018-03-05T18:36:43Z

@sendilkumarn Yep LGTM :) I'm also investigating the HttpClient issues. So should we close this PRand i'll do two others ?
But in the lazy loading PR, should I use the ng-tools/webpack ? Or i keep the awesome-typescript-loader ?

wmarques · 2018-03-05T22:47:04Z

@sendilkumarn this PR will be about the ngtools usage, I'll create a new one for the lazy loading if that's ok for you

sendilkumarn · 2018-03-05T22:56:20Z

@wmarques yeah sure no issues at all. Just split it. I think @ngtools won't break now. So it can be merged and lazy loading we can check and merge

sendilkumarn · 2018-03-05T22:57:24Z

pinging @deepu105 at-loader is not working here due to some issues. It is better time to switch to @ngtools WDYT ? I remember last time, you don't want to move to @ngtools

sendilkumarn · 2018-03-06T07:57:48Z

@wmarques did you refactor the PR ?

deepu105 · 2018-03-06T08:22:10Z

@sendilkumarn @wmarques last time I didn't want to switch as it was slow in development, please test with some 10-15 entities. Each code change was taking so long to compile while running yarn start as everything was being recompiled instead of incremental changes. IF that issue no longer exists and the difference in performance while using Yarn start is very minimal then I'm ok to switch

Could you guys post some stats pre and post this change when using Yarn start

wmarques · 2018-03-06T09:04:50Z

Yep I'll try with a big project. Do you have a repo where I can test this kind of config ?
I don't have time tonight but tomorrow I'll check this and refactor the PR to remove the lazy loading part. Thanks

deepu105 · 2018-03-06T09:23:11Z

Just use one of our travis builds, it has lot of entities. Use the build script under the travis folder and generate a sample app and copy the config from that Thanks & Regards, Deepu

…

On Tue, Mar 6, 2018 at 10:04 AM, William Marques ***@***.***> wrote: Yep I'll try with a big project. Do you have a repo where I can test this kind of config ? I don't have time tonight but tomorrow I'll check this and refactor the PR to remove the lazy loading part. Thanks — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7231 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDlF1rkBO4-pEp1QStWG_A0r_SklNalks5tblE6gaJpZM4ScThu> .

wmarques · 2018-03-07T23:20:09Z

I refactored the PR it should now only contains the ngtools part ! I'm testing it with a big application to check performances.

EDIT: so I tried with the sample app in travis folder, and the dev workflow is very slow with @ngtools, as @deepu105 said it seems to recompile everything and each refresh takes too much time (ie: 17010ms chunk assets processing), maybe in v6 it will be faster...

sendilkumarn · 2018-03-09T06:56:07Z

can we try with ts-loader if we can ? It was also migrated to v4.

pascalgrimaud · 2018-03-09T07:16:32Z

Just finished to align the registry with JHipster v5: jhipster/jhipster-registry#251

@wmarques @sendilkumarn : do you think we should do the same for the registry too ?

deepu105 · 2018-03-09T07:28:05Z

Pascal, its not very important for Registry at the moment.We can do it once we figure out all the details later Thanks & Regards, Deepu

…

On Fri, Mar 9, 2018 at 8:16 AM, Pascal Grimaud ***@***.***> wrote: Just finished to align the registry with JHipster v5: jhipster/jhipster-registry#251 <jhipster/jhipster-registry#251> @wmarques <https://github.com/wmarques> @sendilkumarn <https://github.com/sendilkumarn> : do you think we should do the same for the registry too ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7231 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABDlFwyHvs8iF9SLZjC7yRliB9tsyPyDks5tcixXgaJpZM4ScThu> .

wmarques · 2018-03-10T15:08:53Z

@sendilkumarn You mean using ts-loader for the dev mode ? Or for dev and prod ?

deepu105 · 2018-03-10T16:24:01Z

@wmarques @sendilkumarn seems like migrating to webpack 4 didn't give us any performance improvements and our tests seems to be slower now, so I'm worried about this so can we first do a benchmark?

deepu105 · 2018-03-11T18:20:14Z

@wmarques also take a look at s-panferov/awesome-typescript-loader#497

deepu105 · 2018-03-11T18:27:52Z

Btw I can really see webpack performance improvement in React :)
Build time down to 15 secs with ts-loader, gonna try to make it even faster

deepu105 · 2018-03-11T19:04:08Z

So with ts-loader, cache-loader, thread-loader and fork-ts-checker-webpack-plugin I got build times of 12 secs for React with few entities. The yarn start compilation is very fast as well, ~10s fisrt run and ~3s for a change, also everything including yarn start works so we can try the same for Angular as well and compare to ngtools for performance.

deepu105 · 2018-03-12T21:07:23Z

@wmarques do you still want to do this? if so please benchmark this against the current setup with ts-loader first plz

William Marques added 4 commits March 5, 2018 15:29

Lazy load admin, use ngtools always

eaeaec6

fix test loader

d23e7fe

Enable sourcemap in tsconfig

7f7b34f

Fix e2e tests

827c653

wmarques force-pushed the lazy-load-admin branch from 1324873 to 827c653 Compare March 5, 2018 14:58

William Marques added 3 commits March 5, 2018 16:23

remove trailing slash

2cab3f2

copy tsconfig e2e file

cc720a3

add admin url prefix in protractor test

46e24cd

sendilkumarn reviewed Mar 5, 2018

View reviewed changes

sendilkumarn requested changes Mar 5, 2018

View reviewed changes

wmarques mentioned this pull request Mar 5, 2018

Remove HttpClientModule to avoid multiple instances jhipster/ng-jhipster#63

Merged

William Marques added 4 commits March 5, 2018 19:46

use main_src_dir

239f37f

fix app.main path

a90e7d7

'use' Syntax for istanbul loader

b6862eb

import HttpClientModule in CoreModule

73f47d4

wmarques mentioned this pull request Mar 5, 2018

Fix webpack 4 issues with "start" script #7227

Merged

4 tasks

Polish angular-cli.json

433db96

wmarques changed the title ~~Angular: Lazy load Admin Module~~ Angular: Use ngtools/webpack instead of at-loader Mar 5, 2018

wmarques mentioned this pull request Mar 5, 2018

Angular: Lazy load admin module #7235

Merged

4 tasks

Remove admin module lazy loading

0c6d02c

deepu105 added the v5 label Mar 9, 2018

deepu105 added the pr: on-hold label Mar 12, 2018

sendilkumarn closed this Mar 12, 2018

sendilkumarn mentioned this pull request Mar 12, 2018

Uses ngtools webpack plugin #7282

Closed

jdubois added this to the 5.0.0-beta.0 milestone Apr 3, 2018

Angular: Use ngtools/webpack instead of at-loader #7231

Angular: Use ngtools/webpack instead of at-loader #7231

Conversation

wmarques commented Mar 5, 2018

wmarques commented Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sendilkumarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wmarques commented Mar 5, 2018

sendilkumarn commented Mar 5, 2018

wmarques commented Mar 5, 2018 • edited Loading

wmarques commented Mar 5, 2018

sendilkumarn commented Mar 5, 2018

sendilkumarn commented Mar 5, 2018

sendilkumarn commented Mar 6, 2018

deepu105 commented Mar 6, 2018

wmarques commented Mar 6, 2018

deepu105 commented Mar 6, 2018 via email

wmarques commented Mar 7, 2018 • edited Loading

sendilkumarn commented Mar 9, 2018

pascalgrimaud commented Mar 9, 2018

deepu105 commented Mar 9, 2018 via email

wmarques commented Mar 10, 2018

deepu105 commented Mar 10, 2018

deepu105 commented Mar 11, 2018

deepu105 commented Mar 11, 2018

deepu105 commented Mar 11, 2018

deepu105 commented Mar 12, 2018

wmarques commented Mar 5, 2018 •

edited

Loading

wmarques commented Mar 5, 2018 •

edited

Loading

wmarques commented Mar 7, 2018 •

edited

Loading