RN 60 upgrade #4133

chrisbobbe · 2020-06-01T20:15:31Z

Closes #3548, #4026. Yay!

In this revision, there are two prettier [nfc] commits that are fine to merge first; just those two are at #4132.

I ran rm -rf node_modules; rm -rf ios/Pods; yarn; tools/test --full at every commit, and they all passed. I opened the app on my iOS device and on the Android emulator (the Android devices has been having "Could not connect to development server" errors), poked around, sent a message, etc.; it all seems to work.

There's just a bit of follow-up work to do, but I'll put that in its own PR so I don't crowd this one.

chrisbobbe · 2020-06-02T01:49:28Z

Oops! I was just reminded by #4089 (comment) and the doc linked from there that we're sticking with Fixes: #..., not Closes: #... for consistency's sake; I'll fix this tomorrow. I'd seen a few instances of Closes: #... recently and thought there might be a new tendency to distinguish fixing broken things from doing tasks, generally. But I agree, consistency is nice.

gnprice

Thanks! Glad this is now nearly complete.

In the main upgrade commit, would it be possible to make a more concise summary near the top of what changed and why? I'm imagining something like

* Upgrade react-native, react, and flow-bin following the templates.

* Upgrade react-native-sound, react-native-webview, and rn-fetch-blob
  as required to satisfy peer-dependencies.

* Adapt our Podfile to RN v0.60's new layout of pods, following the templates;
  details below.

* Update our yarn.lock and Podfile.lock by running `yarn`, and deduplicate
  with `yarn yarn-deduplicate` as prompted by `tools/test deps`.

Ah, aha, as I read more I realize one correction to that: it'd be more like

...

* Upgrade react-native-webview to satisfy peer-dependencies.

* Adapt our Podfile to RN v0.60's new layout of pods, following the templates.

* Upgrade react-native-sound and rn-fetch-blob to versions with podspecs
  compatible with the new pod layout in RN v0.60.

But perhaps that helps illustrate why a summary would be helpful 😉

Maybe also a bullet about how we ignore or have already handled changes to the Xcode and Gradle configs.

Then after the summary, can still go into detail on the more complex of these.

Otherwise this looks good through the upgrade and the FlowFixMe followup! Still reading the commits after that.

src/webview/js/js.js

gnprice · 2020-06-02T03:05:17Z

`d8329e6` android build: Prepare to activate Hermes.

Tiny nit, but I'd more typically say "enable Hermes" or "switch to Hermes".

This part corresponds to

- facebook/react-native@8e375850d
- facebook/react-native@4bb0b4f20 (a partial reversion)

Then:

- In 0.60.1, facebook/react-native@e857d7066 was released,
  giving Hermes support.
- In 0.60.2, facebook/react-native@0738fe573 was released,
  fixing an error in the template app.

I read this as saying this part (this commit?) corresponds to the first two of these. But I think it corresponds to all of them together? I think squashing it like this probably is indeed the cleanest way to show it. We can probably also tell the story more simply to match -- like just list the four commits and say it's the combination of them.

chrisbobbe · 2020-06-02T03:14:01Z

I read this as saying this part (this commit?) corresponds to the first two of these.

Ah, right, thanks for catching that! I think I meant "this part" as "they
started using NPM to manage the JSC version", in the preceding

RN did this in two parts. First, and released in RN v0.60.0, they
started using NPM to manage the JSC version.

