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

[angular] Fix vendor and update @angular-builders #13118

Merged
merged 4 commits into from
Nov 23, 2020

Conversation

kaidohallik
Copy link
Contributor

Fixes #13082, follow up to #10624 and #13035

This PR has 2 commits:

Some comments:

  • now we have real Angular 11 and ng serve --hmr is working with full HMR, so deleted current module.hot lines from app.main.ts (which was anyway too simplified and didn't enable full HMR)
  • as @angular-builders@11 gives warning: Option "extractCss" is deprecated: Deprecated since version 11.0. No longer required to disable CSS extraction for HMR. then removed "extractCss": true, from angular.json (latest Angular CLI v11 also doesn't generate this line), tested both dev and prod and css working fine

Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (bellow reviewers) and adding skip-ci label, you can still see CI build result at your branch.

Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
@angular-builders update is recommend per just-jeb/angular-builders#873 (comment)

@kaidohallik kaidohallik marked this pull request as draft November 23, 2020 07:27
@kaidohallik
Copy link
Contributor Author

I converted to draft because adding changes here:

  • as in Angular part we can use HMR now, then for better visibility of this option adding npm script webpack:hmr and adding reference to this into README.md as alternative to npm start, for now for Angular, if Vue and/or React part will set up HMR and add npm script webpack:hmr then can show this section in README.md also for React and/or Vue
  • as app.main and app.module are changing then doing changes to follow Angular Style guide in those files (https://angular.io/guide/styleguide#bootstrapping) as this is related to vendor.ts removal

@kaidohallik kaidohallik marked this pull request as ready for review November 23, 2020 08:45
@wmarques
Copy link
Contributor

Can we always use HMR ? Are there any drawbacks to not always use it for dev?

@kaidohallik
Copy link
Contributor Author

So far as I tested it works well. But Angular CLI ng serve doesn't activate HMR by default, so to follow Angular CLI principles maybe it's better to offer HMR as alternative also in our side?

@kaidohallik kaidohallik marked this pull request as draft November 23, 2020 09:41
Changes done:
* use file name main.ts instead of app.main.ts and move main.ts and polyfills.ts to upper folder
* move day.js import from main.ts to app.module.ts
@kaidohallik kaidohallik marked this pull request as ready for review November 23, 2020 10:10
@kaidohallik
Copy link
Contributor Author

I updated in third commit also main.ts and polyfills.ts location, moved them to upper folder as Angular Style Guide suggests and how Angular CLI generates those files.

@mshima
Copy link
Member

mshima commented Nov 23, 2020

We were using angular 10 compile stack indeed.
Master:

chunk {} common.5da616e29a3a286263a1.js (common) 13.9 kB  [rendered]
chunk {1} runtime.d5744bdaaee5ecc28ae0.js (runtime) 2.92 kB [entry] [rendered]
chunk {2} main.2c37efdf00f81a130127.css, main.1870448ffaa4dc2ddab5.js (main) 804 kB [initial] [rendered]
chunk {3} polyfills.99a8c7dd98120cb67200.js (polyfills) 36.7 kB [initial] [rendered]

With this PR:

Initial Chunk Files               | Names         |       Size
main.1a21d5e2727818d26f4e.js      | main          |  690.67 kB
styles.21e8596da2a9b123fc41.css   | styles        |  147.24 kB
polyfills.edb570e9d76841aef2eb.js | polyfills     |   36.68 kB
runtime.4f8cda741a87d448c534.js   | runtime       |    2.92 kB

Thanks @kaidohallik

@mshima mshima merged commit 4e50655 into jhipster:main Nov 23, 2020
@kaidohallik kaidohallik deleted the fix-vendor branch November 23, 2020 12:24
@wmarques
Copy link
Contributor

Yes @mshima you were totally right about the builder, mybad :) At least now we know what to do for the next update 😄
Thanks for fixing this !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The order of global and vendor scss files is incorrect in angular.json
4 participants