-
-
Notifications
You must be signed in to change notification settings - Fork 198
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
Update to angular 11 #874
Update to angular 11 #874
Conversation
BREAKING CHANGE: update to angular 11
@just-jeb the PR is ready for review. |
@all-contributors please add @jsone-studios for issues and code. |
I've put up a pull request to add @jsone-studios! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! I've only had a few questions, please address them and I'll merge.
@@ -7,41 +7,39 @@ | |||
"build": "yarn ng build", | |||
"test": "yarn ng test", | |||
"lint": "yarn ng lint", | |||
"e2e": "yarn ng e2e", | |||
"postinstall": "ngcc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the ngcc
postinstall has been removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postinstall is no longer recommended (it it faster to process libs that application will import than whole node_modules)
https://github.com/angular/angular/blob/master/CHANGELOG.md#910-2020-03-25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ngcc
produced a warning because of a "solution-style" tsconfig. When I tried to resolve the warning, I found the PR angular/angular#38003 introducing it and found this comment of an angular team member:
Currently our docs do not recommend running ngcc as a postinstall hook, except for a particular scenario related to Angular Universal: https://angular.io/guide/ivy#ivy-and-universalapp-shell.
I tried without the ngcc
postinstall and it did work without any issues. Therefore I have removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... The ngcc
here was rather for example than because of actual need. The thing is that Angular CLI builders (including Karma) use Angular tool chain on order to prepare files for tests, while Jest doesn't. This means that there is no way to run ngcc
programmatically only on libraries that you need (because ts-jest
doesn't utilize Angular tool chain).
Well, there is a way but it's very complicated and requires major rewrite of ts-jest
.
Hence, if your app uses incompatible libs (that were not compiled with Ivy) you have to run ngcc
manually on these libs.
I guess we can remove it from the examples and add to the docs, this should be enough.
@@ -7,40 +7,39 @@ | |||
"build": "yarn ng build", | |||
"test": "yarn ng test", | |||
"lint": "yarn ng lint", | |||
"e2e": "yarn ng e2e", | |||
"postinstall": "ngcc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the ngcc
postinstall has been removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postinstall is no longer recommended (it it faster to process libs that application will import than whole node_modules)
https://github.com/angular/angular/blob/master/CHANGELOG.md#910-2020-03-25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment
"jest-preset-angular": "^8.2.1", | ||
"@angular-devkit/architect": ">=0.1100.0 < 0.1200.0", | ||
"@angular-devkit/core": "^11.0.0", | ||
"jest-preset-angular": "^8.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it closes #854 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does. I have mentioned it in the commit message but forgot to mention it in the PR.
Sorry guys I'm reverting it for now, |
The update actually looks pretty straight forward, but requires some work:
|
Giving it a second thought - I'm gonna release a beta version to let you guys update and will update the |
You can try it now with |
I have tried to install custom webpack builder with @next tag. A few observations:
|
@just-jeb sorry for notifying you, but I would like to know, if you think this is problem on my part or it should be solved by you. Thank you very much 👍 |
@kmaraz I'm not familiar with your specific setup but it does work for most of people so I would assume the problem is on your side. Would you mind sharing your |
I have tested it on a different machine and it worked like a charm :) Thank you for your help. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Peer dependency warnings when installing the builders in an angular 11 project.
Issue Number: #873
What is the new behavior?
No warnings about peer dependencies. This also fixes the failing dependabot PRs #870 #871 and #872.
Does this PR introduce a breaking change?
Other information