Upgrade to RN v0.59! #3561
Upgrade to RN v0.59! #3561
Conversation
|
Pushed b862c07 to this branch ; |
|
Pushed a Prettier upgrade, and the fixes it needed, to master as c5b9370..6bfdac6. That represents part of what's in this branch. Also worked out how to make it simpler and more automatic to upgrade arbitrary swathes of our dependencies, which I'll use for other upgrades when picking this up tomorrow. See commit messages. |
|
And pushed to master 6bfdac6..9abec3c, upgrading ESLint and related packages. This involved fixing various small things in our code found by some new or newly-enhanced rules, and disabling other rules that didn't seem appropriate. This should just about clear the way for a |
|
And pushed ba90a44 to this PR branch, rebasing onto the new master. Again |
|
One thing that requires a bit of attention before landing:
... which is apparently because it's running our build on Ubuntu 12.04 Precise?! That's a release from 2012, which went EOL over two years ago: Oh Travis. That info is from "Build system information" at the top... and despite supposedly having "build dist" set to Anyway, more investigation required. |
gnprice
force-pushed
the
upgrade
branch
2 times, most recently
from
874f48a
to
50b17e7
Compare
|
Pushed 50b17e7 here, after pushing 76c33f8..1dd430f (13 commits) to master. The changes now in master include
One new observation that affects the plan:
|
In fact, it lacks a new enough glibc to run our current Flow either! The build has been failing, even in master, for the past week. It appears that Travis has started running our builds on an even more ancient, and farther past EOL, OS release than it was doing before. More details in #3565, which I just filed. |
|
Pushed three commits to master: d083963..64e02a8, including a fix to the Travis issue #3565. Then updated this branch to 678a3e3 . The essential things remaining to do before the upgrade can land are:
|
These aren't really right at this point anyway. But follow upstream at facebook/react-native@1151c096d to keep our diff from upstream small.
|
And pushed 50b12ac to this branch. At this point the main remaining question-mark is to ensure things work on iOS; the branch includes draft changes but they need testing. Pre-empted now by #3567; it'll be harder to accurately test on iOS until that's resolved. [EDIT: Not yet resolved, but I diagnosed it far enough that it no longer interferes with testing other changes, like this one, on iOS.] |
This corresponds to facebook/react-native@a2ef5b85d , one of the changes in the upstream template from RN v0.57 to RN v0.59. It's a bit hard to understand what the commit is saying. The docs it's referring to help a lot -- they've since been deleted, but can be found here: facebook/react-native-website@6bf0a02fd On studying the commit, and the code in RN itself which it interacts with... I'm pretty sure it actually does nothing useful! And it makes this code more complex, and also gives a confusing picture of what the APIs it's using actually do. So, instead of applying it, just add a comment saying why not. Details on reaching that conclusion follow. First: the commit message links to two issues it's supposed to fix. But those are both failures at *build* time, with `main.jsbundle` missing. That commit does nothing at build time. That alone makes it impossible for it to fix the linked bugs, short of some surprising magic that would cause it to affect build-time behavior. No such magic is called out in the commit message (and there's no discussion on the PR, except procedural), so I'm tempted to think the author and reviewer were simply confused here. If they were, they were only as confused as the docs were before that. Well, and after reading RCTBundleURLProvider... it does indeed look like the docs were just confused. First, in the headerdoc for a version of this `jsBundleURLForBundleRoot:` method (in `React/Base/RCTBundleURLProvider.h`): /** * Returns the jsBundleURL for a given bundle entrypoint and * the fallback offline JS bundle if the packager is not running. * if resourceName or extension are nil, "main" and "jsbundle" will be * used, respectively. */ So if the packager isn't running, this code already falls back to getting the `main.jsbundle` resource -- the same thing this new logic in the template also does. Well, but what if a packager is somehow running in release mode? We don't want to use it, so it'd be good to skip even looking for it -- which will also save latency. Is the commit helpful for that reason? In fact, RCTBundleURLProvider takes care of that already too. Looking at the implementation shows that if !RCT_DEV (a synonym of !DEBUG), the packager unconditionally "isn't running" -- the code to look for it is `#if`'d out entirely. So the extra complication in that upstream commit facebook/react-native@a2ef5b85d can't possibly fix the issues it's intended to fix; and in fact it has virtually no effect at all. Therefore, skip it.
This is one of the handful of changes under ios/ in the RN v0.59 upgrade (from v0.57), from commit facebook/react-native@68b0d4d51 . Fixes a bug to make edit/reload cycles in development more reliable. Awesome.
This is a best effort at merging facebook/react-native@3341adac4 with our Xcode config; that's the entire change to this file that appears between our current RN v0.57.8 and the upcoming v0.59.10. Empirically it seems to work, so that's good. There's been a lot of divergence in this file, between the template and our version, that I suspect is mostly accidental. We might be better off discarding our Xcode config entirely and rebuilding it from scratch with a current template. That's out of scope for this upgrade, though. I'm a little puzzled that the app builds and runs just fine both before and after this change. It's clear I don't totally understand what the change is doing. Well, so be it.
This lets us insert fixmes in a separate commit from the upgrade that makes them necessary.
When we upgrade Flow to v0.92 in tandem with RN to v0.59, we'll start getting errors here. Add fixmes for them.
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`.
As the `$FlowFixMe-56` indicates, this was introduced in the upgrade of RN to v0.56 (and corresponding upgrade of Flow)... and the comment doesn't sound like we had much idea at the time what the error actually meant. Now with the upcoming upgrade to RN v0.59 and of Flow to v0.92, the error this applies to disappears. Its disappearance seems just as mysterious as its presence... it could be that it was just a bug and there never was a type error, or it could be that the line the error is reported on has moved around so that it's covered by a different fixme comment, or something else. Well, drop the fixme. This also lets us re-enable failing on unused fixmes.
|
OK, pushed ed490db to this branch. I believe it's good at this point! Doing some double-checks before I push to master. The main changes compared to yesterday's draft are some cleanups of the iOS changes, following testing and investigation of them. |
|
Phew -- and done! 😅 Everything seems to work. Some post-upgrade followups remain; I'll post those separately. See the issue thread #3399 for references. |
Fixes #3399 .
Today I sat down and used
rn-diff-purgeto do this upgrade from scratch, spending a bit less than an hour. This benefits from the considerable work we did this spring to fix type issues found by the newer Flow release (especially inreact-reduxuse), and other changes previously merged; but I left aside the changes remaining in #3422, for the sake of a fresh start.Here's the result. The branch needs some cleaning-up; but at the tip, things seem in pretty good shape. Things that definitely work:
tools/test android flow jest-- so Flow, all unit tests, and the Android debug buildThings that don't:
tools/test lint prettier-- some complaints from the new versions of these tools, need to investigateThings not yet tested:
Also there are some TODO remarks in commit messages.