-
-
Notifications
You must be signed in to change notification settings - Fork 655
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
deps: Upgrade to Expo SDK 44 #5441
Conversation
From the commit message for the upgrade:
So, here is that elaboration. (Formatted for a commit message because that's where I thought I'd put it, before I decided it's just too long.)
|
Yeah, agreed.
Hmm. It sure does make the yarn.lock a lot longer, which sounds like it's pulling in a lot of dependencies:
But it doesn't increase the total size of node_modules by nearly as much. Before:
After:
So it's adding 111MB of new dependencies, which sounds big… except that we have 1109MB already, so it's only 10%. I think it makes sense to not try to optimize that when the cost would be version skew. |
All that reasoning for upgrades to take or not take makes sense. Thanks for the details!
Hmm, yikes. Ultimately I do expect we'll be on Hermes, and so I guess Hermes Inspector as a result. But until then, we definitely need a way to get a JS debug console. Holding react-native-reanimated back seems like a good plan. |
Thanks for the review!
Just sent #5447 for this. |
203dac8
to
de7fd9d
Compare
de7fd9d
to
6bc87a4
Compare
Revision pushed, and this one includes my current revision for #5449. It may be easier to settle that one first. |
6bc87a4
to
f40d8c2
Compare
Just rebased, so this is ready for review. 🙂 |
f40d8c2
to
76ab9d9
Compare
Just rebased again, so this is ready for review. 🙂 |
I first noticed this while kicking the tires on the upcoming upgrade of expo-screen-orientation. But then I tested on `main` and found the issue is already present, before that upgrade.
We'll use this soon when we upgrade `expo` to 44. I assume, all else being equal, it's better to put this in devDependencies than install it globally, since we want commands to have consistent results across environments. (The guide we're about to follow, to upgrade to Expo 44, would have us install it globally.) It comes with a ton of new dependencies, though, and that might be a reason to not have it in the project. And add .expo/ to our gitignore. It gets created when you run `expo upgrade`, containing a README that says it shouldn't be committed. And an Expo template app has it in the gitignore: https://github.com/expo/expo/blob/202e7517/templates/expo-template-bare-minimum/gitignore#L53 And run `yarn yarn-deduplicate && yarn`, as prompted by `tools/test deps`.
Following the "bare workflow" instructions for upgrading: https://blog.expo.dev/expo-sdk-44-4c4b8306584a#8ff0 When I ran `expo upgrade`, it went through a phase it described as "Updating packages to compatible versions (where known)." This changed a lot of version ranges, even of non-Expo things like react-native-reanimated. I kept some of these and ignored others, and I plan to elaborate on these choices in the GitHub PR thread. I didn't find more info in the CLI's docs about how it chooses these upgrades. I believe the code is https://github.com/expo/expo-cli/blob/expo-cli%405.4.12/packages/expo-cli/src/commands/info/upgradeAsync.ts#L541-L597 and that https://github.com/expo/expo/blob/sdk-44/packages/expo/bundledNativeModules.json gets consulted as part of that. In the "Update your `App.Delegate.m` according to this diff" step, I applied the substance of the change, with a few formatting differences to minimize our diff, on top of some changes I made for the expo-screen-orientation upgrade. That upgrade trimmed out some setup requirements for the package: https://github.com/expo/expo/blob/main/packages/expo-screen-orientation/CHANGELOG.md#410--2021-12-03
All looks good -- merging. Thanks for taking care of this upgrade! |
76ab9d9
to
3edc37a
Compare
Following the "Bare workflow" instructions for upgrading: https://blog.expo.dev/expo-sdk-45-f4e332954a68#5954 As with the last one, 44 in 3edc37a, the `expo upgrade` command went through a phase it described as "Updating packages to compatible versions (where known)," and that changed several of our dependency ranges, even of non-Expo things. (For speculation on what that phase is about, see 3edc37a.) We rejected version-range changes to these libraries that would have resulted in downgrades to the libraries' resolved versions: - react-native-safe-area-context from 4.3.1 to 4.2.4 - react-native-screens from 3.13.1 to 3.11.1 - react-native-webview from 11.22.2 to 11.18.1 We rejected upgrades to @react-native-community/netinfo and react-native-reanimated for the reasons we gave in the Expo 44 upgrade; see those at zulip#5441 (comment) . And we rejected bumping react-native from 0.67.4 to 0.68.2, with optimism that we can get this Expo upgrade done before that RN upgrade, which is zulip#5344. We took the rest. In the step > Update your `App.Delegate.mm` (you should have moved from .m to > .mm in the previous step) according to this diff , the indicated changes just seemed to be the same changes we applied in the Expo 44 upgrade (3edc37a), just rebased atop a RN v0.68 upgrade. (The ".m" to ".mm" change is an example of that.) When we do the RN v0.68, we should study some special code that Expo wants us to add as part of that upgrade, beyond what the RN template app says. But that's a job for later, and I didn't see anything to do right now in this item. I tested basic functionality on my iPhone 13 Pro running iOS 15.6, and on the office Android device, a Samsung Galaxy S9 running Android 9. I didn't see any problems with building or running.
Following the "Bare workflow" instructions for upgrading: https://blog.expo.dev/expo-sdk-45-f4e332954a68#5954 As with the last one, 44 in 3edc37a, the `expo upgrade` command went through a phase it described as "Updating packages to compatible versions (where known)," and that changed several of our dependency ranges, even of non-Expo things. (For speculation on what that phase is about, see 3edc37a.) We rejected version-range changes to these libraries that would have resulted in downgrades to the libraries' resolved versions: - react-native-safe-area-context from 4.3.1 to 4.2.4 - react-native-screens from 3.13.1 to 3.11.1 - react-native-webview from 11.22.2 to 11.18.1 We rejected upgrades to @react-native-community/netinfo and react-native-reanimated for the reasons we gave in the Expo 44 upgrade; see those at zulip#5441 (comment) . And we rejected bumping react-native from 0.67.4 to 0.68.2, with optimism that we can get this Expo upgrade done before that RN upgrade, which is zulip#5344. We took the rest. In the step > Update your `App.Delegate.mm` (you should have moved from .m to > .mm in the previous step) according to this diff , the indicated changes just seemed to be the same changes we applied in the Expo 44 upgrade (3edc37a), just rebased atop a RN v0.68 upgrade. (The ".m" to ".mm" change is an example of that.) When we do the RN v0.68, we should study some special code that Expo wants us to add as part of that upgrade, beyond what the RN template app says. But that's a job for later, and I didn't see anything to do right now in this item. I tested basic functionality on my iPhone 13 Pro running iOS 15.6, and on the office Android device, a Samsung Galaxy S9 running Android 9. I didn't see any problems with building or running.
…Expo This belatedly follows a change in Expo's template app, templates/expo-template-bare-minimum. The change was made in expo/expo@2a0079132, and we missed it in the Expo 44 upgrade (zulip#5441) because Expo's upgrade guide forgot about it. See discussion of this and other changes that were missed: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1452850 This seems to fix a bug I've observed on the office Android device (Samsung Galaxy S9 running Android 9), where if I exit the app via the back button, then re-enter the app, the state gets reset such that we register for a new event queue.
This belatedly follows a change in Expo's template app, templates/expo-template-bare-minimum. The change was made in expo/expo@d20594b55 and expo/expo@b59dbc4d8, and we missed it in the Expo 44 upgrade (zulip#5441) because Expo's upgrade guide forgot about it. See discussion of this and other changes that were missed: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1452850 From reading the Expo commits and their associated PRs, I don't feel like I understand the purpose of this change. But Greg says it doesn't seem unreasonable: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1458126 And nothing seemed broken when I tested the change on my iPhone 13 Pro running iOS 16.1. In particular, since we have a comment mentioning social auth, I tested that I could still log in with Google, and that worked. Those Expo commits also touched a different method, meant to implement an Apple feature called "Handoff": https://support.apple.com/en-us/HT209455 Here, we ignore the changes to that method; we haven't implemented it, and while Handoff could sometimes be useful, it isn't a priority. Also, Greg is skeptical about Expo's template-app changes to that method.
…Expo This belatedly follows a change in Expo's template app, templates/expo-template-bare-minimum. The change was made in expo/expo@2a0079132, and we missed it in the Expo 44 upgrade (zulip#5441) because Expo's upgrade guide forgot about it. See discussion of this and other changes that were missed: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1452850 This seems to fix a bug I've observed on the office Android device (Samsung Galaxy S9 running Android 9), where if I exit the app via the back button, then re-enter the app, the state gets reset such that we register for a new event queue.
This belatedly follows a change in Expo's template app, templates/expo-template-bare-minimum. The change was made in expo/expo@d20594b55 and expo/expo@b59dbc4d8, and we missed it in the Expo 44 upgrade (zulip#5441) because Expo's upgrade guide forgot about it. See discussion of this and other changes that were missed: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1452850 From reading the Expo commits and their associated PRs, I don't feel like I understand the purpose of this change. But Greg says it doesn't seem unreasonable: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1458126 And nothing seemed broken when I tested the change on my iPhone 13 Pro running iOS 16.1. In particular, since we have a comment mentioning social auth, I tested that I could still log in with Google, and that worked. Those Expo commits also touched a different method, meant to implement an Apple feature called "Handoff": https://support.apple.com/en-us/HT209455 Here, we ignore the changes to that method; we haven't implemented it, and while Handoff could sometimes be useful, it isn't a priority. Also, Greg is skeptical about Expo's template-app changes to that method.
…Expo This belatedly follows a change in Expo's template app, templates/expo-template-bare-minimum. The change was made in expo/expo@2a0079132, and we missed it in the Expo 44 upgrade (zulip#5441) because Expo's upgrade guide forgot about it. See discussion of this and other changes that were missed: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1452850 This seems to fix a bug I've observed on the office Android device (Samsung Galaxy S9 running Android 9), where if I exit the app via the back button, then re-enter the app, the state gets reset such that we register for a new event queue.
This belatedly follows a change in Expo's template app, templates/expo-template-bare-minimum. The change was made in expo/expo@d20594b55 and expo/expo@b59dbc4d8, and we missed it in the Expo 44 upgrade (zulip#5441) because Expo's upgrade guide forgot about it. See discussion of this and other changes that were missed: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1452850 From reading the Expo commits and their associated PRs, I don't feel like I understand the purpose of this change. But Greg says it doesn't seem unreasonable: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Expo.2044.20small.20things/near/1458126 And nothing seemed broken when I tested the change on my iPhone 13 Pro running iOS 16.1. In particular, since we have a comment mentioning social auth, I tested that I could still log in with Google, and that worked. Those Expo commits also touched a different method, meant to implement an Apple feature called "Handoff": https://support.apple.com/en-us/HT209455 Here, we ignore the changes to that method; we haven't implemented it, and while Handoff could sometimes be useful, it isn't a priority. Also, Greg is skeptical about Expo's template-app changes to that method.
Following the "Bare workflow" instructions for upgrading: https://blog.expo.dev/expo-sdk-45-f4e332954a68#5954 As with the last one, 44 in 3edc37a, the `expo upgrade` command went through a phase it described as "Updating packages to compatible versions (where known)," and that changed several of our dependency ranges, even of non-Expo things. (For speculation on what that phase is about, see 3edc37a.) We rejected version-range changes to these libraries that would have resulted in downgrades to the libraries' resolved versions: - react-native-safe-area-context from 4.3.1 to 4.2.4 - react-native-screens from 3.13.1 to 3.11.1 - react-native-webview from 11.22.2 to 11.18.1 We rejected upgrades to @react-native-community/netinfo and react-native-reanimated for the reasons we gave in the Expo 44 upgrade; see those at zulip#5441 (comment) . And we rejected bumping react-native from 0.67.4 to 0.68.2, with optimism that we can get this Expo upgrade done before that RN upgrade, which is zulip#5344. We took the rest. For the step > Update your `App.Delegate.mm` (you should have moved from .m to > .mm in the previous step) according to this diff , we found that we'd already taken some of those changes when Expo backported them to the Expo 44 template app. Others, like the .m to .mm rename, we'd like to postpone until we do the RN 68 upgrade. A full audit of Expo's template-app commits from 44 to 45 found no changes we'd be interested in applying, except some related to RN 68 that we'd like to do with that upgrade; I've mentioned those on our RN 68 upgrade issue. See: zulip#5507 (comment) I tested basic functionality on my iPhone 13 Pro running iOS 15.6, and on the office Android device, a Samsung Galaxy S9 running Android 9. I didn't see any problems with building or running.
Following the "Bare workflow" instructions for upgrading: https://blog.expo.dev/expo-sdk-45-f4e332954a68#5954 As with the last one, 44 in 3edc37a, the `expo upgrade` command went through a phase it described as "Updating packages to compatible versions (where known)," and that changed several of our dependency ranges, even of non-Expo things. (For speculation on what that phase is about, see 3edc37a.) We rejected version-range changes to these libraries that would have resulted in downgrades to the libraries' resolved versions: - react-native-safe-area-context from 4.3.1 to 4.2.4 - react-native-screens from 3.13.1 to 3.11.1 - react-native-webview from 11.22.2 to 11.18.1 We rejected upgrades to @react-native-community/netinfo and react-native-reanimated for the reasons we gave in the Expo 44 upgrade; see those at zulip#5441 (comment) . And we rejected bumping react-native from 0.67.4 to 0.68.2, with optimism that we can get this Expo upgrade done before that RN upgrade, which is zulip#5344. We took the rest. For the step > Update your `App.Delegate.mm` (you should have moved from .m to > .mm in the previous step) according to this diff , we found that we'd already taken some of those changes when Expo backported them to the Expo 44 template app. Others, like the .m to .mm rename, we'd like to postpone until we do the RN 68 upgrade. A full audit of Expo's template-app commits from 44 to 45 found no changes we'd be interested in applying, except some related to RN 68 that we'd like to do with that upgrade; I've mentioned those on our RN 68 upgrade issue. See: zulip#5507 (comment) I tested basic functionality on my iPhone 13 Pro running iOS 15.6, and on the office Android device, a Samsung Galaxy S9 running Android 9. I didn't see any problems with building or running.
This unfortunately causes a peer-dep warning: warning " > @react-navigation/material-top-tabs@6.6.13" has incorrect peer dependency "@react-navigation/native@^6.0.0". But no issues have yet appeared in manual testing, and that's consistent with the note on the React Navigation upgrade guide that says, "To make upgrading easier, it is possible to mix packages from the `6.x.x` and `5.x.x` version ranges.": https://reactnavigation.org/docs/upgrading-from-5.x#note-on-mixing-react-navigation-5-and-react-navigation-6-packages Changelog: https://github.com/react-navigation/react-navigation/blob/main/packages/material-top-tabs/CHANGELOG.md Done by reading and following the relevant parts of the React Nav upgrade guide, including general information at the top and also the section specific to Material Top Tabs: https://reactnavigation.org/docs/upgrading-from-5.x/#material-top-tab-navigator Done now because it lets us get rid of react-native-reanimated, as foreshadowed in our React Nav 6 upgrade issue: zulip#4936 (comment) That's helpful because the old version of react-native-reanimated that we're on uses a certain iOS API that Apple identifies as privacy-sensitive, triggering a "Privacy Manifest" requirement: software-mansion/react-native-reanimated#5819 For zulip#5847, we're working on reducing and handling such requirements. That API usage is removed in v2.9.0-rc.0 of Reanimated (see just-linked PR), but I didn't pursue upgrading it because that path seems to require abandoning remote JS debugging, at least according to a note from 2022. For details on that, search for "react-native-reanimated" here: zulip#5441 Related: zulip#5847 Fixes-partly: zulip#4936
Following the "bare workflow" instructions for upgrading:
https://blog.expo.dev/expo-sdk-44-4c4b8306584a#8ff0
Marking as a draft because I'd like to use tsflower for some of the dependencies I'm bumping here. 🙂 I've started a discussion.Done!I've kicked the tires with this revision on iOS and Android, with and without "Debug JS Remotely" activated, and things seem fine.