Pre-RN v0.61 -> v0.62 upgrade changes

[chrisbobbe]

Most of #3782.

I expect this to include the complete set of pre-upgrade changes that correspond to template-app changes as highlighted in the upgrade helper diff that we plan to make (note that some required changes to our own code, like those needed for the Flow version change, will naturally not show up in the diff—and not all of these are included here). A list of exceptions to those changes in the diff is at #3782 (comment). A few post-upgrade commits from that diff will be done in a different PR, #4247.

I'm more confident about the completeness and the ordering of the template-app-diff-related changes than I have been at this stage in previous upgrades because I used Google Sheets to make a checklist and planner, starting from a column on the left with the output of git k v0.61.5..v0.62.2 -- template run on the react-native repo.

The upgrade helper links to a nice-looking guide for how to do the Xcode changes; this was particularly thoughtful, as the changes in the Xcode-related files (like project.pbxproj) can seem opaque and not clearly reproducible. Unfortunately, that guide is pretty out-of-date and misses some reportedly crucial things; I've made notes about specific things, mostly in the giant iOS commit that prepares to enable Flipper.

Also in this PR branch, toward the beginning, are a few commits that prepare for the Flow upgrade and changes to React Native's types across the upgrade. There will almost certainly be more of these, so we might want to think about where these commits should be ordered, before we merge anything (it would be great to have all the Flow-related prep commits kind of bundled together if we can).

[chrisbobbe]

