-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
[packages] match few dependencies to the workspace versions #15069
[packages] match few dependencies to the workspace versions #15069
Conversation
Co-authored-by: Expo CI <34669131+expo-ci@users.noreply.github.com>
Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines. I've found some issues in your pull request that should be addressed (click on them for more details) 👇
|
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 but first would like to see an explanation or test plan validating the graphql package ugprade.
@@ -54,7 +54,7 @@ | |||
"babel-plugin-module-resolver": "^4.1.0", | |||
"expo-module-scripts": "^2.0.0", | |||
"fuse.js": "^6.4.6", | |||
"graphql": "^14.2.1", | |||
"graphql": "^15.3.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.
Is this breaking change safe? Cf. https://github.com/graphql/graphql-js/releases/tag/v15.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.
@ide I have updated earlier GraphQL in Home app, and there was a bit of migration/refactor needed, but from my checks it looks like GraphQL in dev-menu
is used very sparse, maybe there are several lines and few method/imports used (all from graphql-tag
package). So it looks like the GraphQL is just a required dependency which is not used directly in the code.
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.
Being sparsely used doesn't mean it's safe to upgrade. I looked into graphql-tag's commit history to see when support for graphql@15 was confirmed/added. It turns out that happened here, which was included in graphql-tag@2.12.0. yarn why
says we use graphql-tag@2.12.15 everywhere so we have the updated version and bumping the graphql version is safe, but we need to understand why dependency upgrades are safe before doing them.
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.
@ide Yup, I agree. Thank you for the check and dependencies relation analysis!
Co-authored-by: Expo CI <34669131+expo-ci@users.noreply.github.com>
Co-authored-by: Expo CI <34669131+expo-ci@users.noreply.github.com>
Why
Refs #14561
How
This PR bumps the few dependencies in the Expo
packages
to match theirs versions to the latest versions used in the workspace.Test Plan
yarn
andyarn test
(if available) commands in the modified packages directories have ended with success.Checklist
expo build
(eg: updated@expo/xdl
).expo prebuild
& EAS Build (eg: updated a module plugin).