-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Update Amplify dependencies (#6148) #6282
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 Amplify dependencies (#6148) #6282
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6282 +/- ##
=======================================
Coverage 73.26% 73.26%
=======================================
Files 208 208
Lines 12920 12920
Branches 2432 2525 +93
=======================================
Hits 9466 9466
+ Misses 3291 3263 -28
- Partials 163 191 +28
Continue to review full report at Codecov.
|
packages/api-graphql/package.json
Outdated
| }, | ||
| "peerDependencies": { | ||
| "@aws-amplify/pubsub": "^2.1.1" | ||
| "@aws-amplify/pubsub": "^3.0.0" |
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.
What is the reasoning for updating this?
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.
Lerna doesn't update linked peerDependencies (lerna/lerna#1575 (comment)), so ours our out of sync with what devDependencies.
In this case though (see #6499), a direct dependencies doesn't need to be restated as a peerDependencies (because customers don't need to install it themselves), so this line can be safely removed.
packages/api/package.json
Outdated
| }, | ||
| "peerDependencies": { | ||
| "@aws-amplify/pubsub": "^2.1.1" | ||
| "@aws-amplify/pubsub": "^3.0.0" |
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.
Same callout as above.
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 mentioned this in #6499 as well, but @aws-amplify/api has no direct usage of @aws-amplify/pubsub, so this can be removed. (@aws-amplify/api-graphql has this defined in its peerDependencies)
ericclemmons
left a comment
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.
There are some peerDependencies we can safely remove (similar to #6499) and get this merged in. 🙏
packages/api-graphql/package.json
Outdated
| }, | ||
| "peerDependencies": { | ||
| "@aws-amplify/pubsub": "^2.1.1" | ||
| "@aws-amplify/pubsub": "^3.0.0" |
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.
Lerna doesn't update linked peerDependencies (lerna/lerna#1575 (comment)), so ours our out of sync with what devDependencies.
In this case though (see #6499), a direct dependencies doesn't need to be restated as a peerDependencies (because customers don't need to install it themselves), so this line can be safely removed.
packages/api/package.json
Outdated
| }, | ||
| "peerDependencies": { | ||
| "@aws-amplify/pubsub": "^2.1.1" | ||
| "@aws-amplify/pubsub": "^3.0.0" |
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 mentioned this in #6499 as well, but @aws-amplify/api has no direct usage of @aws-amplify/pubsub, so this can be removed. (@aws-amplify/api-graphql has this defined in its peerDependencies)
sammartinez
left a comment
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.
LGTM 🌮
|
I removed the 2 peer dependencies on |
ericclemmons
left a comment
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.
Thanks for the updates! Once tests pass, we'll get this merged in.
This will be considered a patch release.
Co-authored-by: Sam Martinez <[email protected]> Co-authored-by: Eric Clemmons <[email protected]>
Co-authored-by: Sam Martinez <[email protected]> Co-authored-by: Eric Clemmons <[email protected]>
|
This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs. Looking for a help forum? We recommend joining the Amplify Community Discord server |
Issue #, if available: Fixes #6148
Description of changes:
Updated the inter-package dependencies to use the latest major version.
I did a fairly primitive search using
(aws|amplify).*\^[^3]and then updated anything which had later versions. There is possibly a more thorough / less manual approach.This doesn't attempt to solve this problem in future although I think that would be a good idea. Any thoughts?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.