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

Bump angular dependencies #9825

Merged
merged 6 commits into from
Jun 3, 2019

Conversation

vishal423
Copy link
Contributor

@vishal423 vishal423 commented May 29, 2019

Closes #9817

  • Upgrade core-js

  • Upgrade ng-jhipster

  • Upgrade cache-loader

  • 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

@vishal423
Copy link
Contributor Author

ng-jhipster updates are tracked under jhipster/ng-jhipster#95

@wmarques
Copy link
Contributor

@vishal423 Maybe we can also change the lazy-loading module syntax to the new one using import()

"@fortawesome/angular-fontawesome": "0.4.0",
"@fortawesome/fontawesome-svg-core": "1.2.18",
"@fortawesome/free-solid-svg-icons": "5.8.2",
"@ng-bootstrap/ng-bootstrap": "4.1.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In v4.2, date objects are not being considered as valid (validations inbuilt into ng-bootstrap and introduced in v4.2). There is an open issue to track this: ng-bootstrap/ng-bootstrap#3215

Reverting to version before that works fine.

@vishal423
Copy link
Contributor Author

@wmarques, I need to check on the lazy loading syntax change, specifically for JIT compilation as we use a separate library and I guess that doesn't yet support dynamic imports. I think this change can be made into separate PR.

@vishal423 vishal423 marked this pull request as ready for review May 31, 2019 12:22
@vishal423 vishal423 closed this May 31, 2019
@vishal423 vishal423 reopened this May 31, 2019
@vishal423
Copy link
Contributor Author

I spent some time to leverage new dynamic import syntax and observed few issues:

@wmarques
Copy link
Contributor

wmarques commented Jun 3, 2019

@vishal423 Ok then, let's merge this and we'll look onto that on a separate PR, thanks for your investigation 👍
Don't forget to claim your bug bounty ;)

@wmarques wmarques merged commit ffb0c63 into jhipster:master Jun 3, 2019
@@ -52,6 +52,7 @@ env:
- JHI_DISABLE_WEBPACK_LOGS=true
- JHI_E2E_HEADLESS=true
- JHI_SCRIPTS=$TRAVIS_BUILD_DIR/test-integration/scripts
- NG_CLI_ANALYTICS="false"
Copy link
Member

Choose a reason for hiding this comment

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

@vishal423 : can you explain me what is it plz ? Maybe we need to add this to Azure Pipelines too ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Angular CLI added an analytics feature (like us), this env variable prevents from sending data usage to the CLI project.
I don't think we need to add it into our pipeline as we don't rely on the angular CLI to build our application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In absence of this option, Travis builds got stuck in the post-install scripts (failed build logs @ https://travis-ci.org/jhipster/generator-jhipster/builds/539685496). I haven't noticed this issue on the Azure build pipleines (not sure why such discrepancy).

@DanielFran
Copy link
Member

@wmarques Do not forget that it needs a new version of ng-jhipster.
See jhipster/ng-jhipster#95

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.

Upgrade to Angular 8
4 participants