Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to RN v0.59 #3399

Closed
gnprice opened this issue Mar 13, 2019 · 6 comments · Fixed by #3561
Closed

Upgrade to RN v0.59 #3399

gnprice opened this issue Mar 13, 2019 · 6 comments · Fixed by #3561
Assignees
Labels
P0 critical Highest priority

Comments

@gnprice
Copy link
Member

gnprice commented Mar 13, 2019

React Native 0.59 is out!
https://facebook.github.io/react-native/blog/2019/03/12/releasing-react-native-059

We should upgrade.

This is a big release with a lot of changes. Major things I'm eager to get include (details in above-linked post):

This was referenced Mar 20, 2019
gnprice added a commit that referenced this issue Apr 11, 2019
Similar to 7037393 a few months ago: done for the same reasons,
and in the exact same way.

The remaining cases that cause errors are mostly examples of #3451,
so we can convert them to exact object types as we fix that.

And then there are a handful of types that are genuinely meant to
be inexact object types.  When we upgrade soon to Flow v0.84 (as
part of #3399), we can make those explicit; tracking that as #3452.
@gnprice
Copy link
Member Author

gnprice commented Apr 15, 2019

Update from a chat with @borisyankov today: there are two areas of work remaining to complete this:

@gnprice
Copy link
Member Author

gnprice commented Jun 13, 2019

OK, reviewing current status, referring to the latest version of the draft PR #3422:

  • Still the Android build changes in android/. These are on my plate now.
  • We're down to a handful of type errors:
    • The screen: HomeTab, screen: PmConversationsCard, etc., in MainTabs.js and StreamTabs.js, give errors. I'm kind of tired of trying to untangle the maze of similarly-named types in the react-navigation libdef -- I think I'm going to just mark these with FlowFixMe and write these off as a react-navigation issue that we can live with.
    • Unused suppression comment 🎉 in AppWithNavigation.js. We can do the same dance we've done before where we disable errors on unused such comments; do the upgrade; remove the now-unused comment; then re-enable the errors. (The enabling and disabling is a one-line change in .flowconfig.)
  • I get an error on running jest: Preset react-native not found. Not sure offhand if we had that error on previous versions of the PR. Probably something easy to fix, but investigation needed.
  • I'll want to go through the sequence of things in the PR and answer the questions I asked in my original review feedback. Most likely I'll end up pulling some pieces out into separate preceding commits, like upgrading various ancillary dependencies, and upgrading libdefs.

@gnprice
Copy link
Member Author

gnprice commented Jun 13, 2019

A few more notes:

  • The recommended tool these days for driving these upgrades is rn-diff-purge.
    • Basically you get a diff between a fresh hello-world app generated from upstream's templates at the two different versions. This is a pretty great technique!
    • For these two versions, it looks like
$ git diff --stat=92 version/{0.57.8,0.59.8}
 RnDiffApp/.babelrc                                         |  3 ---
 RnDiffApp/.flowconfig                                      |  3 +--
 RnDiffApp/__tests__/App-test.js                            | 14 +++++++++++++
 RnDiffApp/android/app/BUCK                                 | 18 ++++------------
 RnDiffApp/android/app/build.gradle                         | 13 ++++++------
 RnDiffApp/android/app/build_defs.bzl                       | 19 +++++++++++++++++
 RnDiffApp/android/app/src/debug/AndroidManifest.xml        |  8 +++++++
 RnDiffApp/android/app/src/main/AndroidManifest.xml         |  4 ++--
 RnDiffApp/android/build.gradle                             | 16 +++++---------
 RnDiffApp/android/gradle/wrapper/gradle-wrapper.properties |  2 +-
 RnDiffApp/babel.config.js                                  |  3 +++
 RnDiffApp/index.js                                         |  4 +++-
 RnDiffApp/ios/RnDiffApp.xcodeproj/project.pbxproj          |  8 +++++++
 RnDiffApp/ios/RnDiffApp/AppDelegate.h                      |  5 +++--
 RnDiffApp/ios/RnDiffApp/AppDelegate.m                      | 29 ++++++++++++++------------
 RnDiffApp/ios/RnDiffApp/main.m                             |  2 +-
 RnDiffApp/ios/RnDiffAppTests/RnDiffAppTests.m              |  2 +-
 RnDiffApp/metro.config.js                                  | 17 +++++++++++++++
 RnDiffApp/package.json                                     | 16 +++++++-------
 19 files changed, 122 insertions(+), 64 deletions(-)

(The number after --stat= is just the width to tell git diff to print at; that's what it takes to avoid an ellipsis on the long gradle-wrapper pathname.)

  • I'll want to go through that diff -- it'll be quite helpful in answering the questions I mentioned above, and probably separating out some pieces.

  • There's a v0.59.9 now, so we'll want that instead of v0.59.8.

  • There's also a v0.60.0-rc.0 and v0.60.0-rc.1. So, almost time for the next upgrade -- we'll aim to do that one faster 😉

@gnprice gnprice added P0 critical Highest priority and removed P1 high-priority labels Jul 8, 2019
@gnprice
Copy link
Member Author

gnprice commented Jul 8, 2019

Bumping to P0 to follow #3323, which this blocks.

@gnprice
Copy link
Member Author

gnprice commented Jul 8, 2019

A couple of miscellaneous links -- things that looked likely useful when I was digging into this for the comments just above -- so I can close those tabs:
react-native-community/discussions-and-proposals#68 (comment)
https://brucelefebvre.com/blog/2019/04/03/react-native-upgrade-by-example/

gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jul 25, 2019
Fixes zulip#3399!

The bulk of the work of this upgrade went into a large number of
commits previous to this one.  The majority of the last few dozen
commits, after 1c86488^, were part of the upgrade; some discussion
in zulip#3561.  An earlier wave of effort focused on getting things working
with the upgrade of Flow to v0.92; see zulip#3450 and especially f8c9810,
and then zulip#3453 and zulip#3442.  Some other early discussion is at zulip#3422.

This commit itself was done mainly by running:

  $ tools/upgrade rn v0.59.10

Then, that tool needs a bit of an update for changes upstream.
Because I'm feeling pressed to get this upgrade out the door (and
to deal with the various separate trouble on iOS), I just took
care of the other relevant packages by hand: updated `@babel/core`
and `metro-react-native-babel-preset` in package.json according to
the diff seen in `rn-diff-purge`, then reran `yarn`.
@gnprice
Copy link
Member Author

gnprice commented Jul 25, 2019

Done! Details in dfbdd97; in more detail in #3561; and more detail in the many commits mentioned in dfbdd97, including the majority of the 58 commits in git log 1c86488ce^..ed490db13.

A few post-upgrade followups remain to do. I'll post those separately, with references to here.

gnprice added a commit that referenced this issue Jul 25, 2019
This is partly a followup task from the RN v0.59 upgrade, #3399;
we've upgraded a lot of dependencies, so we should upgrade the
libdefs to match.

Done with:

  $ yarn update-libdefs

This turned up a couple of type errors, because there's now a real
libdef for WebView!  Marked those with fixmes; will fix properly next.
gnprice added a commit that referenced this issue Jul 26, 2019
This fixes #3323, meeting the Google Play 64-bit requirement that
comes into effect next week, 2019-08-01 -- in other words, it makes it
possible for us to continue uploading new releases after that date.

Aside from ticking a box, it's expected to improve performance on
devices with 64-bit CPUs -- which is most new devices of the last few
years, and 85% of all our installs on active devices as reported by
the Play Console.  (In "Release management > Device catalog",
filtering "ABI" to `arm64-v8a` or `x86_64`.)

The main work to make this possible happened in RN upstream; we pulled
it in with our upgrade to RN v0.59, #3399.  This is one last fragment
of the diff in upstream's template app between those versions,
enabling 64-bit versions in our build config.

One unfortunate regression this causes: we're still distributing a
single APK for all architectures, and so adding 64-bit architectures
makes it a lot bigger.  At a quick estimate from comparing
`yarn build:android-nokeys` before and after, we go from 13MB to 22MB.

The Play Store has had a solution to that for a while now, called
"Android App Bundle".  We should switch to that.  We're already
tracking that task as #3547, and this change increases its priority.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 critical Highest priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant