Upgrade react-navigation to v2

[gnprice]

EDIT: This issue is for just the v2 upgrade; the v3 upgrade is now #3649. Original description was for both v2 and v3, below.

---

We use react-navigation for navigation in the app. We're on version 1.6.1. There has been a lot of development since then, with two new major versions: 2 and 3.

The newer releases contain various bugfixes, as well as compatibility with newer other software. Issues blocked on this upgrade include #3066, #3566, #3569; also possibly #2716 and #3370; and PRs #2831 and #3067.

There's partial work toward a v2 upgrade in #2702, and toward v3 in #3502. Filing this issue partly so I have an actual issue to link to rather than saying "the thing that #2702 and #3502 are intended to do" 😉 .

Priority because blocking #3066 which is P1.

[gnprice]

To try to clear the ground for this, I just went and made a fresh reading of the v2 release notes. Two things stand out as affecting us:

• navigate(routeName) in StackNavigator is “less pushy”

  As I wrote back on #2702:

  We do a lot of navigate and no push. And I think the behavior we want is typically what's now push, to the extent that they'd ever differ... the new navigate behavior sounds to me like a mixture of push and pop that will feel flaky and unpredictable as a user (unless it just never ends up being pop.)

• NavigationActions split up according to router

  If you are using NavigationActions.push or other stack-specific actions, you’ll need to import StackActions and use StackActions.push instead.

  We call NavigationActions.navigate, so that'll need to change. Given the above, we'll want it to become StackActions.push.

• StackNavigator, TabNavigator and DrawerNavigator are now deprecated in favour of createStackNavigator, createTabNavigator, and createDrawerNavigator, which are functionally identical but more clearly communicate that they are functions and that they return a component.

  We'll want to follow this renaming. [EDIT: originally put this in the "don't affect us" section]

---

Things I think don't affect us:

• Previously, push only applied to the deepest active stack router. This meant that if you had Stack A > Stack B ...

  We only have one stack navigator.

• For example, if you have a stack navigator with a header, and a drawer inside of that stack, then in some circumstances the title of the stack would change every time you change screens in the drawer. This is because the stack navigator would crawl into child navigators and pull navigationOptions off of the deepest active screen. ...

  Our only use of navigationOptions is for tabBarLabel and tabBarIcon, which it seems unlikely the stack navigator is looking at.

• New API for creating navigators

  We don't do this.

• Drawer routes have been replaced with actions

  We don't use a drawer.

• Navigation actions API overhaul ... this would only impact you if you attempted to use the stack helpers from outside of a stack.

  The only thing we do with the navigation object is look at navigation.state, and in one case call navigation.addListener.

• NavigationActions no longer have toString() implementations

  AFAIK we don't use this. Certainly it shouldn't be relevant in actual app logic; it'd only come up in debugging.

• Previously, TabNavigator would render a navigation bar on the top of the screen on Android and the bottom on iOS. We’ve now pulled these navigators apart, so you can use createBottomTabNavigator and createMaterialTopTabNavigator explicitly depending on what you need.

  We already control this explicitly.

---

"Enhancements" that look relevant:

• State persistence - automatically save state and reload it when the app restarts

  I don't think we want this. Fortunately it appears to be opt-in.

• Add createMaterialBottomTabNavigator for a material design style tab bar.

  This sounds like potentially a thing to switch to for our MainTabs.

• Make StackNavigator keyboard aware -- it hides automatically when you start to swipe back, and refocuses if you cancel the swipe back gesture

  Sounds like a good fix!

• Smoothly transition header visibility in Stack

  Also sounds like a good fix!

• Allow modification of SafeAreaView props

  Looks possibly related to fixing Safe areas are not handled correctly on iPhone X #3066 .

[gnprice]

OK, I have a draft branch for the v2 upgrade:
master...gnprice:nav-upgrade

Passes tools/test --full, and the app seems to work fine. Needs a bit of branch cleanup, and probably a bit more manual testing.

To make the branch I went through making an upgrade from scratch; then I consulted #2702 to compare notes. Differences from #2702 include:

• Switching from navigate to push per the API change mentioned above

• On the tab navigators, following upstream's recommendation to switch to react-navigation-tabs and in particular use their createMaterial{Bottom,Top}TabNavigator. This followed the warnings printed by upstream's code:

    console.warn node_modules/react-navigation/src/react-navigation.js:63
      TabNavigator is deprecated. Please use the createBottomTabNavigator or createMaterialTopTabNavigator instead.
    console.warn node_modules/react-navigation/src/react-navigation.js:174
      TabBarTop is deprecated. Please use the react-navigation-tabs package instead: https://github.com/react-navigation/react-navigation-tabs
    console.warn node_modules/react-navigation/src/react-navigation.js:63
      TabNavigator is deprecated. Please use the createBottomTabNavigator or createMaterialTopTabNavigator instead.
    console.warn node_modules/react-navigation/src/react-navigation.js:180
      TabBarBottom is deprecated. Please use the react-navigation-tabs package instead: https://github.com/react-navigation/react-navigation-tabs

(that output is from Jest tests, in running tools/test).

Looking back at #2702, it didn't pull in react-navigation-tabs, and so in e18f702 it instead used createMaterialTopTabNavigator for the main tabs, which go on the bottom. As noted in #2702 (comment), that caused the tabs to look wrong.

• The Upgrade to React Navigation v2 #2702 branch ended with a change related to SafeAreaView. I don't know whether that was related to the upgrade.

  Haven't yet tested my branch on iOS -- I guess that's a thing to do before merge, specifically on (a simulator for) the iPhone X family.

[gnprice]

• Haven't yet tested my branch on iOS -- I guess that's a thing to do before merge, specifically on (a simulator for) the iPhone X family.

Answer to this (from a simulated iPhone X with iOS 12.4) turns out to be:

• The lightbox is quite broken: the header goes right into the notch.
• Most of the rest of the app is more subtly broken: the app bar is taller than it is on a simulated iPhone 7, and taller than it is on system apps. So I suspect this comes from double-counting the safe area somewhere.
• ... But both of those symptoms are exactly the same already in master. So that isn't a regression in this branch, and we'll deal with it separately, as Safe areas are not handled correctly on iPhone X #3066 .

[gnprice]

OK, and merged a v2 upgrade! It's 8dcc768..0d2066f .

[gnprice]

Split out the v3 upgrade as its own issue #3649 -- what I probably should have done in the first place. And closing this one!

Notably, while the v2 upgrade was P1 because it blocked #3066, the v3 upgrade is not a priority.