WIP: A very WIP upgrade to React Navigation v3 #3502
Conversation
Upgrade to React Navigation 2 and do the minimal changes that make our code compatible. Navigating behavior has changed, and pushing a route with the same key does not result in duplicated entry in the history. The code to wire RN navigator to Redux have changed. The newer approach is simpler, just using `reduxifyNavigator` helper does most of the work. Since `pop` was removed from `NavigationActions` use the `pop` action from `StackActions` instead.
Update definitions by running `flow-typed update`. This lets Flow know about the new constructor functions.
The type `NavigationNavigateAction` we use is too specific and not the correct one. We want the `NavigationAction` type instead. Also, to make it simpler and consistent, we rename our own type `NavigateAction` to `NavigationAction`.
The `StackNavigator` constructor function has been deprcated. Using the `createStackNavigator` that expects the same inputs.
`TabNavigator` constructor is deprecated in favor of the function 'createMaterialTopTabNavigator'. Our config no longer needs some parameters passed, so this is simplified while keeping the very same behavior.
Replace `TabNavigator` constructor with `createMaterialTopTabNavigator` creator function which is the one that matched the old behavior. Note: While `createBottomTabNavigator` sounds like what we might need, it is a simpler navigator compared to what we were using and actually `createMaterialTopTabNavigator` is the correct one.
Instead of using the written by us `getSameRoutesCount` and the standard `StackActions.pop` use the new 'StackActions.popToTop'.
Now that we don't need `getSameRoutesCount` we can remove it together with the accompanying unit-tests.
Reaict Native Gesture Handler provides native-driven gesture management APIs for building best possible touch-based experiences in React Native. While we might need to use it directly sometime in the future, the need to add it now is that React Navigation 3 depends on it. This PR adds 'react-native-gesture-handler' as dependency and configures it for use in both iOS and Android. It follows the documentation on how to do it here: https://reactnavigation.org/docs/en/getting-started.html https://kmagiera.github.io/react-native-gesture-handler/docs/getting-started.html First, `react-native link` was used, but then manually a lot of code changes that were cosmetic and line reordering were undone. After that, some manual changes were implemented (according to the documentation)
https://reactnavigation.org/blog/2018/11/17/react-navigation-3.0.html `flow-typed` updated manually
|
Ah, good thought, thanks! I think the thing to prioritize here will be braindumping what you've learned about this upgrade. Doing that in code is great where it's easy; in general,
That can be in commit messages, or in this PR thread. |
|
The commit |
I think the upgrade is straightforward, they lib just tends to churn a bit on the API and some navigation objects have had the format of the config objects changed a bit. Nothing major.
The 'very WIP' part comes from the fact that I worked on this PR a long time ago, but now that I have reviewed the code, it needs a bit of cleanup and would likely be a fast and painless merge. |
Good to know, thanks! Is it mergeable independently of the v2 upgrade #2702, or does it depend on that? (It comes after it in this branch.) And thanks for the other notes too; that's helpful. |
|
Yeah, it does not depend on anything. Which is a dependency of RN3. It can be used separately from it (but unlikely to) |
With the upcoming upgrade to RN v0.59 (from v0.57), without this change I found that I could navigate to any tab as usual... but just once, and then the bottom tabs had no effect, until the app was restarted. Similarly, if I used my one main-tab navigation to go to "streams", then the top tabs there were good for exactly one navigation: I could navigate from "Subscribed" to "All streams", but not back. So, that's pretty broken. The issue appears to be react-navigation/react-navigation#5713. This change is a workaround suggested in a comment there (thanks!) The look isn't awesome, but at least it keeps things functioning. The issue thread sounds like the issue is fixed in a later release of react-navigation. We're on a quite old release, 1.6.1. We have some partial work toward an upgrade (#2702 and #3502); so this is one more reason to finish that.
|
Hi, first comment on Zulip code here! :) One thing I ran into a few months ago with React Navigation 3 (I'm 95% sure it was introduced in version 2 actually), was that if you try to It seems a PR was made to React Navigation to add a helpful message or error, but rejected with very little ceremony: react-navigation/react-navigation#2140. Maybe someone has run into this (or you've seen that they've fixed the issue in the last few months)? |
|
Thanks @chrisbobbe ! Looks like the underlying issue for that is react-navigation/react-navigation#1053 , which in turn they copied to https://react-navigation.canny.io/feature-requests/p/throw-warning-or-error-when-navigate-to-an-invalid-route and closed. That is an annoying pitfall, and it sure seems like they should be able to mitigate it with a warning. For our project, we largely deal with this issue by centralizing the |
|
Closing as superseded by #4249, which has already been merged. |
Based on and a follow up to #2702