Prep for RN v0.65 upgrade #5324
Conversation
There was a problem hiding this comment.
Thanks @chrisbobbe for your careful work on this! All looks good, except I think the last commit's changes are best held back for the main upgrade commit -- see below.
| } | ||
| dependencies { | ||
| classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version" | ||
| classpath 'com.android.tools.build:gradle:4.1.0' | ||
| classpath 'com.android.tools.build:gradle:4.2.1' |
There was a problem hiding this comment.
android build: Upgrade Android Gradle Plugin to v4.2.1
Done to follow the template-app change in
facebook/react-native@7599593b3, on the path to the RN v0.65
upgrade.
Release notes here:
https://developer.android.com/studio/releases/gradle-plugin#4-2-0
Quite a few changes announced; hopefully they're all good ones. Our
tests pass, and a debug build on Android built and ran fine for me.
Cool. I'm looking through those release notes. This one is interesting:
Starting in AGP 4.2, it is no longer possible to override Gradle properties from subprojects. In other words, if you declare a property in a gradle.properties file in a subproject instead of the root project, it will be ignored.
We have a fair number of such files supplied by different third-party libraries. You can see all of them (plus a few that don't apply, in template or example apps rather than live subprojects) like so:
$ find -name gradle.properties | xargs rg -v '^ *($|#)'
E.g.,
./node_modules/react-native-webview/android/gradle.properties
1:ReactNativeWebView_kotlinVersion=1.3.50
2:ReactNativeWebView_webkitVersion=1.4.0
3:ReactNativeWebView_compileSdkVersion=29
4:ReactNativeWebView_buildToolsVersion=29.0.3
5:ReactNativeWebView_targetSdkVersion=28
6:ReactNativeWebView_minSdkVersion=21
Glad to not have any of those applying ad hoc anymore!
I think that's the only one that affects us.
package.json
Outdated
| @@ -52,6 +52,7 @@ | |||
| "react": "17.0.1", | |||
| "react-intl": "5.24.6", | |||
| "react-native": "0.64.3", | |||
| "react-native-codegen": "^0.0.7", | |||
There was a problem hiding this comment.
The RN repo went back and forth between having and not having this
dependency in the template app, and I found it hard to follow why or
what it does. But the RN v0.65 release was made with the dep
included at this version range, so, might as well?
Following your handy links to those commits, it looks like when it got removed from the template dependencies, it was because it got moved inside the react-native NPM package itself.
Thinking about that, I think it means the ideal thing is probably to hold back this change to do in the same commit with the RN upgrade itself. At our current version, this isn't in the (template) package.json because it's closely coupled with the code in react-native itself.
I'm not sure whether adding it to our package.json here would do nothing (and RN would keep on invoking its own internal version of react-native-codegen), or would result in using the new react-native-codegen version from NPM. If the former, then adding it here is potentially confusing because it looks like it does something but doesn't actually. If the latter, then in principle it's incompatible -- our current RN version was explicitly meant to use its internal copy of react-native-codegen, not a separately-released one from NPM.
There was a problem hiding this comment.
Cool, yeah, makes sense.
Following your handy links to those commits
For posterity, those commits were:
facebook/react-native@58c80d4f8
facebook/react-native@e99b8bbb4
facebook/react-native@cd6c9f327
facebook/react-native@98e173445
facebook/react-native@e77595784
I think I'll make the main upgrade commit less verbose by omitting that list and linking here.
….66.0. Done to follow the template-app changes in facebook/react-native@eae7e97f8 facebook/react-native@84a81dac1 facebook/react-native@f1d1e23b2 on the path to the RN v0.65 upgrade. Apparently this is fine; I didn't get a peer-dep warning or any test failures. Finally, run `yarn yarn-deduplicate && yarn` as prompted by `tools/test deps`.
Done to follow the template-app change in facebook/react-native@7599593b3, on the path to the RN v0.65 upgrade. Release notes here: https://developer.android.com/studio/releases/gradle-plugin#4-2-0 Quite a few changes announced; hopefully they're all good ones. Our tests pass, and a debug build on Android built and ran fine for me. Greg points out that this one is interesting and good: > Starting in AGP 4.2, it is no longer possible to override Gradle > properties from subprojects. In other words, if you declare a > property in a gradle.properties file in a subproject instead of > the root project, it will be ignored. For more details on that, see zulip#5324 (comment) . The release notes say Java 8 is now specified by default. So, follow the RN commit and remove the config lines that specify Java 8. The RN commit also bumps the template app's Gradle version to 6.9. We already did that for ours, in fb0cf31.
Done to follow the template-app change in facebook/react-native@9a923be89, on the path to the RN v0.65 upgrade. We've already added lines for mavenCentral, following Expo, in aedc8b8. Here, follow RN in -- well, not quite removing the jcenter lines, but narrowing their scope to a single dependency, with a comment explaining why. For why some lines are added to android/app/build.gradle, see the commit message of the RN commit: > Current flipper depends on Stetho version that is not available on > MavenCentral, so had to exclude and bump the version. A later commit, facebook/react-native@d8b115afa, removes those lines and bumps Flipper to v0.93. We'll assume that's because Flipper v0.93 is sufficiently not-"current" to allow removing the lines, and we'll remove them too when we bump Flipper to v0.93.
Done to follow the template-app change in facebook/react-native@d8b115afa, on the path to the RN v0.65 upgrade. For why we can remove the lines in android/app/build.gradle now, see the recent commit that added them.
And just use $FlowFixMe instead. These aren't places where we're dealing with a `fetch` return value, which is what FixmeUntypedFetchResult is for. The `apiCall` function does do a `fetch`, but it doesn't return the result directly; it does some processing of it. It's not clear what exactly these types should be, but that's because the interesting parts of these values are highly specific to the caller. For now, make a note on `apiCall` about that, with a $FlowFixMe, and let the $FlowFixMe propagate to `apiCall`'s callers. See discussion: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/FixmeUntypedFetchResult/near/1363106
chrisbobbe
force-pushed
the
pr-rn-65-prep
branch
from
8a5883d to
ef6156a
Compare
|
Thanks for the review! Revision pushed, with those changes and an additional commit addressing some linked CZO discussion today. #5330 is also in flight toward this RN upgrade. 🙂 |
|
Thanks! Looks good; merging. |
|
Thanks for the review! |
Note: If you get an error like CocoaPods could not find compatible versions for pod "RCT-Folly" on `yarn` or `pod install` when crossing this commit (checking out a branch that has it from one that doesn't, or vice versa), run `pod update RCT-Folly` from `ios/` and try the command again. Background: facebook/react-native#32659 (comment) In this commit: - Bump `react` and `react-test-renderer` to follow the RN template app - Adapt to changes in Flow: - Two options removed: esproposal.optional_chaining, esproposal.nullish_coalescing - RN no longer clobbers Flow's built-in definitions for `fetch`! - Etc.; see comments - Add react-native-codegen, at ^0.0.7. The RN maintainers moved this dep back and forth between the `react-native` NPM package and the RN template app. For RN v0.65 it ended up in the template app, suggesting that projects using RN v0.65 should depend on it directly. So, do that. See discussion: zulip#5324 (comment)
Note: If you get an error like CocoaPods could not find compatible versions for pod "RCT-Folly" on `yarn` or `pod install` when crossing this commit (checking out a branch that has it from one that doesn't, or vice versa), run `pod update RCT-Folly` from `ios/` and try the command again. Background: facebook/react-native#32659 (comment) In this commit: - Bump `react` and `react-test-renderer` to follow the RN template app - Adapt to changes in Flow: - Two options removed: esproposal.optional_chaining esproposal.nullish_coalescing - RN no longer clobbers Flow's built-in definitions for `fetch`! - Etc.; see comments - Add react-native-codegen, at ^0.0.7. The RN maintainers moved this dep back and forth between the `react-native` NPM package and the RN template app. For RN v0.65 it ended up in the template app, suggesting that projects using RN v0.65 should depend on it directly. So, do that. See discussion: zulip#5324 (comment)
Note: If you get an error like CocoaPods could not find compatible versions for pod "RCT-Folly" on `yarn` or `pod install` when crossing this commit (checking out a branch that has it from one that doesn't, or vice versa), run `pod update RCT-Folly` from `ios/` and try the command again. Background: facebook/react-native#32659 (comment) In this commit: - Bump `react` and `react-test-renderer` to follow the RN template app - Adapt to changes in Flow: - Two options removed: esproposal.optional_chaining esproposal.nullish_coalescing - RN no longer clobbers Flow's built-in definitions for `fetch`! - Etc.; see comments - Add react-native-codegen, at ^0.0.7. The RN maintainers moved this dep back and forth between the `react-native` NPM package and the RN template app. For RN v0.65 it ended up in the template app, suggesting that projects using RN v0.65 should depend on it directly. So, do that. See discussion: zulip#5324 (comment)
Related: #5230