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

React Navigation v5 upgrade (part 2) #4393

Merged
merged 25 commits into from
Jan 14, 2021

Commits on Jan 13, 2021

  1. screen types [nfc]: Rewrite navigation type for screens on AppNavig…

    …ator.
    
    This is NFC because there are no changes to our effective type
    annotations -- we just restructure our code to be more like the new
    way that's documented [1] for React Navigation v5: alongside each
    navigator, have a "param list" type (here, `AppNavigatorParamList`)
    that maps a route name to that route's params and a generic helper
    type (here, `AppNavigationProp`) that consults it, then have each
    screen use that helper to type its `navigation` prop.
    
    As noted in a recent commit where we added src/nav/globalTypes, we
    might switch to a different structure after the upgrade is complete,
    but we're sticking with the docs for now.
    
    [1] https://reactnavigation.org/docs/typescript
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    71992d2 View commit details
    Browse the repository at this point in the history
  2. screen types [nfc]: Rewrite navigation type for screens on MainTabs.

    Like the previous commit, but for `MainTabs`.
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    b09325a View commit details
    Browse the repository at this point in the history
  3. screen types [nfc]: Rewrite navigation type for screens on `StreamT…

    …abs`.
    
    Like the previous commits, but for `StreamTabs`.
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    48cf5ca View commit details
    Browse the repository at this point in the history
  4. screen types [nfc]: Rewrite navigation type for screens on SharingS…

    …creen.
    
    Like the previous commits, but for `SharingScreen`.
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    c08e882 View commit details
    Browse the repository at this point in the history
  5. tabs: Add elevation: 0 to base tab-nav config's tab bar style.

    The issue is illustrated in a screenshot at
    react-navigation/react-navigation#7299 (comment).
    That's also where I got the idea to set `elevation` to 0.
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    e764419 View commit details
    Browse the repository at this point in the history
  6. MessageReactionList: Pass initialRouteName, even if undefined.

    We were scrupulous about not passing `initialRouteName` at all, in
    some cases, but it turns out that passing `undefined` in those cases
    is fine.
    
    See discussion at
    https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20MessageReactionList/near/1058099.
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    07f748e View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    ac378a7 View commit details
    Browse the repository at this point in the history
  8. ZulipAppContainer: Pass theme prop to AppContainer.

    Both React Navigation v4 and v5 have interfaces for setting the
    theme between light and dark, but they're quite different.
    
    One kind of unfortunate difference is that, in v5, using that
    interface is effectively required, even though we have a so-far
    quite adequate way of managing the theme. React Navigation v5 widely
    applies some styling overrides to its provided components (such as
    the background of a scene in a bottom-tab navigator) even when we
    don't pass a `theme` prop. I'm not sure if it applies them based on
    its own arbitrary default, or if it's listening to the system-wide
    theme setting. But we're left with a choice between papering over
    each of those overridden styles, or taking up their theme interface
    and doing something with it.
    
    We take the latter approach because it's not piecemeal. (It would be
    tricky to find all the forced styles we'd need to override, and it's
    not obvious that we'll always be able to paper over them. One such
    fix is to set a `backgroundColor` within `sceneContainerStyle` in
    `MainTabs` -- but this wouldn't have been possible even a few months
    ago, before @react-navigation/bottom-tabs@5.10.0.)
    
    To smoothen the change toward using the `theme` prop in v5, start
    doing so now, while we're still on v4. Theme support is much more
    rudimentary in v4 (i.e., the `theme` prop here just takes "dark" or
    "light"), but I don't see anything particularly disruptive by
    starting to use it now [1], so, might as well.
    
    [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20theme.20colors/near/1061811
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    59ed926 View commit details
    Browse the repository at this point in the history
  9. nav [nfc]: Remove some soon-to-be-unnecessary comments.

    When we're done upgrading to React Navigation 5, including the
    removal of @react-navigation/compat (which is completely untyped and
    has no existing libdef) comments like these will be unnecessary.
    Currently, our navigators don't check that their attached screens
    are annotated with the correct shape of the `navigation` prop -- so
    we've just been saying they are, in these comments. We'll get those
    checks when the upgrade is complete.
    
    We remove them now to reduce the size of the diffs in the upcoming
    commits.
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    1731f4f View commit details
    Browse the repository at this point in the history
  10. deps: Add libdefs for new, differently named React Navigation packages.

    We can change out the libdefs for the React Navigation v5 upgrade
    outside the main upgrade commit because the packages changed names.
    In v5, they're all under the `@react-navigation` scope. So, add the
    new ones, and plan to clean out the old ones after the upcoming main
    upgrade commit.
    chrisbobbe committed Jan 13, 2021
    Configuration menu
    Copy the full SHA
    64a928f View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    8c51a39 View commit details
    Browse the repository at this point in the history

Commits on Jan 14, 2021

  1. prettier: Ignore flow-typed/@react-navigation.

    As Greg points out [1], it's best to smoothen the path to making a
    PR to the FlowTyped repo with the changes we're about to make to
    this libdef. That means not converting it to our own style, which
    means telling Prettier and ESLint to ignore these files.
    
    ESLint already ignores them because of the `**/flow-typed/**`
    line in `.eslintignore`.
    
    [1] zulip#4393 (comment)
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    f92a8e2 View commit details
    Browse the repository at this point in the history
  2. react-navigation libdef: Make *NavigationProp covariant in RouteName …

    …param.
    
    When we make our own types based on `StackNavigationProp` -- e.g.,
    when we define `AppNavigationProp`, in an upcoming commit -- we'll
    want those types to be covariant in their own `RouteName` param.
    
    That's because we want, e.g., `AppNavigationProp<'realm'>` to be
    recognized as a subtype of
    `AppNavigationProp<'realm' | 'account' | …>`, i.e.,
    `AppNavigationProp<$Keys<AppNavigatorParamList>>`.
    
    So, start by making `NavigationProp`, `StackNavigationProp`, and
    `InexactStackNavigationProp` covariant in their `RouteName` params,
    following discussion at
    zulip#4393 (comment).
    
    Also make the change for types of navigators other than stack
    navigators, across all the libdef files. There's a lot of repetition
    because libdef files can't import types from other files [1].
    
    [1] zulip#3458 (comment)
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    f7c1629 View commit details
    Browse the repository at this point in the history
  3. react-navigation libdef: Fix getRootState.

    It seems that `PossiblyStaleNavigtionState` [1] is the Flow libdef's
    version of what appears in the TypeScript as
    `PartialState<NavigationState>` [2].
    
    In contrast with the Flow libdef, the TypeScript has `getRootState`
    returning the non-"partial", non-"possibly stale" thing, i.e.,
    `NavigationState` [3]. That suggests we should consider making the
    Flow libdef say the same thing, which also has the name
    `NavigationState`.
    
    React Navigation does have a doc on "Partial state objects" [4].
    Based on that, Greg points out [5] that a "stale" nav state is
    accepted as input to various React Navigation options, but a
    non-stale state is always the output -- suggesting that that the
    return value of something like `getRootState` should surely not be
    stale.
    
    Some logging confirms that there aren't prominent cases where
    `stale` is `true` or missing. (Any such cases would suggest that we
    should keep the `PossiblyStaleNavigationState` return type.)
    
    See discussion [6].
    
    [1] https://github.com/flow-typed/flow-typed/blob/master/definitions/npm/%40react-navigation/native_v5.x.x/flow_v0.104.x-/native_v5.x.x.js#L526
    [2] https://github.com/react-navigation/react-navigation/blob/main/packages/routers/src/types.tsx#L58
    [3] https://github.com/react-navigation/react-navigation/blob/main/packages/core/src/types.tsx#L471
    [4] https://reactnavigation.org/docs/navigation-state/#partial-state-objects
    [5] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20.60PossiblyStaleNavigationState.60/near/1097141
    [6] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20.60PossiblyStaleNavigationState.60/near/1096924
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    05d2e0d View commit details
    Browse the repository at this point in the history
  4. deps: Upgrade to React Navigation v5.

    Done mostly with the upgrade guide at
    https://reactnavigation.org/docs/upgrading-from-4.x, but also by
    consulting the v4 and v5 documentation of specific areas.
    
    In this commit:
    
    - Take the packages' name changes (e.g., react-navigation ->
      @react-navigation/native)
    
    - Add react-native-tab-view to handle a peer-dep warning:
    
      warning " > @react-navigation/material-top-tabs@5.2.19" has unmet
      peer dependency "react-native-tab-view@>= 2.0.0".
    
    - Run the usual `yarn yarn-deduplicate && yarn`.
    
    - Use the newly available `NavigationContainer` instead of the
      removed `createAppContainer`. We leave it in its own file
      (src/nav/Zulip { App -> Navigation } Container.js), since it still
      has to handle some special logic:
    
      - setting up the NavigationService with a ref (the initialization
        gets slightly more complicated, with the `onReady` prop)
      - deciding what to pass for the `theme` prop, based on the current
        theme (for more discussion of our use of this prop, see the
        recent commit where we started passing it)
    
    - Update `NavigationService` to handle the new shape of the thing it
      tracks with a ref (`AppContainer` -> `NavigationContainer`), and
      handle some new initialization logic for `NavigationContainer`
      (its `onReady` prop).
    
    - Change `MessageReactionList` over to the new component-based
      API [1], without going through @react-navigation/compat first.
      (It's small and self-contained.)
    
    - Undo f4b8253, i.e., stop threading through `navigation` prop and
      assigning to `static router` in MainScreenWithTabs. The "common
      mistakes" doc that prescribed those things has been removed in v5,
      and, empirically, everything works fine without it.
    
    - Use @react-navigation/compat to reduce the size of this commit
      (we'll incrementally move away from it in the upcoming commits).
    
      (Note that that package is completely untyped [2] -- this causes
      Flow to misleadingly mark some `$FlowFixMe`s unnecessary; still,
      we go along with Flow and remove them -- but once we've completely
      adopted the v5 APIs, our types should be stronger than before.)
    
      There are a few things we use the compat package for [3]:
    
      - Postpone changes to the definitions of our four navigators:
        `AppNavigator`, `MainTabs`, `StreamTabs`, and `SharingScreen`,
        with `createCompatNavigatorFactory`.
      - In `navActions`, postpone taking the new interfaces of various
        action creators (`StackActions.push`, etc.). This isn't possible
        with `.reset` actions, but we only use three of those, so we
        change them in this commit.
      - Keep using the `withNavigationFocus` HOC, so we can switch to
        the new custom Hook later.
    
    And a few minor naming / simple API tweaks:
    
      - In `SmartUrlInput`, use the 'focus' event type instead of the
        now-removed one, 'didFocus', and adapt to the new API for
        removing a listener.
      - What used to be called `routeName` on a route in the navigation
        state is now called `name`.
      - Even though we're using `createCompatNavigatorFactory`, we have
        to access `tintColor` instead of `color` on the object passed to
        the function we pass to `tabBarLabel` and `tabBarIcon` in a
        route config on a tab navigator.
      - Various types are renamed, like `NavigationParams` ->
        `ScreenParams`.
      - Apparently `upperCaseLabel` in a tab navigator's
        config...vanished in this new version. Add a TODO with findings
        from a GitHub discussion about this [4].
    
    [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20MessageReactionList/near/1061924
    [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5/near/1046571
    [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20compat/near/1058109
    [4] zulip#4393 (comment)
    chrisbobbe committed Jan 14, 2021
    1 Configuration menu
    Copy the full SHA
    1228450 View commit details
    Browse the repository at this point in the history
  5. deps: Clear out old libdefs for react-navigation.

    We're free to do this, since the upgrade to the differently named
    packages for React Navigation v5 has just been completed.
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    1db60d2 View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    0fed2a6 View commit details
    Browse the repository at this point in the history
  7. navActions: Stop using @react-navigation/compat.

    The compatability layer [1] has eased our migration to React
    Navigation v5. Now, remove our use of it here by adapting to the new
    APIs.
    
    The ugprade guide [2] describes the changes. There are a lot of
    changes to the interface, but it does say the following:
    
    """
    In addition, there have been some changes to the way the navigation
    actions work. These changes probably won't affect you if you didn't
    do any advanced tasks with these methods.
    """
    
    I don't know where they put the threshold for "advanced", but I
    think we should be fine.
    
    They do note (as they do on their "navigating without the navigation
    prop" doc [3]) that it's best to use the `navigation` object when
    one can:
    
    """
    It's highly recommended to use the methods on the navigation object
    instead of using action creators and `dispatch`. It should only be
    used for advanced use cases.
    """
    
    But I haven't noticed any major disruptions so far.
    
    [1] https://reactnavigation.org/docs/compatibility
    [2] https://reactnavigation.org/docs/upgrading-from-4.x/#action-creators
    [3] https://reactnavigation.org/docs/navigating-without-navigation-prop
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    140c28c View commit details
    Browse the repository at this point in the history
  8. ChatScreen: Stop using @react-navigation/compat.

    The `withNavigationFocus` HOC was dropped in v5. Also in v5, they
    introduced a `useIsFocused` Hook. But, as Greg points out [1],
    they've mitigated the disruption of this sudden change by supplying
    `withNavigationFocus` in the compat package, which we've been using.
    
    We made the switch to the new API easier, in a recent commit, by
    changing `ChatScreen` from a class component to a function component
    that uses React Hooks.
    
    [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20compat/near/1058117
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    6f74614 View commit details
    Browse the repository at this point in the history
  9. MainTabs: Stop using @react-navigation/compat.

    And switch over to the React Navigation v5 component-based API [1]
    for it.
    
    For all screens on `MainTabs`, that means the `navigation` prop gets
    a new shape, and the `route` prop is introduced. Also, where we
    invoke the screens to attach them to the navigator, the screens'
    `navigation` and `route` prop type annotations are checked for
    correctness.
    
    Since we newly define the `MainTabs` component ourselves, make it a
    Hooks-based component, as that seems to be the way of the future.
    
    [1] https://reactnavigation.org/docs/upgrading-from-4.x/#configuring-the-navigator
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    6bbea37 View commit details
    Browse the repository at this point in the history
  10. AppNavigator: Stop using @react-navigation/compat.

    Like the previous commit, but for `AppNavigator`.
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    ce7bf0c View commit details
    Browse the repository at this point in the history
  11. SharingScreen: Stop using @react-navigation/compat.

    Like the previous commits, but for `SharingScreen`.
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    89a9b6c View commit details
    Browse the repository at this point in the history
  12. StreamTabs: Stop using @react-navigation/compat.

    Like the previous commits, but for `StreamTabs`.
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    72c9e4b View commit details
    Browse the repository at this point in the history
  13. deps: Remove @react-navigation/compat.

    We're finished upgrading to React Navigation 5, so we can drop this.
    
    Fixes: zulip#4296
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    60c9c68 View commit details
    Browse the repository at this point in the history
  14. AppNavigator [nfc]: Stop getting some values from the parent component.

    Instead, have AppNavigator grab them itself, with react-redux.
    chrisbobbe committed Jan 14, 2021
    Configuration menu
    Copy the full SHA
    dd6ef39 View commit details
    Browse the repository at this point in the history