-
Notifications
You must be signed in to change notification settings - Fork 85
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
[eas-cli] update expo packages to SDK 52 versions #2706
[eas-cli] update expo packages to SDK 52 versions #2706
Conversation
Subscribed to pull request
Generated by CodeMention |
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 codepath where this is not using the versioned config getter for SDK 52? Might be worth focusing on making all versioned packages in eas-cli unused for SDK >= 52/53 like we did for getting the app config.
Upgrading these is always risky (we hit an issue last time we tried to upgrade them IIRC)
Size Change: -264 kB (-0.5%) Total Size: 52.6 MB
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2706 +/- ##
==========================================
+ Coverage 52.74% 52.75% +0.01%
==========================================
Files 580 580
Lines 22283 22272 -11
Branches 4358 4353 -5
==========================================
- Hits 11751 11747 -4
+ Misses 10496 10489 -7
Partials 36 36 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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 codepath where this is not using the versioned config getter for SDK 52? Might be worth focusing on making all versioned packages in eas-cli unused for SDK >= 52/53 like we did for getting the app config. Upgrading these is always risky (we hit an issue last time we tried to upgrade them IIRC)
Discussed it in DMs with @wschurman and it seems like the best option for now is to go forward with this PR as-is for now then do the decoupling for the SDK 53 release. Decoupling seems like a right long-term call allowing us to avoid many of these upgrade-related issues in the future.
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.
It seems like this removed privacy
thing is not a breaking change because ...This field was previously used when the Expo website had optional public-facing pages for projects, which we no longer provide.
Is it true @wschurman?
Nice catch! This should fix it. https://app.graphite.dev/github/pr/expo/universe/17574/www-No-longer-require-obsolete-privacy-setting-in-app-creation-mutation |
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.
Let's rebase on that www PR once approved and landed unless getting this out is urgent in which case leaving AppPrivay.Unlisted
is fine for now (the field is a no-op anyways, doesn't do anything).
@@ -149,7 +150,7 @@ async function ensureEASUpdatesIsConfiguredInExpoConfigAsync({ | |||
return { | |||
projectChanged: true, | |||
// TODO(cedric): fix return type of `modifyConfigAsync` to avoid `null` for type === success repsonses |
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.
yep, this modifyConfigAsync
is probably a good thing to version next. We versioned syncUpdatesConfigurationAsync
by creating a CLI for it in expo-updates.
Thanks! |
0db357b
to
89aec94
Compare
✅ Thank you for adding the changelog entry! |
Why
Update
@expo
packages to SDK 52 versionsHow
Update
@expo
packages to SDK 52 versionsFixes #2696
It seems like the
privacy
field was removed from the expo config in the SDK 52 update: https://expo.dev/changelog/2024/11-12-sdk-52.I used
AppPrivay.Unlisted
for thecreateApp
mutation input for which this is still required. It was an option selected for an app whenexp.privacy
was undefined.It also seems that
findWorkspaceRoot
function in@expo/package-manager
was changed toresolveWorkspaceRoot
.Test Plan
Tests, test commands manually