-
Notifications
You must be signed in to change notification settings - Fork 12k
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
fix(@angular-devkit/build-ng-packagr): add tsickle as optional peer dep #16178
Conversation
This way it'll be automatically updated if present.
afc59ea
to
a55a11f
Compare
this is cool @filipesilva! I didn't know that npm and yarn now supported optional peerDeps. |
btw I came across some issues with tsickle 0.37.1 which we might need to be aware of: angular/angular#33788 |
@IgorMinar, is this safe to merge or are those issues blocking for this PR as well? |
@IgorMinar npm added support in npm/cli#224 and yarn added in yarnpkg/yarn#6487. |
@@ -11,13 +11,19 @@ | |||
"rxjs": "6.5.3" | |||
}, | |||
"peerDependencies": { | |||
"ng-packagr": "^9.0.0-rc.0" | |||
"ng-packagr": "^9.0.0-rc.0", | |||
"tsickle": "~0.37.1" |
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 was under the impression that the fix for this was the include the dependency in the packageGroup.
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.
Yeah that was the original idea, but then Charles brough the optional peer dep up that both ensures it is updated, and ensures that if it is present on install (outside of update) it should be the right version. Do you think there are problems with this approach?
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.
Not at all, I was just curios to know why the change :)
Also of note, |
Discussed in a meeting, it's safe to do this because we will be removing it right after during the migration. This will just correct the peerdep failure if any exists. |
Merged to 9.0.x patch: https://github.com/angular/angular-cli/commits/9.0.x. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This way it'll be automatically updated if present.