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 react-navigation to v4 #4248

Closed
chrisbobbe opened this issue Sep 2, 2020 · 2 comments · Fixed by #4249
Closed

Upgrade react-navigation to v4 #4248

chrisbobbe opened this issue Sep 2, 2020 · 2 comments · Fixed by #4249
Assignees
Labels
a-navigation dependencies Pull requests that update a dependency file

Comments

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Sep 2, 2020

See the release announcement (2019-09-16) and the release tag on GitHub. To be done once we're on v3 (#3649).

There's a handy v3 -> v4 upgrade guide for this, here. (I haven't found one for the v2 -> v3 upgrade.)

It appears that FlowTyped does have a libdef for v4! So it's very likely that we won't have to use Flowgen for this upgrade.

@chrisbobbe chrisbobbe added blocked on other work To come back to after another related PR, or some other task. dependencies Pull requests that update a dependency file a-navigation labels Sep 2, 2020
@chrisbobbe chrisbobbe self-assigned this Sep 2, 2020
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 4, 2020
v4 includes lots of housekeeping changes that aim to make
maintenance easier [1].

In particular, we no longer grab stack, tab, or drawer navigation
logic from react-navigation itself; we must now depend directly on
separate libraries that provide these and import from them.

- We got a head start on this in the v3 upgrade (in a recent commit)
  by ensuring we had react-navigation-{tabs,stack,drawer} installed
  at v3-compatible versions.

- Now, we bump these to their latest versions (no peer-dep warnings
  when we do this!) and default to using carets in their version
  ranges since we see no indication that we can't.

- This means installing react-native-reanimated, as we knew we'd
  need to. (We mock it in our Jest setup, following some
  instructions [2].) For react-navigation-stack, we add peer
  dependencies react-native-safe-area-context and
  @react-native-community/masked-view.

The upgrade guide asks us to stop depending on
@react-navigation/core and @react-navigation/native and change any
imports of those to react-navigation imports instead. We don't have
any imports to change, and we can freely remove
@react-navigation/native. We can't yet remove @react-navigation/core
because it's still a peer dependency of
react-navigation-redux-helpers [3]. So we keep it, and align its
version range with the one in react-navigation's dependencies.

There's a long list of identifiers whose usage has changed.
Searching our code for each one, these are the interesting ones:

- `create*Navigator` being imported from the respective
  react-navigation-{tabs,stack,drawer} library; quite easy to handle

- `cardStyle` (in stack nav config) being moved to
  `navigationOptions` (or `defaultNavigationOptions`). We deleted
  our only use of `cardStyle` in the recent react-navigation v3
  upgrade because we only used it to make the background color white
  and, white became the default background color in that upgrade.

Finally, attempt to upgrade libdefs for the dependencies we touched,
if we import from them directly. We get new version-appropriate
libdefs for react-navigation, react-navigation-drawer, and
react-navigation-tabs. Unfortunately react-navigation-stack only has
a v1 libdef.