One important question I have, before merging the Android Flipper changes (I'd like to put the answer to this in the appropriate commit message(s)): Why, more precisely, are there no build failures when trying to import the contents of com.facebook.flipper before the main upgrade commit? On iOS, the Flipper dependencies are downloaded from CocoaPods, so those dependencies can be added at any time.

On Android, I suspect this nice lack of build failures is associated with the premature release of facebook/react-native@3a66fc7dc in v0.61, but I'd like to explain better what's happening; in particular, there's a use of "reflection" in that commit, and I'm not sure if it suppresses any potential errors ("com.facebook.flipper.ReactNativeFlipper" appears as a string literal?).

[chrisbobbe]

Also, as noted in discussion, we may need to be on react-navigation v4 for this upgrade. Getting to v3 is #3649; v4 is #4248. (EDIT: I've got v3 and v4 open as #4249)

[gnprice]

Excellent! Looking forward to having this in.

I've read up through this point:
13de46b typingReducer [nfc]: Annotate normalizedRecipients variables.
4b2321a UserList [nfc]: Remove style prop.
cf13577 Touchable [nfc]: Inline value for background prop on Android.
2aeaa34 Touchable: Pass 2nd argument of TouchableNativeFeedback.Ripple.
052739f fetchActions tests: Store some messages in variables.
9d13751 Input [nfc]: Remove unused this.textInput.
d2e750c PasswordInput [nfc]: Remove unused this.textInput.
f8d2fe6 Input, SmartUrlInput: Use modern React.createRef API.
26b66c6 ComposeBox [nfc]: Acknowledge and make clear a type-checking lack.
222cc8f deps: Upgrade babel-related dependencies.
8ec0226 deps: Upgrade rollup-related dependencies.
ae3a57a common: Delete unused Arc and Circle.
3cfc830 flow: Follow a React Native change.
744c47d android formatting: Allow blank lines in, e.g., gradle.properties.
11fa114 android build [nfc]: Format files we're about to change.
cdd7ee6 android build [nfc]: Add a lint-suppression line.

and all looks great, modulo small comments below.

Also in this PR branch, toward the beginning, are a few commits that prepare for the Flow upgrade and changes to React Native's types across the upgrade. There will almost certainly be more of these, so we might want to think about where these commits should be ordered, before we merge anything (it would be great to have all the Flow-related prep commits kind of bundled together if we can).

I think the order they're in is fine; even though there may be more like them later, it's helpful to merge swathes of changes when ready so as to reduce merge conflicts and to simplify subsequent rounds of review.

Setting this down for the evening, but when I come back to it I'll look at the remaining commits:
b0c8120 android build: Add androidx.swiperefreshlayout dependency.
aa61bf3 android: Prevent activity recreation on theme change.
3613726 android build: Upgrade Android Gradle Plugin to v3.5.2.
4863776 android build: Update Gradle to 6.0.1.
2b91dc2 android build: Prevent build failure with duplicate libc++_shared.so.
d75dbe7 flipper (android): Prepare to enable Flipper (1/5).
d8b7aa0 flipper (android): Prepare to enable Flipper (2/5).
3f36e56 flipper (android): Prepare to enable Flipper (3/5).
6d6655e flipper (android): Prepare to enable Flipper (4/5).
30d2b1d flipper (android): Prepare to enable Flipper (5/5).
9afb27e ios [nfc]: Remove Facebook copyright notices from some files.
1af5bdb ios build: Turn on "Missing Localizability" for project.
67d7e0a flipper (ios): Prepare to enable Flipper.

[gnprice]

Hmm, I was curious to take a quick stab at adding this to the wrapper -- but I seem to be missing something when I try to reproduce the issue. I edited this to say

  mentionWarnings = React.createRef<MentionWarnings>();

and I don't get any Flow errors. What else should I be looking at?

[chrisbobbe]

Ah, try with this:

mentionWarnings = React.createRef<typeof MentionWarnings>()

I guess the version without typeof isn't the one we want. I'm not really sure why there's a difference in behavior, especially one where neither throws a "you should do it the other way!" error.

[chrisbobbe]

especially one where neither throws a "you should do it the other way!" error.

Like the one noted in this WIP commit on the draft PR #4247 I just sent (for reference) for the actual upgrade and some followup:

504d83d flow fixes (handle properly TODO): TextInput

Cannot use `TextInput` as a type. A name can be used as a type only
if it refers to a type definition, an interface definition, or a
class definition. To get the type of a non-class value, use
`typeof`.

[chrisbobbe]

@gnprice said:

The Flow question is still open for me to perhaps investigate, I guess.

Sure, if you'd like. I had tried for a good while before giving up, but you might catch something I didn't—I remember running into an inexplicable intersection of types with & but not feeling confident enough to fiddle with that (and getting this to work wasn't instrumental to getting the RN upgrade through, anyway).

[gnprice]

Weird that that's no longer in the current docs. Looks like that only describes turning a rule off globally or for a whole Java or Kotlin method, or XML element, at a time. A method can be pretty long! It's good to be able to keep these ignores tightly focused, so they don't accidentally hide unrelated new issues in the future.

And it doesn't seem to describe any way at all that applies to a part of a Groovy file (like all these *.gradle build scripts are.)

Definitely good to have this suppression. I've noticed that warning in Android Studio, and it's better to suppress noise so new/real issues stand out.

[chrisbobbe]

Thanks for the review! I just pushed a new revision answering your comments; I also replied to your question about Flow errors with MentionWarnings, above, at #4244 (comment).

[gnprice]

Thanks @chrisbobbe ! The revision looks good. The Flow question is still open for me to perhaps investigate, I guess.

Now finished reading the rest of the branch; see comments below.

[gnprice]

One thing that may help simplify thinking about this upgrade: the difference between patch releases (v5.6 vs v5.6.1 vs ... vs v5.6.4) should be small and limited to bugfixes. (You're seeing that in the release notes and upgrade guides.) In general it's best to upgrade straight to the latest patch/bugfix release in a given series -- and that's reflected in the fact that, for example, the 5.6.4 release notes are a slightly modified copy of the 5.6 release notes, almost entirely about the differences from 5.5.x, rather than being about the differences from 5.6.3 to 5.6.4.

So for this upgrade I'd go 5.5.1 -> 5.6.4 -> 6.0.1, without examining the older 5.6.x versions.

[chrisbobbe]

In general it's best to upgrade straight to the latest patch/bugfix release in a given series

Mmm, true. In this case, I took an approach of following the React Native changes commit-for-commit (as we do with everything else, to preserve detail or even add more detail), and also of checking the upgrades in those commits against our own procedure for upgrading Gradle, which is to run the ./gradlew command shown at the top of gradle-wrapper.properties. This might reveal whether our upgrade procedure misaligns with theirs in some systematic way (the procedures seem to align well, at least in these upgrades), and also highlight any deviations RN seems to make from their auto-upgrade procedure.

This approach confirms one thing that's already obvious but not explained: the comments added in android/gradle.properties from react-native@be2a2529a (# AndroidX package structure [...]) were almost definitely added by a human at a keyboard, not by an upgrade command (at least not one remotely like the one we're using...but no—that would be silly).

Things got a little weird when I realized that I couldn't just run our command to go from v5.5.1 to v6.0.1 and get the same result as I would by following all the incremental upgrades (modulo the human-added comment I noted above). I suspect this is something common to our upgrade procedure and the one React Native uses.

In particular, take a look at this diff in android/gradlew from facebook/react-native@b1c954b1f (going from v5.6 to v5.6.2):

-# For Cygwin, switch paths to Windows format before running java
-if $cygwin ; then
+# For Cygwin or MSYS, switch paths to Windows format before running java
+if [ "$cygwin" = "true" -o "$msys" = "true" ] ; then

That automatic change does get made if I do a v5.6.1 -> v5.6.2 upgrade with our command. But:

• That change does not get made if I do an all-at-once v5.5.1 -> v6.0.1 upgrade.
• It's also left out if I do a v5.5.1 -> v5.6.2 upgrade.
• It's also left out if I do the usually reasonable v5.5.1 -> v5.6.4 upgrade. (Er...trying this again, it seems like that diff does get applied when I do the other leg, v5.6.4 -> v6.0.1...? Hmm.)
• Er, and now I look closer, it's actually left out when I do v5.6 -> v5.6.2—I realize this is the exact upgrade as facebook/react-native@b1c954b1f does...hmm. But what they probably did (that PR superseded Update Gradle wrapper to 5.6.1 facebook/react-native#26227), and what I probably did as well, is first do v5.6 -> v5.6.1, then, without clearing that result, do v5.6.1 -> v5.6.2.

I think as soon as I had doubts about a more obviously sensible strategy (all-at-once to v6.0.1 was what I'd thought of at first) complicating the path to a clearly correct reproduction recipe of the RN changes, I decided to just follow all the incremental upgrades that they did. I also wanted to fill in some detail about potentially interesting changes with each one—but looking back, a lot of what I came up with is pretty boring, especially for the patch-version upgrades, hmm.

[gnprice]

In particular, take a look at this diff

Hmm, that is some wacky behavior! I don't like that the Gradle wrapper's upgrade behavior seems to be path-dependent like that.

On staring at that a bit, I have a hypothesis:

• Each of the upgrades you tried where that diff didn't get applied were upgrades where you started from a version before v5.6.1; the one upgrade where it did was where you started from that version.
• Maybe the update to the gradlew script depends on the version you start from, rather than the one you upgrade to?

It's at least pretty easy to imagine how that behavior could arise -- you have one version installed, you ask that version to upgrade, but while that existing version is still running it also goes and tries to update your wrapper scripts to the latest and best that it knows about. Not good behavior but easy to imagine.

On that theory I'd expect android/gradlew.bat to behave the same way, for what that's worth. I.e., any given change would appear on the next upgrade after upgrading to some particular version.

---

I also took a look at the Gradle source code. It looks like this change was made first in gradle/gradle@4b849cad3, for a source version of the wrapper script. Then in gradle/gradle@89bce8f they updated the Gradle version used to build Gradle itself (one of many routine such updates), and the same commit also updated the wrapper scripts used for building Gradle itself, picking up the same change. Both of those happened before v5.6.0, so that's still a bit puzzling, though.

[gnprice]

but looking back, a lot of what I came up with is pretty boring, especially for the patch-version upgrades, hmm.

Yeah. I think that's at the core of why I'd prefer to condense these -- there's a lot of detail here, and it makes it harder to focus on the most important information.

I think one structure I'd like for a commit message here would look something like this:

• This corresponds to the following five RN commits: [list the five commits]
• We treat this as an upgrade from 5.5.x to the latest 5.6.x and then to 6.0.1, which is the latest 6.0.x.
  • One of the RN upstream commits also added some comments in gradle.properties, which we reproduce.
  • We ran into a couple of different build failures; so we also add those to the troubleshooting doc, with the cache-clearing commands we used to resolve them (from SO: [link]).
• Release notes and upgrade guides, for 5.6.4 and 6.0.1.
  • Didn't spot any deprecations or breaking changes as applying to us; though hard to tell, and hope we'd catch any from just trying the build.
  • They recommend this build scan. First one found no deprecations, but second found those two things.
  • Also spotted at the console some deprecation warnings from the Android Gradle Plugin rather than Gradle itself, applying to these three of our dependencies (with our issues etc.).

One thing I like about that structure is that it collects all the information of a given type in one place, so it's easier to see the full list: in particular the list of upstream commits, and then the list of what was done in the commit. Relatedly, it puts those two things up front, and only after them the longer text describing things we looked at that (were important to check but) didn't end up resulting in any change appearing in this commit.

I expect it'd also turn out somewhat shorter; mainly by suppressing details about the intermediate patch versions, but partly also because when all the information of a given type is together that saves some repetition in labeling and framing it.

[chrisbobbe]

Sure, sounds good! And thanks for the investigation and advice!

[gnprice]

guide. The report (https://scans.gradle.com/s/2pgdvrpgnpyms [2])
didn't have a "Deprecations" item in the left nav, as shown in the
guide's screenshot, but it did show some deprecation warnings in the
console. (A similar output was seen from the alternative command
they suggest, `gradle help --warning-mode all`; I ran it with
`./gradlew`.) We see this in the console for three of our
dependencies:

Odd that that part is missing.

Ah, one thing: note that those three console messages contain a link... to Android docs, and not Gradle. So this is probably a message from the Android Gradle Plugin, rather than Gradle core -- and it may not be doing whatever structured internal thing Gradle does to populate that "Deprecations" view.

Though if there just aren't any deprecations visible to Gradle, then it seems like a UI bug in the scan tool that it doesn't show that view anyway and explicitly say it's empty.

[gnprice]

I read through the upgrade guides, and don't see anything I recognize as affecting us other than the compile thing. But there's plenty of Gradle code we're using that isn't ours, so I wouldn't necessarily spot something.

The upgrade guide lists many new deprecations, and we are newly
seeing some deprecation warnings in the build scan (see below).

Do these warnings also show up in the console output of an appropriate Gradle command? The scan tool sounds helpful for browsing and seeing where the usages are; but I'd hope it isn't the only way to learn there's something to warn about at all.

[chrisbobbe]

Hmm, comparing the console output shown by the scan tool for v5.6.4 and v6.0.1, there aren't additional warnings.

an appropriate Gradle command

Aha, so I just tried this with tools/test android --full on v5.5.1, v5.6.4, and v6.0.1. In the output, between the two colorful "Installing unimodules" sections (which we wish didn't show up, as you've said before), there are some notices.

At all three of those versions, those notices are—modulo some changes in ordering (probably because the build process uses parallelization) highlighted by an online diff checker—

Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/expo-constants/android/src/main/java/expo/modules/constants/ConstantsService.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/expo-application/android/src/main/java/expo/modules/application/ApplicationModule.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/expo-file-system/android/src/main/java/expo/modules/filesystem/FileSystemModule.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/@react-native-community/async-storage/android/src/main/java/com/reactnativecommunity/asyncstorage/AsyncStorageModule.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/@react-native-community/cameraroll/android/src/main/java/com/reactnativecommunity/cameraroll/CameraRollModule.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/react-native-device-info/android/src/main/java/com/learnium/RNDeviceInfo/RNDeviceModule.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/react-native-sound/android/src/main/java/com/zmxv/RNSound/RNSoundModule.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/react-native-webview/android/src/main/java/com/reactnativecommunity/webview/RNCWebViewManager.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/@sentry/react-native/android/src/main/java/io/sentry/RNSentryModule.java uses or overrides a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: /Users/chrisbobbe/dev/zulip-mobile/node_modules/unimodules-app-loader/android/src/main/java/org/unimodules/apploader/AppLoaderProvider.java uses unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

[gnprice]

At all three of those versions, those notices are

Interesting. Those appear to be entirely disjoint from the ones seen in that build scan! Specifically in the build scan I see:

BuildListener#buildStarted(Gradle) has been deprecated.
This is scheduled to be removed in Gradle 7.0.
32 usages

The maven plugin has been deprecated.
This is scheduled to be removed in Gradle 7.0.
Please use the maven-publish plugin instead.
19 usages

Whereas all the warnings in the output you quoted refer to Java source files -- so they seem to be about APIs used within the app, not used by its build system.

[gnprice]

I think this line (and its friends in other files) are meant to track when we've gone through the bit of Xcode UI to tell us about things to upgrade. So it'd make most sense to bump them only after going through that UI, and for each change either making it or deciding not to make it. (I.e. basically doing #4112.) Did you take a look at what that shows for us?

I'm happy to apply upstream changes without using that UI or bumping this line, though.

In facebook/react-native@ebb629d05 there's another diff hunk that appears that looks like it isn't reflected here:

-                       developmentRegion = English;
+                       developmentRegion = en;
                        hasScannedForEncodings = 0;
                        knownRegions = (
-                               English,
                                en,
                                Base,
                        );

So I'd guess that Xcode would tell us to make that change too.

[chrisbobbe]

Thanks for catching this!!

[chrisbobbe]

Er, hmm, actually when I go through that process (EDIT: ah, looks like I was addressing a different deprecation warning; see below), that diff hunk doesn't get applied. Looking into this now.

[chrisbobbe]

The diff you've posted above seems to relate to this, from the Xcode 10.2 release notes:

Xcode now more carefully distinguishes between legacy localization identifiers such as “English” and modern localization identifiers such as “en” and represents them both in both project files and the user interface. (45469882)

I found that from a CocoaPods issue, CocoaPods/Xcodeproj#669.

The specific deprecation warning, according to that issue, is this:

Migrating the English, deprecated localization to English is recommended for all projects. This will ensure localized resources are placed in “en.lproj” directories instead of deprecated “English.lproj” directories.

That sure sounds familiar, but I can't seem to get the same warning.

[chrisbobbe]

Aha! So, starting from a point where LastUpgradeVersion and LastUpgradeCheck are 0940 in the three places in the commit in question, then clearing the build folder and quitting and restarting Xcode, I do get that warning:

and the fix does add that hunk.

...Then, it seems Xcode has forgotten about the other warning that I think gave us the CLANG_ANALYZER_LOCALIZABILITY_NONLOCALIZED setting...In the upgrade guide, that looks like this:

In general it's been frustrating trying to see these deprecation warnings (and be able to fix them automatically!) in cases where you'd think they'd apply. Anyway, it seems like we've pretty much nailed down the changes in facebook/react-native@ebb629d05 as things we want to take in this commit, so maybe I'll go ahead and include both.

[chrisbobbe]

Aha, and the "Missing Localizability" change also has a CocoaPods PR: CocoaPods/CocoaPods#9612

[gnprice]

Great!

Does this complete #4112, or is there more to that?

[chrisbobbe]

Well, with some of these notices seemingly randomly absent at times when it seems like they should show up, it's hard to be sure. There are also several of these that seem to apply to our dependencies, not our own project. One that happens to show up now (that I don't remember seeing except recently, maybe yesterday?) is listed under our ZulipMobile workspace but looks like it's about Yoga, which is a React Native thing:

And there's always been a handful under the Pods project:

I don't think there's a strong reason we should touch anything to do with our dependencies; morally, their maintainers should handle them, but also, I suspect anything we do to them might get clobbered by pod install.

There's a handful of deprecation complaints about our native code itself (as opposed to various settings in Xcode) that seem to center on notifications:

But I suspect most of these will go away with #4163. If I remember correctly, a few will remain, but they'll mostly appear alongside comments in the code that acknowledge a deprecation and say that we're using the deprecated API on purpose, for compatibility with older iOS versions.

[gnprice]

There's a lot in this commit!

I agree with the reasoning to squash these together.

One thing I'm doing to help me understand it is looking at the history of upstream commits that touched the relevant files. In particular the Podfile and AppDelegate.m were mostly affected only by the Flipper-relevant changes, so here's a pretty low-noise list of most of the relevant changes:

$ git log --oneline --reverse v0.61.5..v0.62.2 -- template/ios/Podfile template/ios/HelloWorld/AppDelegate.m
82a8080f0 Change podspec name of yoga to Yoga
70274f4e9 Add code to enable Flipper in React Native iOS Template
e8541e03f Add React Dev tools to the default template
cc35d0f1b Fix typos in comments about use_frameworks! (#26381)
4c998fd05 Rename JSCallInvoker{,Holder} to CallInvoker{,Holder}
898b1db6d Fix Flipper integration on iOS and update it (#27426)
8f954b365 Edits to flipper pods comments (#27485)
05f5cb534 initalizeFlipper should be set in template app by default (#27569)
6a10d5dfd Remove FB copyright notices from iOS template (#27725)
b4d1fcfb2 Smoothen Flipper iOS integration (#28044)
ada73a354 Bump FlipperKit version on iOS to be compatible with react-native-fli… (#28225)
4bb17944f Revert "Bump FlipperKit version on iOS to be compatible with react-native-fli… (#28225)"
f6a8452e7 Bump FlipperKit version on iOS to be compatible with react-native-flipper (#28277)
8858d879e Exclude all FlipperKit transitive dependencies from iOS Release builds (#28504)

There's one upstream commit this one mentions it includes that isn't in that list:

e3218a0d9 Remove empty Swift file for Flipper (#27922)

Other than that, I think this commit covers the changes in:

70274f4e9 Add code to enable Flipper in React Native iOS Template
e8541e03f Add React Dev tools to the default template
898b1db6d Fix Flipper integration on iOS and update it (#27426)
8f954b365 Edits to flipper pods comments (#27485)
b4d1fcfb2 Smoothen Flipper iOS integration (#28044)
ada73a354 Bump FlipperKit version on iOS to be compatible with react-native-fli… (#28225)
4bb17944f Revert "Bump FlipperKit version on iOS to be compatible with react-native-fli… (#28225)"
f6a8452e7 Bump FlipperKit version on iOS to be compatible with react-native-flipper (#28277)
8858d879e Exclude all FlipperKit transitive dependencies from iOS Release builds (#28504)

Left for after the main upgrade commit is:

05f5cb534 initalizeFlipper should be set in template app by default (#27569)

Unrelated changes to those files, to be handled elsewhere, are:

82a8080f0 Change podspec name of yoga to Yoga
cc35d0f1b Fix typos in comments about use_frameworks! (#26381)
4c998fd05 Rename JSCallInvoker{,Holder} to CallInvoker{,Holder}
6a10d5dfd Remove FB copyright notices from iOS template (#27725)

[gnprice]

And with that list in mind, looking back at the total changes to those two files:

$ git diff v0.61.5..v0.62.2 -- template/ios/Podfile template/ios/HelloWorld/AppDelegate.m

these changes agree with everything there, except omitting some changes that all look like matches for the five intentionally-omitted commits in those last two lists. So that all looks good.

There's the one change to the Yoga line in the Podfile, which is in this commit but didn't look related when I was comparing those two diffs -- but there it is in 898b1db6d, and rereading I see your discussion of it in the commit message.

[gnprice]

And then from this list of commits left for elsewhere:

05f5cb534 initalizeFlipper should be set in template app by default (#27569)

82a8080f0 Change podspec name of yoga to Yoga
cc35d0f1b Fix typos in comments about use_frameworks! (#26381)
4c998fd05 Rename JSCallInvoker{,Holder} to CallInvoker{,Holder}
6a10d5dfd Remove FB copyright notices from iOS template (#27725)

I see the last one a couple of commits earlier in this branch, and you've discussed the first one. ... And I guess the yoga/Yoga rename happened already in our v0.61 upgrade; must have been backported. The other two are presumably on your list elsewhere, then?

[chrisbobbe]

Yep—how about I share the list with you? I think there are a couple small loose ends that I tidied up without updating the list, but you might spot something I missed. I'm sending you an invite now. (Obviously my "FINAL" marker was just for getting the revision together, pre-code review.)

[chrisbobbe]

cc35d0f1b Fix typos in comments about use_frameworks! (#26381)

I've got this noted as "Minuscule typo correction in comments, and it lands on different text later anyway".

4c998fd05 Rename JSCallInvoker{,Holder} to CallInvoker{,Holder}

That's in

ee3f7a5 main upgrade commit

on my draft #4247.

[chrisbobbe]

OK, I've responded to your comments and pushed a revision. Also, please don't forget to take a look at my question at #4244 (comment) before merging any of the Android Flipper stuff. 🙂

[gnprice]

One important question I have, before merging the Android Flipper changes (I'd like to put the answer to this in the appropriate commit message(s)): Why, more precisely, are there no build failures when trying to import the contents of com.facebook.flipper before the main upgrade commit? On iOS, the Flipper dependencies are downloaded from CocoaPods, so those dependencies can be added at any time.

On Android, I suspect this nice lack of build failures is associated with the premature release of facebook/react-native@3a66fc7 in v0.61, but I'd like to explain better what's happening; in particular, there's a use of "reflection" in that commit, and I'm not sure if it suppresses any potential errors ("com.facebook.flipper.ReactNativeFlipper" appears as a string literal?).

There are no build failures because we have those com.facebook.flipper classes in our dependencies, because of the debugImplementation("com.facebook.flipper:… lines in the app's build.gradle.

Those lines only apply in a debug build, because they're debugImplementation rather than plain implementation. But the source file in our tree that imports from com.facebook.flipper is itself under src/debug, i.e. it's in the debug "source set", so it's only compiled in a debug build and that's fine.

Then in turn because the class (com.zulipmobile.ReactNativeFlipper) built from that file is itself only present in a debug build, we have to avoid importing it outside a debug build too. That's what the reflection code in initializeFlipper is for -- it's doing basically the equivalent of importing that class but at runtime, and then going on to invoke a method on the class.

And the bit of reflection code is surrounded by a try/catch with a list of several exception classes to catch, which are different things that could go wrong in the reflection attempt (class doesn't exist, doesn't have that method, and so on.) In each of those cases it indeed basically ignores the exception, just printing a stack trace for it. (This shouldn't be needed, because it's all also inside an if (BuildConfig.DEBUG). But I guess someone wanted to write defensively.)

(BTW I think there's a cleaner way they could have done this, without any reflection, by using the "source sets" feature a bit more fully. That'd be to have another version of com.zulipmobile.ReactNativeFlipper, in the default source set under src/main/, with the same public method but an empty method body. Then we could import that class the normal way and unconditionally invoke that method on it; and in a non-debug build it'd get the trivial version of the class and do nothing, while in a debug build it'd get the full Flipper experience.)

I don't think any of this changes with the main upgrade commit. Looking at the commits in #4247 that remain after this PR, and filtering to android/:

$ git lsp --reverse pr/4244..pr/4247 android/

the one Flipper-related change I see is that we add a call to that initializeFlipper method, the one that goes and does the reflection.

[gnprice]

OK, merging the first 18/29 of these!

38d8598 typingReducer [nfc]: Annotate normalizedRecipients variables.
78c574a UserList [nfc]: Remove style prop.
f42038d Touchable [nfc]: Inline value for background prop on Android.
cd2e96a Touchable: Pass 2nd argument of TouchableNativeFeedback.Ripple.
f6a3f45 fetchActions tests: Store some messages in variables.
b3a3ce8 Input [nfc]: Remove unused this.textInput.
0c57434 PasswordInput [nfc]: Remove unused this.textInput.
c1f9d15 Input, SmartUrlInput: Use modern React.createRef API.
333c312 ComposeBox [nfc]: Acknowledge and make clear a type-checking lack.
77ba172 deps: Upgrade babel-related dependencies.
5ebc048 deps: Upgrade rollup-related dependencies.
2ab9a19 common: Delete unused Arc and Circle.
6f53032 flow: Follow a React Native change.
08d5bac android formatting: Allow blank lines in, e.g., gradle.properties.
9683540 android build [nfc]: Format files we're about to change.
af407a4 android build [nfc]: Add a lint-suppression line.
d179e90 android build: Add androidx.swiperefreshlayout dependency.
f31b257 android: Prevent activity recreation on theme change.

In the last of those, I edited the commit message reflecting my comment here.

That leaves 11/29 commits on the branch:
bae123e android build: Upgrade Android Gradle Plugin to v3.5.2.
d290aaf android build: Update Gradle to 6.0.1.
d5c4471 android build: Prevent build failure with duplicate libc++_shared.so.
9c01f55 flipper (android): Prepare to enable Flipper (1/5).
e0fcc45 flipper (android): Prepare to enable Flipper (2/5).
bcdfb22 flipper (android): Prepare to enable Flipper (3/5).
c5ca3c5 flipper (android): Prepare to enable Flipper (4/5).
e904ce4 flipper (android): Prepare to enable Flipper (5/5).
967e8ce ios [nfc]: Remove Facebook copyright notices from some files.
142b470 ios build: Address two Xcode deprecation warnings.
f5e75cc flipper (ios): Prepare to enable Flipper.

Most of those are 100% ready, too, but there are some we're still discussing or you'd said you wanted to update the commit message in.

[chrisbobbe]

@gnprice said:

There are no build failures because we have those com.facebook.flipper classes in our dependencies, because of the debugImplementation("com.facebook.flipper:… lines in the app's build.gradle.
[...]

Ah OK great, thanks for the explanation! 🙂

One thing I notice is that there isn't a debugImplementation("com.facebook.flipper:… line in the (1/5) commit (where a reflection for com.facebook.flipper starts happening), but there's still no build failure. If I add a import com.facebook.flipper; line in MainApplication.java at that commit (but leave the app's build.gradle untouched), I do get a "cannot find symbol" error pointing to that added line. So, at that commit, I guess the fact that com.facebook.flipper is "imported" at runtime (via reflection) means we get a reprieve from build failures—and at runtime, there probably is an error about it not being found, but it gets caught in the try/catch, so it isn't a problem?

Incidentally, at that (1/5) commit, the class we want to grab from the "imported" com.facebook.flipper is ReactNativeFlipper. It looks like that class's definition was moved from Facebook's code to the app developer's code (ours) in the (2/5) commit.

[chrisbobbe]

OK, just pushed my latest revision for review.

[gnprice]

So, at that commit, I guess the fact that com.facebook.flipper is "imported" at runtime (via reflection) means we get a reprieve from build failures—and at runtime, there probably is an error about it not being found, but it gets caught in the try/catch, so it isn't a problem?

Yep, that sounds right.

Incidentally, at that (1/5) commit, the class we want to grab from the "imported" com.facebook.flipper is ReactNativeFlipper. It looks like that class's definition was moved from Facebook's code to the app developer's code (ours) in the (2/5) commit.

Yep!

I don't love the design choice to push all that detail into the app -- surely very few apps' developers will want to even learn enough about the guts of Flipper to consider customizing it, so it's just a big blob of copy-pasted code to maintain. (And any apps that did want to customize it could always have done so anyway, and invoked their version instead of a central one.) But for whatever reason that's the choice they seem to have made.

[gnprice]

And merged! Thanks again for all your work on this.

[chrisbobbe]

Thanks for the review! 🙂 The next thing that I'm hoping to land is an upgrade to React Navigation v4 (I have #4249 open for that); you can follow links to where I point out an error message that appears with react-navigation v2 and react-native v0.62, but does not appear with react-navigation v4 and react-native v0.62.