RN v0.61 -> v0.62 upgrade followup #4261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrisbobbe
added
dependencies
|
This all LGTM, thanks! Once #3782 is in, please merge at will. I noticed in the jest-expo upgrade commit that it makes |
chrisbobbe
removed
the
blocked on other work
chrisbobbe
force-pushed
the
rn-62-followup
branch
2 times, most recently
from
bafd746
to
f6f3de2
Compare
Done to reduce boring mocks (in particular, remove boring mocks for things from Expo) in our Jest setup. A redo of 62621ef and e40c020, reverted in 347aa96, which we can do following the recent RN upgrade to v0.62 (see 347aa96 for details). Since we're on react@16.11.0, use 16.11.0 as the version range for react-dom, instead of 16.9.0 as we did in 62621ef. (We don't use any code in react-dom, but we unfortunately have to include it to satisfy peer dependencies; see 62621ef.) This time around, we get a peer-dependency warning suggesting we need to install expo-splash-screen: """ warning "jest-expo > @expo/config > @expo/configure-splash-screen@0.1.17" has unmet peer dependency "expo-splash-screen@*". """ So, do that. It really should go under `devDependencies`, since we only need it for jest-expo. But I find that react-native-unimodules is automatically linking `expo-splash-screen`'s native code on iOS and Android, whether it's in `devDependencies` or `dependencies`. Although I didn't encounter problems building and running for release on iOS or Android with it in `devDependencies`, it seems problematic to link native code from a package we've said is for development only. So, reluctantly, say that it's not for development only, by putting it under `dependencies`. We may in fact want to use it in production one day; if we do, we'll probably need to put together a Flow libdef. [1] We can't do the usual thing to prevent it from getting linked, adding lines to `react-native.config.js`. It seems Unimodules isn't informed by that file, which is where React Native's autolinking (a separate process) takes its cues from. There may be a way to tell Unimodules to exclude it, but it seems to mean calling code in two or three places that's mostly meant for internal use; see https://forums.expo.io/t/unimodules-what-else-can-i-exclude/41079 and expo/expo#9736 (comment).
Now that we can, following the recent RN v0.61 -> v0.62 upgrade.
As noted in the comments, Flow has gotten a bit smarter, so we can write this code more straightforwardly.
Prompted by discussion on zulip#4101 [1]. Flow started supporting method calls in optional chains in v0.112.0 [2], but ESLint isn't yet on board [3]; it gives a false positive for `no-unused-expressions`. A comment on the ESLint issue [4] suggests we could be more systematic by using an ESLint plugin from Babel, instead of one-off suppressions of `no-unused-expressions` like this one. That plugin moved from (on NPM) `babel-eslint-plugin` to `@babel/eslint-plugin`. We can't use the new one until we're on ESLint 7 (see zulip#4254), but we could probably use the old one. [1] zulip#4101 (comment) [2] facebook/flow#4303 [3] eslint/eslint#11045 [4] eslint/eslint#11045 (comment)
See discussion [1], where Greg points out that it looks like a
mistake to suppose that this change would come with v0.111; there's
nothing at that version in the changelog [2] that looks related.
He points out that there *is* something related at the entry for
v0.126.0:
"""
* Allow indexers in exact object annotations
"""
And that the minimal example provided works at v0.126.0, but not
before [3]:
```
const val: {| +[string]: number |} = { foo: 3 };
```
There isn't currently a version of React Native out that uses v0.126
or later, unfortunately; v0.63 is the latest, and it uses v0.122.
But react-native's `master` branch is on v0.133, which is actually
the latest!
[1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20v0.2E113.20upgrade/near/1012737
[2] https://github.com/facebook/flow/blob/master/Changelog.md#01110
[3] https://flow.org/try/#0MYewdgzgLgBAbgQwDYC4YG8A+MDa0BOAlmAOYC6aYArgLYBGApvjJgL4wC8GMAZiCGgDMMVgG4AUEA
This is such a big leap between versions that my approach is to just tear down the package and set it up again, all in the same commit. This actually went very smoothly, for a few reasons: - We use a very small part of its API, especially after 18c37ce. - At least in the part we use, the library's own Flow types seem to be up-to-date. (We could have switched entirely to `expo-device`, and we could still do that, except that Expo has abandoned Flow completely, in favor of TypeScript. React Native Community has much better Flow support, generally.) - Since the addition of this package, we started using CocoaPods (33f4b41) and autolinking (a9a9ac7), which make it much easier to manage dependencies that use native code. In this commit: - Look over past commits and the installation instructions for v0.21.5 of this package [1] to see what we need to tear down: - Remove workaround code from 44a7e07; the problem was solved in react-native-device-info/react-native-device-info@95887635, released in v2.1.2. - Keep mock of this library from 8300c9f; experimentally, it's still necessary. - (Nothing else stands out.) - Upgrade version range of react-native-device-info from ^0.21.5 to ^6.0.2. - Skip some manual setup instructions labeled "AndroidX Support" [2] that say to add things in the `ext` block in `android/build.gradle`. - One chunk of these instructions says it's meant for supporting `deviceId` (it probably means `getDeviceId`), with a menu of different choices for that based on what modern new features we want to use. Probably best to make that choice if and when we actually decide to use `getDeviceId`. - Another chunk is labeled "include as needed". It suggests `compileSdkVersion` and `targetSdkVersion` be at least 28, in order to use AndroidX; ours are already. There are a few things there I don't quite understand, but we've been doing fine with AndroidX since we started using it in e433197, and presumably "as needed" implies we would know (or soon find out) if we needed that stuff. - Adjust our runtime code, if necessary (it's not necessary): - The two methods we do use, `getSystemName` and `getSystemVersion`, are still documented [3] [4] with the same usage as before. The changelog [5] doesn't suggest anything about these two methods, except for a blip with `getSystemName` where it had been returning "unknown" near-universally for one or two release candidates of v3, before that was promptly fixed. Manual testing on one physical Android device and one physical iOS device suggests that the same strings are given by those methods before and after the upgrade. (Also, notably, no build failures or runtime errors were observed.) In that experiment, the exported constant in src/utils/userAgent.js was: Before: - "ZulipMobile/27.154 (iOS 13.7)" - "ZulipMobile/27.154 (Android 9)" After: - "ZulipMobile/27.154 (iOS 13.7)" - "ZulipMobile/27.154 (Android 9)" If, one day, we're surprised by some unexpected string being used, we can dig into the implementation difference between v0.21.5 and v6.0.2 of this library and see what changed. [1] https://github.com/react-native-community/react-native-device-info/tree/v0.21.5#installation [2] https://github.com/react-native-community/react-native-device-info/tree/v6.0.2#androidx-support [3] https://github.com/react-native-community/react-native-device-info/tree/v6.0.2#getsystemname [4] https://github.com/react-native-community/react-native-device-info/tree/v6.0.2#getsystemversion [5] https://github.com/react-native-community/react-native-device-info/blob/v6.0.2/CHANGELOG.md Fixes: zulip#4240
Also change the `@unimodules/core` version to match, as we did in e116c3f. Nothing in my reading suggests that we can't do this before the RN v0.61 -> v0.62 upgrade, but I figured the chance of success would be greater if we did it after the upgrade. No breaking changes were specifically announced in the `react-native-unimodules` changelog [1]; however, it does say, "Updated dependencies to match versions included in Expo SDK38." To be safe, I checked the list of breaking changes in the release notes for Expo SDK 38 [2]. Nothing there seems to apply to us; there are several announcements about breaking changes in `expo-*` packages, but we don't use any in that list. Like in e116c3f, we see several changes to `ios/Podfile.lock`. Unlike in e116c3f, we don't see any changes on the Android side, even after building and running the app. After this update, I didn't encounter any build failures or runtime errors on iOS or Android. [1] https://github.com/expo/expo/blob/master/packages/react-native-unimodules/CHANGELOG.md#0101--2020-05-29 [2] https://github.com/expo/expo/blob/master/CHANGELOG.md#3800
Only one breaking change was announced in 7.0.0 [1], and it's for something we don't use: - `Icon.AndroidToolbar` convenience component removed as it was removed from React Native core. [...] So, might as well take the latest version. [1] https://github.com/oblador/react-native-vector-icons/releases/tag/v7.0.0
v7 is the latest, but might as well get there incrementally. The v6 release seems most excited to talk about its new use of the new Context API, which sounds like it might mean a lot of breaking changes we have to address manually (I guess I'm thinking of zulip#4222). But no; in fact, there's just one small announced breaking change that applies to us: - The `withRef` option to `connect` has been replaced with `forwardRef`. If `{forwardRef : true}` has been passed to `connect`, adding a ref to the connected wrapper component will actually return the instance of the wrapped component. So, make that change, including the implied removal of `getWrappedInstance`; that bit of UI works fine on Android and iOS after that removal. There's a FlowTyped libdef for this version, so, take that. An additional Flow error arose, falsely telling consumers of our `connect`-wrapped `MentionWarnings` component that they need to pass props like `auth`; in fact, those are not expected to be passed because they're provided by `connect`. Just as we've done at the `connect` call site, add a temporary $FlowFixMe until we can get `connect` properly type-checked. It looks like we don't want to linger on v6 for very long; v7 addresses some performance complaints that arose in v6. So, we'll move to v7 ASAP.
From the release notes [1]: """ This release has undergone extensive performance benchmarking, and we're confident that it's the fastest version of React-Redux yet! """ Cool! It doesn't necessarily mean a dramatic improvement all across our app (or anywhere); we'd need to do our own testing to establish that. But this is encouraging. This version now uses React Hooks in the `connect` implementation, so they announce that we need to be using React 16.8.4 or later. That's no problem; we're well past that and have been for a while. In fact, the corresponding React peer-dep change is announced as the only breaking public API change. There's a FlowTyped libdef for this version, so, grab that; there are no new Flow errors. [1] https://github.com/reduxjs/react-redux/releases/tag/v7.0.1
Might as well use the most up-to-date version of this tool. Looking at the changelog [1], I don't see anything that should interfere with our previous use of the tool. Version 2 dropped support for Node < 10; we're recommending people to use Node 10. A small handful of flags that we don't use (or don't document using, anyway) have become variadic. `tools/test deps` and `yarn yarn-deduplicate` still run fine at this commit. [1] https://github.com/atlassian/yarn-deduplicate/blob/master/CHANGELOG.md
chrisbobbe
force-pushed
the
rn-62-followup
branch
from
f6f3de2
to
eb8ed32
Compare
Thanks for the review! Done.
Yeah, that's annoying. The latest release of jest-expo, 39.0.0 (this PR has us at 38.0.2), requires RN v0.63 (via its "react" peer-dep) and would still pull in Jest 25: https://github.com/expo/expo/blob/2196cebae55bda181ccceadec809942f51ee9e39/packages/jest-expo/package.json. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #4240
Here is a small handful of commits to tie up some known loose ends after this upgrade (open as #4247, for #3782).
Then, starting from this commit:
4abb1e4 deps: Upgrade
react-native-unimodulesto ^0.10.1, the latest.I've thrown in a few upgrades to some dependencies. I don't have any particular reason to think these can't go before the RN upgrade, but I thought I might as well not attempt them before the upgrade, and risk something going wrong and having to do them after the upgrade anyway. I can put these into another PR, if you'd like, @gnprice.