and "Then:" referred to the second part (and didn't characterize that second part at all; I should fix that). I agree it's ambiguous; I'll find a way to clear that up.

chrisbobbe · 2020-06-02T03:18:50Z

Thanks for your comments so far! They're all very helpful; I'll address them tomorrow. I wasn't sure if it was possible to adequately summarize all the information but clearly you've gotten most of the way there, so I'll take it from there! 😄

chrisbobbe · 2020-06-02T03:23:10Z

Tiny nit, but I'd more typically say "enable Hermes" or "switch to Hermes".

Ah, haha. I think the variable they use in the template is indeed enableHermes.

Now I have some monstrosity in my mind about a mad scientist in some sci-fi comic evilly commanding "Activate Hermes!" and Percy Weasley's owl Hermes shoots out from somewhere and squawks.

chrisbobbe · 2020-06-02T20:53:40Z

I remembered that we need a libdef for react-native-webview 7. FlowTyped doesn't have one, so I'm putting one together by looking at the FlowTyped one for version 6 (which is available), plus a diff of some TypeScript files in the react-native-webview repo.

chrisbobbe · 2020-06-03T01:49:56Z

Updated this wth the react-native-webview 7 libdef. Ah, and one other change: I upgraded to r-n-webview 7.6.0, instead of 7.0.0, as in the previous revision. Figured it's best to take the latest version we can, and they do specifically claim to follow semantic versioning.

I believe I've also addressed your other comments, but let me know if the summaries / rewordings could be improved, of course. 🙂

gnprice · 2020-06-03T03:20:28Z

Very helpful, thanks!

`2db86bb` Upgrade React Native from v0.59 to v0.60.

- Upgrade `react-native-webview` to satisfy peer dependencies and
  remove the outdated libdef (to be replaced in an upcoming commit).

The removal doesn't happen in this commit, so it's confusing to have it in this list of things that happen.

It was already explained pretty clearly in the commit where it happened, so I think it can just not be mentioned here.
- Upgrade react-native-sound and rn-fetch-blob to versions with
  podspecs compatible with the new pod layout in RN v0.60. A lot of
  work has already been done toward this:
- We ignore or have already handled several changes to the Xcode and
  Gradle configs.
The "A lot of work has …" looks like an editing error.
Oh, and I guess the "We ignore or have already handled …" is maybe better as one of the list after this (of things previously done), too.
This first list could use a short heading saying what it's a list of -- like "In this commit:"
Then, lower down: the discussion of the two declared breaking changes in r-n-webview is quite informative! It kind of gets drowned out a bit by all the details of the peer-dep warnings. How about replacing the first few paragraphs of that section, from "After the upgrade, we get" through "We take 7.6.0, the latest.", with something like this?

We upgrade react-native-webview in this commit because versions before 7.x aren't compatible with RN v0.60, and versions 7.x aren't compatible with RN v0.59. We take the latest 7.x version, 7.6.0.
Hmm, also, why not go all the way to the latest?
One thing that could simplify this a bit: upgrade first to the latest 6.x. That would cover the AndroidX update and also the PR we sent.
Another thing that would simplify it, IIUC, is actually to go in this commit only to 7.0.0 -- then to the latest (or latest 7.x) in a followup commit. That way the one thing that actually requires a code change explained in this giant commit message gets to be pulled out.

later than 7.0.0. We take 7.6.0, the latest. Nicely, this gets us
the changes from one of our PRs, released in 7.0.3; see 1982f3f
and its reversion in the commit that followed, bbfac73.
(This is kind of sad to look at because what that PR did was remove some noise from the Gradle output... and unimodules is now about 20 times noisier 😢, plus a bunch of color.)

There are no updates on the Android side that must happen atomically
with the RN upgrade.

Some Android changes never appear in this series:
I think this whole section can just be dropped; it looks like everything in it is something already explained in another commit.

In general this commit message is quite long, and has a lot of different things explained in it. That can make it a bit of a challenge to follow the explanations of the key points about the changes it does make. So cutting material about changes it doesn't make is helpful for that.

Some iOS changes from the upgrade guide don't appear in this series.
…
Similarly, most of this is summarizing stuff already done and explained in previous commits.

Hmm, I guess one thing that's going on in this list, and also the Android one, is that it provides a sort of cross-reference from the diff hunks in the upgrade-helper output. That's definitely a useful thing to do! It was basically an essential part of the work you did to produce the upgrade, and it's good to keep those resulting notes.

I think this commit message isn't the right medium for it, though, because most of it isn't about this commit and it's quite a lot of material.

A good medium for it might be the GitHub issue, Upgrade to RN v0.60 #3548, as a comment. In fact one major advantage that would have is that, as is, this cross-reference is a bit scattered through different parts of the commit message (here; the Android list above; the short paragraph about "scripts" in package.json); if it's its own GitHub comment, you can collect all those together.

Also as a comment you can edit it as needed; at this point there won't be many edits left, but perhaps a few to link to follow-up commits.

In retrospect and for future upgrades, that could be a good way to track those notes while the work is in progress, too.

gnprice · 2020-06-03T04:39:41Z

`747145f` libdefs: Compose and use libdef for `react-native-webview` at 7.6.

This looks like a lot of work -- thanks!

Would be great to send this as a PR to the flow-typed repo, too.

I tried spot-checking some of the changes, as seen in git diff @~9 -- flow-typed/. They seemed fine, but I didn't attempt to read through all of them.

There was a converter from TS to Flow libdefs that we'd talked about before... ah, Flowgen:
https://github.com/joarwilk/flowgen
and you've tried it once before. Did you try it out here? How'd that go?

`0b03a50` android build: Enable "autolinking".

2. Make the changes indicated by `facebook/react-native@261197d85`,
   where it touches the Android side of the React Native template
   app. In this example, `new MainReactPackage()` is nowhere to be
   seen [3], so, remove that from the list. …

[3]: It isn't present in
     https://github.com/facebook/react-native/blob/769e35ba5f4c31ef913035a5cc8bc0e88546ca55/template/android/app/src/main/java/com/helloworld/MainApplication.java#L22-L28,
     which the "autolinking" doc (footnote 1, above) links to.

It sounds like this is just describing the fact that new MainReactPackage() is removed by that commit. Is there a subtlety here that I'm missing? If not, then it seems like the first sentence covers that, and the second sentence and its footnote aren't needed.

Or, for this whole step:

Apply the Android-side changes from facebook/react-native@261197d85. In getPackages, this removes MainReactPackage and gives a new form to put the references to ZulipNativePackage, NotificationsPackage, and SharingPackage.

4. Add a `react-native.config.js` file, as the recommended way [2]
   to say we don't want certain modules to be linked.

Great!

How did you determine that's the right set of config tweaks to make? In particular that there wasn't some other library that's in our node_modules but we don't want to autolink.

`a1d3609` ios build: Enable "autolinking".

2. Make the changes recommended by these commits in
   `facebook/react-native`. They follow an A, revert-A, A' pattern:

   - 261197d85 Implement changes to enable native modules auto
   linking (#24506)

   - da7d3dfc7 Partially back out #24506, fixing iOS e2e tests
   (#24788)

   - 86a97e783 – Bring back autolinking to the iOS template (#25314)

   It simply imports and runs a command, `use_native_modules`.

I think the method's name is use_native_modules! I.e. the ! is just the last character of its name.

In particular if you look in the file that implements it, the definition looks like:

def use_native_modules!(root = "..", config = nil)
  …

Also it looks like this can be summarized by mentioning just the last commit -- the changes made in A that weren't reverted in A' are all outside template/ios/.

6. Run `pod install` and see that the only change to
   `ios/Podfile.lock` is in the checksum. So, job done.

🎉

gnprice

And done reading the rest of the branch. Thanks again!

(More-substantive comments above.)

gnprice · 2020-06-03T04:21:05Z

android/app/src/main/java/com/zulipmobile/MainApplication.java

+            // Add unimodules
+            List<ReactPackage> unimodules = Arrays.<ReactPackage>asList(
                    new ModuleRegistryAdapter(mModuleRegistryProvider)
            );
+            packages.addAll(unimodules);


Is this different from just packages.add(new ModuleRegistryAdapter(…))?

Hmm, interesting! I don't know what else might be supposed to go in this list, eventually. I was following this: https://gist.github.com/mczernek/9de9e184abc430e9e3508d26738c8a14/revisions

which is linked to from https://github.com/unimodules/react-native-unimodules, as "this diff for react-native >= 0.60".

I think it makes sense to just do packages.add(new ModuleRegistryAdapter(…)).

gnprice · 2020-06-03T04:35:05Z

android/app/build.gradle

@@ -239,3 +267,6 @@ tasks.withType(org.jetbrains.kotlin.gradle.tasks.KotlinCompile).all {
        jvmTarget = "1.8"
    }
 }
+
+// Necessary for Autolinking; see facebook/react-native@261197d85
+apply from: file("../../node_modules/@react-native-community/cli-platform-android/native_modules.gradle"); applyNativeModulesAppBuildGradle(project)


nit: newline at end of line

Also, let's uncramp this line by putting the statements on separate lines. I can see why people jam them into one line when writing instructions and saying to copy-paste; but now that it's part of our source code, it's better to put the code in a reasonable format.

gnprice · 2020-06-03T04:36:24Z

android/settings.gradle

@@ -1,33 +1,7 @@
 rootProject.name = 'ZulipMobile'
-
+apply from: file("../node_modules/@react-native-community/cli-platform-android/native_modules.gradle"); applyNativeModulesSettingsGradle(settings)


similarly, let's uncramp this onto two lines

Also the blank lines seem helpful, separating logical stanzas of related code.

chrisbobbe · 2020-06-03T18:25:46Z

Thanks for the detailed review! I'll respond in chat, starting here.

gnprice · 2020-06-05T22:19:34Z

I think you're still working on the r-n-webview libdefs.

On the rest, just small comments:

5. To compare the list of linked packages before and after this
   commit, note that in both cases, the list to be observed is the
   one returned by the `getPackages` implementation given to the
   abstract class `ReactNativeHost`, where it's constructed in
   `MainApplication.java`.

This isn't entirely right -- the abstract class ReactNativeHost has the implementation it has, and we don't give it a new one. The name for the syntax you're seeing in MainApplication.java looks to be "anonymous class": https://docs.oracle.com/javase/tutorial/java/javaOO/anonymousclasses.html That is, we make a subclass of ReactNativeHost, don't bother to give the class a name, and immediately just make an instance of it (as suggested by the new.)

I think just saying "the one returned by the getPackages implementation in MainApplication.java`" is probably enough.

2. Make the change recommended by `facebook/react-native@86a97e783`:
   import and call a method, `use_native_modules!`.

Why the backticks around the commit ID? Without them, the GitHub view of the commit message will linkify it, which seems useful.

Otherwise all looks good!

chrisbobbe · 2020-06-05T23:15:51Z

This isn't entirely right -- the abstract class ReactNativeHost has the implementation it has, and we don't give it a new one. The name for the syntax you're seeing in MainApplication.java looks to be "anonymous class": https://docs.oracle.com/javase/tutorial/java/javaOO/anonymousclasses.html That is, we make a subclass of ReactNativeHost, don't bother to give the class a name, and immediately just make an instance of it (as suggested by the new.)

Ah OK, good to know, thanks! I wasn't aware of anonymous classes.

chrisbobbe · 2020-06-05T23:33:48Z

Why the backticks around the commit ID?

Habit, I think — using backticks means something is definitely not an English word. But...this is already definitely not an English word! And those links are helpful. GitHub will autolink (ha) to the commit from the facebook/react-native repo, but that's fine.

chrisbobbe · 2020-06-05T23:57:43Z

Thanks for the review! I've pushed the changes with the react-native-webview libdef.

gnprice · 2020-06-08T21:56:29Z

Great! This all looks good -- just a couple of small nits.

The PR is marked as draft; is there a reason that's intentional or did you mean to lift that?

`320ab3a` Upgrade React Native from v0.59 to v0.60.

Fixes: #3548

Fixes: #4093

nit: The "Foo: something" metadata lines at the end go together as one stanza; see for example the commit messages in linux.git. For a specific example, here's the latest non-merge commit at the moment in Linus's tree: torvalds/linux@db33ec3

`37cbe11` libdefs: Compose and use libdef for `react-native-webview` at 7.x.x.

The style prop is probably optional, right?

chrisbobbe · 2020-06-08T22:11:30Z

The style prop is probably optional, right?

Mmm, thanks for catching this! Also, I don't remember why I put the + in there; I'm not finding a really suitable example that I would have modeled that on, either in expo-apple-authentication ( a few enum-like types use + there, but that's all) or in ViewProps in react-native/Libraries/Components/View/ViewPropTypes, which is what informs this type.

In fact, it looks like the actual line (line 378) is this:

style?: ?ViewStyleProp,

Not really something we've made a pattern of doing in our code, but maybe I'll use this just for consistency.

chrisbobbe · 2020-06-08T22:12:26Z

and I just un-marked it as a draft! Yeah, I should have done that earlier.

This will be necessary when we move to a custom, better libdef for `react-native-webview` in the RN v0.59 -> v0.60 upgrade.

`react-native-vector-icons` gives two options for setup on Android [1]: - Option: With Gradle (recommended) - Option: Manually When we first installed it in e5cbe3c, we...mostly went with the latter (which is also "with Gradle": it means including the library's Gradle project!), but we didn't follow through with an important edit to `MainApplication.java` as part of that option, or a (silly, as Greg points out [2]) move of some font assets into our source tree. In cd899bb, we accomplished the "With Gradle (recommended)" option. This seems better; we've never needed to do the things described as possible only with the "Manual" setup. But we didn't back out the then-unnecessary code from the "Manual" setup. So, do that now. Greg points out [2] that they're making the Gradle setup a lot more complicated than it really neads to be. [1]: https://github.com/oblador/react-native-vector-icons/tree/v6.6.0#android [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20autolinking/near/894795

This lets us insert fixmes in a separate commit from the upgrade that makes them necessary. Like we did in c542823.

When we upgrade Flow to v0.98 in tandem with RN to v0.60, we'll start getting errors here. Add fixmes for them.

Any later (7.0.0 or above), and we'd see peer dependency warnings about React Native. We'll upgrade to the latest 7.x in the same commit as the upcoming RN v0.60 upgrade. AndroidX support is the one declared breaking change [1] for version 6.x.x. We started using AndroidX in e433197; if we hadn't, we wouldn't be able to take this version. Also update the libdef; FlowTyped conveniently has one for version 6 (but no later): ``` flow-typed install react-native-webview@6 ``` [1]: https://github.com/react-native-community/react-native-webview#versioning

To reduce the size of the upcoming RN v0.60 upgrade. There, we will upgrade to the latest 7.x of `react-native-webview`. There isn't an off-the-shelf libdef from FlowTyped for 7.x, so we compose our own. That addition is pushed out of the main RN upgrade commit, just like this one, to reduce the size of the RN upgrade commit.

Using the RN Upgrade Helper, a web app showing the diff from the release/0.59.10 and the release/0.60.6 branches of the `react-native-community/rn-diff-purge` repo, at https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6. In this commit: - Upgrade `react-native`, `react`, and `flow-bin` following the templates - Upgrade `react-native-webview` to satisfy peer dependencies - Adapt our Podfile to RN v0.60's new layout of pods, following the templates - Upgrade `react-native-sound` and `rn-fetch-blob` to versions with podspecs compatible with the new pod layout in RN v0.60 We've already done a lot of the work toward the upgrade, and we've ignored some changes suggested by the upgrade helper. See comments on zulip#3548. Note: The following warning appears when running Metro and will be fixed with follow-up work (e.g., zulip#4118): ``` warn The following packages use deprecated "rnpm" config that will stop working from next release: - react-native-orientation: https://github.com/yamill/react-native-orientation#readme - rn-fetch-blob: https://npmjs.com/package/rn-fetch-blob ``` ----- Platform-agnostic -------------------------------------------- We upgrade `react-native-webview` in this commit because versions before 7.x aren't compatible with RN v0.60, and versions 7.x aren't compatible with RN v0.59 (judging by peer dependency warnings). We take the latest 7.x version, 7.6.0. Nicely, this gets us the changes from one of our PRs, released in 7.0.3; see 1982f3f and its reversion in the commit that followed, bbfac73. There's just one declared breaking change [1] in 7.x.x: - UIWebView removed (7.0.1). This prompted the `scalesPageToFit` prop to be removed, but we don't use it. The `useWebKit` prop was also removed because it doesn't make sense for it to be anything but `true` now. So, remove our use of it. Also run `yarn yarn-deduplicate && yarn`, as prompted by `tools/test deps`. [1]: https://github.com/react-native-community/react-native-webview#versioning ----- Android ------------------------------------------------------ There are no updates on the Android side that must happen atomically with the RN upgrade. ----- iOS ---------------------------------------------------------- facebook/react-native@2321b3fd7 appears to be the only cause of iOS changes that must happen atomically with the RN v0.60 upgrade. In that commit, all the "subspecs" that were nested under the "React" pod are un-nested so they each become their own pod, with many of these living in their own directory under node_modules/react-native/Libraries [1]. In our own code, this means changing our Podfile to refer to all these new pods, instead of the "React" pod's subspecs. So, do. Our Podfile has a list of "Pods we need that depend on React Native". We must also be sure all the dependencies in this list adapt to facebook/react-native@2321b3fd7. The syntax for pointing explicitly to a subspec is a slash, as in "React/Core" [2]. Knowing this, we check that list for pods that explicitly depended on those subspecs, with "React/[...]": ``` grep -Rl dependency.*React/ --include=\*.podspec \ --exclude="node_modules/react-native/*" node_modules ``` There are two, and they both have new versions that adapt to the new accepted shape: - `zmxv/react-native-sound@2f2c25a69`: "React/Core" -> "React" - `joltup/rn-fetch-blob`, `01f10cb10^..0f6c3e3cc`: "React/Core" -> "React-Core" So, take these new versions, and job done. [1]: They do still live in node_modules. It's possible for pods to be hosted online and downloaded on `pod install`, npm-style. For an example, see the 'Toast' dependency in node_modules/react-native-simple-toast/react-native-simple-toast.podspec. But that's not what's happening here, yet. facebook/react-native@2321b3fd7 hints that this will be the way of the future for React Native, "to make the experience nicer for library consumers". [2]: https://guides.cocoapods.org/syntax/podspec.html#subspec Fixes: zulip#3548 Fixes: zulip#4093

The latest version FlowTyped has a libdef for is 6, unfortunately. Ah, well. This is a best effort at replicating the types at react-native-webview/react-native-webview@c4001338c, tagged as v7.6.0, the latest 7.x. An improvement over the FlowTyped libdef is that WebView's props are now exact. So, e.g., people will be warned not to use the `useWebKit` prop, which was removed in 7.0.0. We found it impractical to correctly express the fact that the `WebView` component also accepts all the props that the `View` component does. Certainly it seems impossible to do so by importing things from React Native [1]. So, a partial reimplementation that allows a `style` prop (typed as an inexact object). See the entry in `docs/howto/libdefs.md` added in this commit for more detail. [1]: zulip#3458 (comment)

The only declared breaking change [1] is the following: """ onNavigationStateChange now triggers with hash url changes """ We haven't been using that prop, so this is fine. (Even if we were...I guess that could be a breaking change?) Inspecting any changed TypeScript files in a local clone of `react-native-webview`, between versions 7.6.0 and 8.0.4, suggests that we don't have to make any changes to the libdef: ``` $ git diff v7.6.0..v8.0.4 -- *.ts ``` (No results.) We'd like to go to the latest (10.2.3, currently), but react-native-webview/react-native-webview@bf1d64571, released in v8.0.5 [2], bumps the `react` peer dependency to ^16.9. We're using `react` at 16.8.6, since `react-native` 0.60.6 has `react` as a peer dependency on 16.8.6 exactly; this gets bumped in facebook/react-native@0ccedf396, released in v0.61.0. So, effectively, this means `react-native-webview` v8.0.5 bumps the React Native requirement to v0.61. Seems surprising that they didn't mention that, but, oh well. [1] https://github.com/react-native-community/react-native-webview#versioning [2] https://github.com/react-native-community/react-native-webview/releases/tag/v8.0.5

This also lets us re-enable failing on unused fixmes.

See zulip#4131, the issue for enabling Hermes. Part of the RN v0.59 -> v0.60 upgrade [1]. This must happen at or after the upgrade commit because the use of Hermes is newly supported. RN did this in two main steps that affected the template app, which we squash together in this commit. RN's first step was to start using NPM to manage the JSC version; this was released in v0.60.0: - facebook/react-native@8e375850d - facebook/react-native@4bb0b4f20 (a partial reversion) RN's second step was a large amount of setup to get Hermes ready, released in v0.60.1: - facebook/react-native@e857d7066 - Then facebook/react-native@0738fe573 fixed a small bug, released in v0.60.2. All four of the above-mentioned commits are reflected in this commit. [1]: https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6

Part of the RN v0.59 -> v0.60 upgrade [1]. (Or, at least, RN and most library maintainers assume it is done as such.) This must happen at or after the upgrade because autolinking is a new feature. "Autolinking" [2] aims to make the "react-native link" command unnecessary, so that when we add a module with native code, all we have to do for the module to be included is run `yarn add`. (Other native-code changes, such as calling functions that the module provides, may still be necessary.) So, for Android: 1. As indicated in the doc [2], unlink everything from Android. As done in rk-for-zulip/zulip-mobile@6b50b1521, we do this with a script, but here, we tell `react-native unlink` to only unlink from Android. To be as exhaustive as possible, we tell it to unlink *all dependencies* in our package.json. Thankfully, if one isn't already linked, unlinking it is a no-op: ``` for dep in `tools/deps`; do react-native unlink --platforms=android "$dep" done ``` As in rk-for-zulip/zulip-mobile@6b50b1521, it looks like a custom script in Sentry removed some setup that we want to remain intact. So, exclude these from the commit: - In `ios/ZulipMobile.xcodeproj/project.pbxproj`, removing the "Upload Debug Symbols to Sentry" PBXShellScriptBuildPhase. We don't want to touch the iOS side at all right now. - In `android/app/build.gradle`, the removal of the line: ``` apply from: "../../node_modules/@sentry/react-native/sentry.gradle" ``` Strangely, `react-native unlink`'s changes in MainApplication.java did not touch the list returned by `getPackages`, but it did remove the imports that many of these list items depend on. So, remove these list items, taking note, for a future step, that a few are left intact: - `new MainReactPackage()` - `new ZulipNativePackage()` - `new NotificationsPackage()` - `new SharingPackage()` 2. Apply the Android-side changes from facebook/react-native@261197d85. In `getPackages`, this removes `MainReactPackage` and gives a new form to put the references to `ZulipNativePackage`, `NotificationsPackage`, and `SharingPackage`. 3. Now, go back to `react-native-unimodules` [3] and follow the "react-native >= 0.60" branch of the "Configure Android" instructions. We had to make some trivial changes in `MainApplication.java`; we push to the list at the now-existing `packages` variable instead of changing the literal list that was previously returned without being stored in a variable. 4. Add a `react-native.config.js` file, as the recommended way [2] to say we don't want certain modules to be linked. For details on `react-native-vector-icons`, see a commit earlier in this series that solidified our choice not to link `VectorIconsPackage`. 5. To compare the list of linked packages before and after this commit, note that in both cases, the list to be observed is the one returned by the `getPackages` implementation in `MainApplication.java`. Autolinking introduces an auto-generated `PackageList` class in `android/app/build/generated/rncli/src/main/java/com/facebook/react/PackageList.java` with a `getPackages` method of its own, which helps compose that list of packages we're observing. The `getPackages` implementation is auto-generated when you build for Android. So, compare (by alphebetizing and diffing) the list before autolinking with the new list put together with help from that auto-generated code. They are the same, so, job done. [1]: https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6 [2]: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md [3]: https://github.com/unimodules/react-native-unimodules

Part of the RN v0.59 -> v0.60 upgrade [1]. (Or, at least, RN and most library maintainers assume it is done as such.) This must happen at or after the upgrade commit because autolinking is a new feature. Just like the previous commit, but for iOS. 1. Following the doc [2] as before, unlink everything. Again, with a script, but this time, only for iOS: ``` for dep in `tools/deps`; do react-native unlink --platforms=ios "$dep" done ``` Exclude the same set of Sentry changes as we did in this step for Android. Also, exclude a change to our Info.plist that would remove the "Fonts provided by application", a.k.a., `UIAppFonts`. This is a setup step [3] that we need to keep. So, we're left with the removal of all items from the "Pods we need that depend on React Native" list. Additionally, remove "libz" from our Xcode config. It was put there by Sentry, as a consequence of running `react-native link` before we were using CocoaPods. But Sentry expresses the same dependency in `Sentry.podspec`. So, having it in our Xcode config is redundant. [4] 2. Make the change recommended by facebook/react-native@86a97e783: import and call a method, `use_native_modules!`. 3. Now, go back to `react-native-unimodules` and follow the ">= 0.60" branch of the "Configure iOS" instructions. Turns out that no changes are needed here; the differences in expressing our React pod dependencies were handled in the main RN v0.60 upgrade commit. 4. Move the two deps under "RN-dependent Pods that we could include, but we've decided not to" into the `react-native.config.js`. 5. Add a workaround in `tools/postinstall` for an issue [5] with running `pod install` from the root directory with `--project-directory=ios`. We tried the workaround suggested in that issue, where we have `use_native_modules!(".")` in the Podfile and still call `pod install --project-directory=ios` from the root directory. (This might have led to a cleaner solution, where we pass "." in the postinstall case, and pass nothing otherwise.) But we got this error, which isn't addressed in that issue, and doesn't seem easy to fix without changes to `native_modules.rb`: ``` [!] No podspec found for `RNCAsyncStorage` in `./node_modules/@react-native-community/async-storage` ``` It's plausible that those investigating this issue were only testing with a template project that didn't have RN-dependent pods like RNCAsyncStorage, so, never saw this error. 6. Run `pod install` and see that the only change to `ios/Podfile.lock` is in the checksum. So, job done. [1]: https://react-native-community.github.io/upgrade-helper/?from=0.59.10&to=0.60.6 [2]: https://github.com/react-native-community/cli/blob/master/docs/autolinking.md [3]: https://github.com/oblador/react-native-vector-icons#option-manually [4]: There's some uncertainty here; we're not seeing `z` or `libz` in ios/Pods/Pods.xcodeproj/project.pbxproj after running `pod install`, with or without autolinking. But it seems to work. More discussion at zulip#4026 (comment) and https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20autolinking/near/884333. [5]: react-native-community/cli#657 Fixes: zulip#4026

gnprice · 2020-06-08T22:25:29Z

OK -- merged! 🎊 This was a big upgrade -- glad it's now complete. 😄

chrisbobbe requested a review from gnprice June 1, 2020 20:15

chrisbobbe force-pushed the rn-60 branch from da94290 to c25981b Compare June 1, 2020 20:16

chrisbobbe added P1 high-priority upstream: RN Issues related to an issue in React Native labels Jun 1, 2020

chrisbobbe mentioned this pull request Jun 2, 2020

ESLint 6.8.0 upgrade #4128

Merged

gnprice reviewed Jun 2, 2020

View reviewed changes

src/webview/js/js.js Outdated Show resolved Hide resolved

chrisbobbe force-pushed the rn-60 branch from c25981b to a1d3609 Compare June 3, 2020 00:06

gnprice reviewed Jun 3, 2020

View reviewed changes

chrisbobbe mentioned this pull request Jun 3, 2020

Upgrade to RN v0.60 #3548

Closed

chrisbobbe force-pushed the rn-60 branch 2 times, most recently from 436c3a3 to 8b3c1aa Compare June 4, 2020 22:07

chrisbobbe marked this pull request as draft June 4, 2020 22:08

chrisbobbe force-pushed the rn-60 branch 2 times, most recently from c1b2f46 to 3cef08b Compare June 4, 2020 22:13

chrisbobbe force-pushed the rn-60 branch from 3cef08b to 37bb053 Compare June 5, 2020 23:56

chrisbobbe force-pushed the rn-60 branch from 37bb053 to ec52c01 Compare June 8, 2020 22:12

chrisbobbe marked this pull request as ready for review June 8, 2020 22:12

chrisbobbe added 14 commits June 8, 2020 15:22

msglist types: Annotate html.

405d95e

This will be necessary when we move to a custom, better libdef for `react-native-webview` in the RN v0.59 -> v0.60 upgrade.

flow: Allow unused fixmes, for now.

a68c15b

This lets us insert fixmes in a separate commit from the upgrade that makes them necessary. Like we did in c542823.

flow: Add fixmes for upcoming upgrade.

b53e992

When we upgrade Flow to v0.98 in tandem with RN to v0.60, we'll start getting errors here. Add fixmes for them.

flow: Fix/remove suppressions as allowed by Flow 0.98.

d96c6ca

This also lets us re-enable failing on unused fixmes.

docs: Add entry for clearing ios/Pods.

203e6e7

gnprice force-pushed the rn-60 branch from ec52c01 to 203e6e7 Compare June 8, 2020 22:24

gnprice merged commit 203e6e7 into zulip:master Jun 8, 2020

chrisbobbe deleted the rn-60 branch November 6, 2020 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RN 60 upgrade #4133

RN 60 upgrade #4133

chrisbobbe commented Jun 1, 2020

chrisbobbe commented Jun 2, 2020

gnprice left a comment

gnprice commented Jun 2, 2020

chrisbobbe commented Jun 2, 2020 •

edited

Loading

chrisbobbe commented Jun 2, 2020

chrisbobbe commented Jun 2, 2020

chrisbobbe commented Jun 2, 2020 •

edited

Loading

chrisbobbe commented Jun 3, 2020 •

edited

Loading

gnprice commented Jun 3, 2020

gnprice commented Jun 3, 2020

gnprice left a comment

gnprice Jun 3, 2020

chrisbobbe Jun 4, 2020

gnprice Jun 3, 2020

gnprice Jun 3, 2020

chrisbobbe commented Jun 3, 2020

gnprice commented Jun 5, 2020

chrisbobbe commented Jun 5, 2020

chrisbobbe commented Jun 5, 2020 •

edited

Loading

chrisbobbe commented Jun 5, 2020

gnprice commented Jun 8, 2020

chrisbobbe commented Jun 8, 2020 •

edited

Loading

chrisbobbe commented Jun 8, 2020

gnprice commented Jun 8, 2020

		@@ -1,33 +1,7 @@
		rootProject.name = 'ZulipMobile'

		apply from: file("../node_modules/@react-native-community/cli-platform-android/native_modules.gradle"); applyNativeModulesSettingsGradle(settings)

RN 60 upgrade #4133

RN 60 upgrade #4133

Conversation

chrisbobbe commented Jun 1, 2020

chrisbobbe commented Jun 2, 2020

gnprice left a comment

Choose a reason for hiding this comment

gnprice commented Jun 2, 2020

d8329e6 android build: Prepare to activate Hermes.

chrisbobbe commented Jun 2, 2020 • edited Loading

chrisbobbe commented Jun 2, 2020

chrisbobbe commented Jun 2, 2020

chrisbobbe commented Jun 2, 2020 • edited Loading

chrisbobbe commented Jun 3, 2020 • edited Loading

gnprice commented Jun 3, 2020

2db86bb Upgrade React Native from v0.59 to v0.60.

gnprice commented Jun 3, 2020

747145f libdefs: Compose and use libdef for react-native-webview at 7.6.

0b03a50 android build: Enable "autolinking".

a1d3609 ios build: Enable "autolinking".

gnprice left a comment

Choose a reason for hiding this comment

gnprice Jun 3, 2020

Choose a reason for hiding this comment

chrisbobbe Jun 4, 2020

Choose a reason for hiding this comment

gnprice Jun 3, 2020

Choose a reason for hiding this comment

gnprice Jun 3, 2020

Choose a reason for hiding this comment

chrisbobbe commented Jun 3, 2020

gnprice commented Jun 5, 2020

chrisbobbe commented Jun 5, 2020

chrisbobbe commented Jun 5, 2020 • edited Loading

chrisbobbe commented Jun 5, 2020

gnprice commented Jun 8, 2020

320ab3a Upgrade React Native from v0.59 to v0.60.

37cbe11 libdefs: Compose and use libdef for react-native-webview at 7.x.x.

chrisbobbe commented Jun 8, 2020 • edited Loading

chrisbobbe commented Jun 8, 2020

gnprice commented Jun 8, 2020

`d8329e6` android build: Prepare to activate Hermes.

chrisbobbe commented Jun 2, 2020 •

edited

Loading

chrisbobbe commented Jun 2, 2020 •

edited

Loading

chrisbobbe commented Jun 3, 2020 •

edited

Loading

`2db86bb` Upgrade React Native from v0.59 to v0.60.

`747145f` libdefs: Compose and use libdef for `react-native-webview` at 7.6.

`0b03a50` android build: Enable "autolinking".

`a1d3609` ios build: Enable "autolinking".

chrisbobbe commented Jun 5, 2020 •

edited

Loading

`320ab3a` Upgrade React Native from v0.59 to v0.60.

`37cbe11` libdefs: Compose and use libdef for `react-native-webview` at 7.x.x.

chrisbobbe commented Jun 8, 2020 •

edited

Loading