[1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/

[2] See https://reactnavigation.org/docs/testing/. We don't include
    the following line also recommended there:

    ```
    // Silence the warning: Animated: `useNativeDriver` is not
    // supported because the native animated module is missing
    jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper');
    ```

    We don't get that warning in the first place, and it's not clear
    what mock is being applied (apart from React Native's own
    default one). If we need to mock `NativeAnimatedHelper` in the
    future, we shouldn't point to the internal path in react-native;
    it should go in our existing react-native mock (alongside
    NativeModules, etc.).

[3] zulip#3804 (comment)

Fixes: zulip#4248
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 4, 2020
v4 includes lots of housekeeping changes that aim to make
maintenance easier [1].

In particular, we no longer grab stack, tab, or drawer navigation
logic from react-navigation itself; we must now depend directly on
separate libraries that provide these and import from them.

- We got a head start on this in the v3 upgrade (in a recent commit)
  by ensuring we had react-navigation-{tabs,stack,drawer} installed
  at v3-compatible versions.

- Now, we bump these to their latest versions (no peer-dep warnings
  when we do this!) and default to using carets in their version
  ranges since we see no indication that we can't.

- This means installing react-native-reanimated, as we knew we'd
  need to. (We mock it in our Jest setup, following some
  instructions [2].) For react-navigation-stack, we add peer
  dependencies react-native-safe-area-context and
  @react-native-community/masked-view.

The upgrade guide asks us to stop depending on
@react-navigation/core and @react-navigation/native and change any
imports of those to react-navigation imports instead. We don't have
any imports to change, and we can freely remove
@react-navigation/native. We can't yet remove @react-navigation/core
because it's still a peer dependency of
react-navigation-redux-helpers [3]. So we keep it, and align its
version range with the one in react-navigation's dependencies.

There's a long list of identifiers whose usage has changed.
Searching our code for each one, these are the interesting ones:

- `create*Navigator` being imported from the respective
  react-navigation-{tabs,stack,drawer} library; quite easy to handle

- `cardStyle` (in stack nav config) being moved to
  `navigationOptions` (or `defaultNavigationOptions`). We deleted
  our only use of `cardStyle` in the recent react-navigation v3
  upgrade because we only used it to make the background color white
  and, white became the default background color in that upgrade.

Finally, attempt to upgrade libdefs for the dependencies we touched,
if we import from them directly. We get new version-appropriate
libdefs for react-navigation, react-navigation-drawer, and
react-navigation-tabs. Unfortunately react-navigation-stack only has
a v1 libdef.

[1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/

[2] See https://reactnavigation.org/docs/testing/. We don't include
    the following line also recommended there:

    ```
    // Silence the warning: Animated: `useNativeDriver` is not
    // supported because the native animated module is missing
    jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper');
    ```

    We don't get that warning in the first place, and it's not clear
    what mock is being applied (apart from React Native's own
    default one). If we need to mock `NativeAnimatedHelper` in the
    future, we shouldn't point to the internal path in react-native;
    it should go in our existing react-native mock (alongside
    NativeModules, etc.).

[3] zulip#3804 (comment)

Fixes: zulip#4248
@chrisbobbe
Copy link
Contributor Author

I've sent #4249 for this.

Also, it appears that we need to be on react-navigation v4 by the time we upgrade to RN v0.62 (#3782); see this react-navigation issue. On RN v0.62 and react-navigation v2, I saw that error message; I no longer saw it on RN v0.62 and react-navigation v4. See also discussion.

@chrisbobbe
Copy link
Contributor Author

Ah, and to explain the "blocked" label on this issue: it's just blocked on react-navigation v3. But #4249 proposes an upgrade to both v3 and v4.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 11, 2020
v4 includes lots of housekeeping changes that aim to make
maintenance easier [1].

In particular, we no longer grab stack, tab, or drawer navigation
logic from react-navigation itself; we must now depend directly on
separate libraries that provide these and import from them.

- We got a head start on this in the v3 upgrade (in a recent commit)
  by ensuring we had react-navigation-{tabs,stack,drawer} installed
  at v3-compatible versions.

- Now, we bump these to their latest versions (no peer-dep warnings
  when we do this!) and default to using carets in their version
  ranges since we see no indication that we can't.

- This means installing react-native-reanimated, as we knew we'd
  need to. (We mock it in our Jest setup, following some
  instructions [2].) For react-navigation-stack, we add peer
  dependencies react-native-safe-area-context and
  @react-native-community/masked-view.

The upgrade guide asks us to stop depending on
@react-navigation/core and @react-navigation/native and change any
imports of those to react-navigation imports instead. We don't have
any imports to change, and we can freely remove
@react-navigation/native. We can't yet remove @react-navigation/core
because it's still a peer dependency of
react-navigation-redux-helpers [3]. So we keep it, and align its
version range with the one in react-navigation's dependencies.

There's a long list of identifiers whose usage has changed.
Searching our code for each one, these are the interesting ones:

- `create*Navigator` being imported from the respective
  react-navigation-{tabs,stack,drawer} library; quite easy to handle

- `cardStyle` (in stack nav config) being moved to
  `navigationOptions` (or `defaultNavigationOptions`). We deleted
  our only use of `cardStyle` in the recent react-navigation v3
  upgrade because we only used it to make the background color white
  and, white became the default background color in that upgrade.

Finally, attempt to upgrade libdefs for the dependencies we touched,
if we import from them directly. We get new version-appropriate
libdefs for react-navigation, react-navigation-drawer, and
react-navigation-tabs. Unfortunately react-navigation-stack only has
a v1 libdef.

[1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/

[2] See https://reactnavigation.org/docs/testing/. We don't include
    the following line also recommended there:

    ```
    // Silence the warning: Animated: `useNativeDriver` is not
    // supported because the native animated module is missing
    jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper');
    ```

    We don't get that warning in the first place, and it's not clear
    what mock is being applied (apart from React Native's own
    default one). If we need to mock `NativeAnimatedHelper` in the
    future, we shouldn't point to the internal path in react-native;
    it should go in our existing react-native mock (alongside
    NativeModules, etc.).

[3] zulip#3804 (comment)

Fixes: zulip#4248
@gnprice gnprice removed the blocked on other work To come back to after another related PR, or some other task. label Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-navigation dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants