Upgrade to React Navigation v2 #2702

borisyankov · 2018-06-20T16:55:28Z

The PR migrates to React Navigation v2 without changing or introducing new behavior.

gnprice

Thanks @borisyankov for doing this upgrade!

One of the really important steps for a major upgrade like this, which I think you've basically done, is reading through the release notes and thinking through what might break or we might need to change. A corollary to that is let's link to the release notes in the commit message, so the reader can easily follow along -- I think that means
https://reactnavigation.org/blog/2018/05/07/react-navigation-2.0.html

Reading that myself, there's one item that concerns me:

navigate(routeName) in StackNavigator is “less pushy”

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.)

Do you agree? If so, we should probably update accordingly.

Some other comments below.

I'm going to go ahead and merge the first commit, which splits up AppWithNavigation.

gnprice · 2018-07-24T22:14:13Z

src/nav/navActions.js


-export const navigateBack = () => (dispatch: Dispatch, getState: GetState): NavigateAction =>
-  // $FlowFixMe
-  dispatch(NavigationActions.pop({ n: getSameRoutesCount(getState()) }));


The commit message says pop was removed, which made me concerned that the preceding commit was putting us in a broken state.

I think it isn't, though -- here it still is in the docs:
https://reactnavigation.org/docs/en/navigation-prop.html#pop

... Oh, subtle. This is NavigationActions.pop, which has indeed gone missing from the docs.

Before checking the docs, I looked in the release notes.
https://reactnavigation.org/blog/2018/05/07/react-navigation-2.0.html
For a major-version upgrade like this, I wanted to be sure to read those anyway. I didn't see this change there, and it'd be pretty disappointing if they broke part of the API like that and didn't mention it... but aha, there's this bit:

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.

And indeed, here's that pop method:
https://reactnavigation.org/docs/en/stack-actions.html

So what I think I'd like to do is:

We shouldn't leave things in a broken state at any commit. The commit that bumps the react-navigation version should fix this line so it keeps working, in the minimal way that's easy to see is correct: switch to StackActions.

This switch to back looks cleaner than getSameRoutesCount, so I'll be glad to see a change that does that. It should be its own followup commit, and the commit message should explain why the new thing really does have the exact same behavior as the old -- that isn't obvious to me from a quick look at the doc for back. (Or it'd be fine for the behavior to be different; then the explanation just needs to say why the new behavior is good.)

gnprice · 2018-07-24T22:24:48Z

src/main/MainTabs.js

@@ -12,7 +12,7 @@ import SettingsCard from '../settings/SettingsCard';
 import { IconHome, IconStream, IconSettings } from '../common/Icons';
 import IconUnreadConversations from '../nav/IconUnreadConversations';

-export default TabNavigator(
+export default createMaterialTopTabNavigator(


Wow, that is confusing that this works! I appreciate that the apparent contradiction is pointed out in the commit message. :-)

It doesn't actually seem to be documented that this is something you can do -- tabBarPosition doesn't appear on this page:
https://reactnavigation.org/docs/en/material-top-tab-navigator.html
As long as it does work, it's OK, but it's something we'd want to call out and be watchful for on future upgrades.

It looks like there is a "bottom" counterpart to this component, though:
https://reactnavigation.org/docs/en/material-bottom-tab-navigator.html
Does that one work? If so, that seems cleaner.

... Aha -- I see #2831 switches to doing exactly this. And from looking at that commit, I see I'd missed this line in that doc:

npm install react-navigation-material-bottom-tabs

I guess I'll continue this thread on that PR.

gnprice · 2018-07-24T22:53:27Z

Oh, also: a commit message says this fixes #2682. It's not obvious to me why that would be -- can you say more?

zulipbot · 2018-08-01T10:00:57Z

Heads up @borisyankov, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

jainkuniya

I tried this PR but getting this issue.

borisyankov · 2018-09-11T12:54:41Z

I did go through the whole PR, updated dependencies, definitions etc. Added one commit that addressed type issues.

I think it is ready to merge.

I am not sure about the issue, but it works for me. Try restarting.

gnprice · 2018-09-16T05:03:21Z

@borisyankov I'm still curious about this question, which doesn't look like it's changed:

Oh, also: a commit message says this fixes #2682. It's not obvious to me why that would be -- can you say more?

The second commit removes a fixme, but yarn flow actually still gives me an error there. Dunno why that'd differ for you vs. me; but I don't actually see a NavigationActions.pop in the new flow-typed file.

... Oh. This is the same issue I pointed out in this comment above:
#2702 (comment)

Would you address that feedback and the question about #2682? Then I'll re-review.

borisyankov · 2018-09-17T15:10:09Z

About #2682 I think I did describe this few times somewhere but who knows if it got swallowed in GitHub's comments or is somewhere in the chats...

RN2 changes its behavior to treat routes with the same key (even if the params are different) as somehow the same navigation for processing purposes.

That would have, typically, required us to do some fixes to make sure the behavior remains the same. Luckily, the new behavior is what we want. On back from two chat screens we go back to main and going several times to topic list for example, would still get us back in one goBack().

gnprice · 2018-09-20T00:33:02Z

(BTW the feedback about pop is still outstanding.)

About #2682 I think I did describe this few times somewhere but who knows if it got swallowed in GitHub's comments or is somewhere in the chats...

OK; once you've successfully explained this to me, please put it in the commit message so that nobody has to wonder about it again in the future. :-)

RN2 changes its behavior to treat routes with the same key (even if the params are different) as somehow the same navigation for processing purposes.

That would have, typically, required us to do some fixes to make sure the behavior remains the same. Luckily, the new behavior is what we want. On back from two chat screens we go back to main and going several times to topic list for example, would still get us back in one goBack().

Hmm. I don't actually see how this is related to #2682. As Robert's description there says:

I rapidly tapped twice on the same topic.

and the screencast shows that we then did two narrow animations, one right after the other.

This is (a) the same params both times, (b) not about going back.

Does this fix #2682, then? If so, how?

gnprice · 2018-09-20T01:03:10Z

(Also, please make sure to fix the build; the PR is certainly not "ready to merge" while our tests fail on it. Looks easy to fix, but easy or hard it's the author's responsibility.)

borisyankov · 2019-05-29T12:51:16Z

@gnprice this is ready for another look.

One of the more questionable commit was completely replaced by this commit:
'navigation: Replace getSameRoutesCount with popToTop'

borisyankov · 2019-05-29T21:49:35Z

I've added one commit from #3067 without which the MainScreenWithTabs component now does not have the needed spacing below/above.

gnprice · 2019-05-29T23:52:11Z

Thanks @borisyankov!

I've tried to identify below the points I'd especially like to get your thinking on, and the ones where I think I understand already and can easily polish it up myself later before merging.

The priority items end up being, in order:

The "less pushy" question on the first commit
The highlights at bottom of tabs in MainTabs
Requests for docs/release-notes links on several commits

`ddd8a11` navigation: Upgrade to React Navigation v2

A question which I'd particularly like to get your thinking on:

Reading [the release notes] myself, there's one item that concerns me:

navigate(routeName) in StackNavigator is “less pushy”

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.)

Do you agree? If so, we should probably update accordingly.

Small mismerge, I think (easy for me to fix later):

+import { connect } from 'react-redux';
-import { connect } from '../react-redux';

Comment above (easy for me to fix later):

let's link to the release notes in the commit message, so the reader can easily follow along

`b600c2f` flow: Update 'flow-typed' for React Navigation 2

Easy for me to fix later: does the previous commit require this already?

`de29b5a` flow: Fix `NavigateAction` type

Ah cool, thanks for catching this.
Easy for me to check/fix later: is this fixing an error (exposed by the upgrade)? If so we'll want to move it forward, or otherwise avoid intermediate breakage.

`1c9608b` navigation: Update the creation of StackNavigator

Cool. Would you add a link to the appropriate bit of release notes and/or docs you relied on for this upgrade? I.e. saying the old thing was deprecated, and what to do instead.

`05505a5` navigation: Update StreamTabs creation

Same comment as in previous commit.
And thanks for making explicit in the commit message that this is meant to keep the same behavior! Quite helpful for understanding it.

`e18f702` navigation: Update `MainTabs` creation

Wow, this is indeed confusing. 🙂 (As your commit message mentions.)
Ah I see, there's still a tabBarPosition: 'bottom',, so that's what keeps it at the bottom despite the name.
Would you add a link to the bit of docs you relied on to settle on this?
Hmm, this goes and adds a highlight at the bottom of the selected tab. That makes sense as what something called createMaterialTopTabNavigator would do... but it feels odd to me when the content is actually above the tab. (Also, it doesn't match the commit message, which says this matches the current behavior.)
- Is there an easy way to adjust this to get the behavior we have in master, without that highlight?

`b8f3d0b` navigation: Replace `getSameRoutesCount` with `popToTop`

Cool. As discussed in Navigation - 'Back' button should always bring you to 'Home' #3503 , I'm not sure I like the behavior this implements (nor the related existing behavior) -- but I definitely appreciate making the logic clearer and easier to understand. Filing the issue was a good move, too, for providing a place for more detailed discussion that the commit message can link to.

`7d3dbae` Remove disused `getSameRoutesCount`

LGTM, thanks!

`0476c49` safeAreaView: Ensure `MainScreenWithTabs` is aligned

Ah, very interesting! Do we already need this change on master, before the react-navigation upgrade? If not, what change caused it to become needed? /cc @jainkuniya -- compare lightbox: Fix SafeAreaInsets padding is not applied. #3292 .

borisyankov · 2019-05-30T14:56:16Z

b600c2f flow: Update 'flow-typed' for React Navigation 2
Easy for me to fix later: does the previous commit require this already?

Not necessarily. To make it easier to spot the potential place to update this...

Just updating the flow-typed definition against our current master results in:

src/nav/AppWithNavigation.js:4:10
4│ Cannot import addNavigationHelpers because there is no addNavigationHelpers export in react-navigation.

src/nav/AppNavigator.js:39:3
Unused suppression comment.
39│ // $FlowFixMe react-navigation types :-/ -- see a36814e

src/nav/AppWithNavigation.js:28:11
Unused suppression comment.
28│ // $FlowFixMe flow-typed says react-navigation expects dispatch to return boolean

borisyankov · 2019-05-30T14:59:44Z

On the other hand, after all the changes, but not updating the Flow Typed types:

src/main/MainTabs.js:4:10
Cannot import createMaterialTopTabNavigator because there is no createMaterialTopTabNavigator export in react-navigation.
4│ import { createMaterialTopTabNavigator } from 'react-navigation';

src/main/StreamTabs.js:4:10
Cannot import createMaterialTopTabNavigator because there is no createMaterialTopTabNavigator export in react-navigation
4│ import { createMaterialTopTabNavigator } from 'react-navigation';

src/nav/AppNavigator.js:2:10
Cannot import createStackNavigator because there is no createStackNavigator export in react-navigation.
2│ import { createStackNavigator } from 'react-navigation';

src/nav/navActions.js:2:29
Cannot import StackActions because there is no StackActions export in react-navigation.
2│ import { NavigationActions, StackActions } from 'react-navigation';

borisyankov · 2019-05-30T15:00:25Z

To me, it seems logical to update this right after the change to RN 2 as it represents the truth even if it raises a few errors temporarily.

borisyankov · 2019-05-30T20:52:33Z

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.)

This has the benefit of enough time passing since I last described it in detail. Hope to be more clear now.
The documentation itself is a bit unclear.

In our case, this navigate vs push does not matter, but almost accidentally,.

How it worked in V1:

We are at 'Home'
navigate to 'Chat, Stream1'
navigate to 'Chat, Topic1'
go Back goes to 'Chat, Stream1'

We did manually do the logic Go back until there is no 'Chat' route

WIth V2 with and without our custom logic:

We are at 'Home'
navigate to 'Chat, Stream1'
navigate to 'Chat, Topic1'
go Back goes to 'Home' since 'Chat' is considered the same route. It would skip as many of the same routes as there are in the stack

borisyankov · 2019-05-30T21:02:08Z

I think navigation is still the better option (is more flexible) than push because:

In our case, it is what we currently want.
If we decide to go with the granular 'go back one chat, do not skip all chats' we can still implement this with navigate.
Even better, in case of 2) we can have the key be JSON.strigify(narrow) so if the user goes from Stream1 to Stream1 again via some link or other navigation, going back will skip the previous Stream1 navigation as the key will be the same.

To clarify by 'chat' I refer to the route 'chat':
https://github.com/zulip/zulip-mobile/blob/master/src/nav/AppNavigator.js#L45
https://github.com/zulip/zulip-mobile/blob/master/src/nav/navActions.js#L20

gnprice · 2019-05-31T00:18:11Z

Thanks, that's helpful!

Rereading the docs (those release notes, and the linked "RFC 4") and your explanation, here's the basic interaction that seems very confusing to me, with the new navigate:

User is at route A. Stack is X, A.
User follows a link to B. Stack is now X, A, B.
User follows a link to C. Stack is now X, A, B, C.
User follows a link to A.
- navigate notices there is an A already on the stack... and pops off everything in the way to get back to that one!
- Yep, it really does that -- the docs aren't quite unambiguous here, but see the implementation, as linked from the release notes.
- Stack is now just X, A.
User navigates back... and ??? gets X -- "what happened to C and B?! I was in the middle of reading those!"

And OTOH if

User is at C, but didn't happen to get there via A: stack is X, B, C.
User follows a link to A.
- Now navigate cheerfully pushes A onto the stack, giving X, B, C, A.
User navigates back, and gets X, B, C as expected.
- "That's odd -- it worked this time. How flaky."

I basically don't see how it can make any sense for the same action (navigate to A), starting from the same screen (C) and even same recent history of screens (B, C), to pop off B and C based on whether there's a match for A way back earlier in the stack.

I can see it making sense if we're at A, and navigating to A (with just different params). I.e., to replace the current screen on the top of the stack. I think that's possibly the case they had in mind; not quite sure. But that's not the logic they implemented.

gnprice · 2019-05-31T00:22:22Z

I think that RFC doc also pretty clearly recommends that we should be using push:

Take for example the flow of navigating from a user profile -> comment -> user profile. It is conceivable that you may visit user profile for "Jane", then in a comment visit the profile for "Bob", then in a comment visit "Jane" again. In this situation it is expected that the second time you visit "Jane" it pushes a new profile screen on top, rather than jumping back to the first one. For these situations, users should use push.

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.

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.

Fixes zulip#3503 The popToTop action takes you back to the first screen in the stack: https://reactnavigation.org/docs/en/stack-actions.html#poptotop 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.

Makes sure our main tabs are aligned properly with the edges of the screen on iOS. More about how safe areas work on iOS: https://developer.apple.com/documentation/uikit/uiview/positioning_content_relative_to_the_safe_area About the React Native-specific component: https://facebook.github.io/react-native/docs/safeareaview

gnprice · 2019-08-12T23:38:12Z

Closing, as we've now made this upgrade -- see #3573 . (One more commit from this PR made it in: 2a066b2)

borisyankov force-pushed the rn2 branch 4 times, most recently from 0782f18 to 91a6a5d Compare June 21, 2018 09:59

jainkuniya mentioned this pull request Jun 23, 2018

Back button from settings screen should return to HomeTab #2716

Open

borisyankov force-pushed the rn2 branch 4 times, most recently from 9e97460 to d46568b Compare July 7, 2018 01:11

borisyankov force-pushed the rn2 branch 4 times, most recently from 3ec9582 to 9f8cf04 Compare July 24, 2018 07:13

gnprice reviewed Jul 24, 2018

View reviewed changes

gnprice mentioned this pull request Jul 25, 2018

Use MaterialTopTabNavigator for main navigation #2831

Closed

borisyankov force-pushed the rn2 branch 2 times, most recently from 8216931 to 1434b86 Compare July 30, 2018 18:31

borisyankov force-pushed the rn2 branch 2 times, most recently from c39e2cf to bda8009 Compare August 24, 2018 22:26

jainkuniya reviewed Sep 11, 2018

View reviewed changes

borisyankov force-pushed the rn2 branch from bda8009 to 63d7166 Compare September 11, 2018 11:21

borisyankov force-pushed the rn2 branch 2 times, most recently from f6f6375 to fe011be Compare September 20, 2018 01:55

borisyankov mentioned this pull request May 29, 2019

Use 'SafeAreaView' through the app #3067

Closed

borisyankov force-pushed the rn2 branch 2 times, most recently from 9977ade to c484f0e Compare May 30, 2019 14:47

This was referenced Jul 24, 2019

Tab navigation isn't animated #3566

Open

Migrate to community ViewPagerAndroid #3569

Open

Upgrade react-navigation to v2 #3573

Closed

borisyankov added 9 commits August 9, 2019 16:52

flow: Update 'flow-typed' for React Navigation 2

3a21a05

Update definitions by running `flow-typed update`. This lets Flow know about the new constructor functions.

flow: Fix NavigateAction type

f23f764

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`.

navigation: Update the creation of StackNavigator

392e104

The `StackNavigator` constructor function has been deprcated. Using the `createStackNavigator` that expects the same inputs.

navigation: Update StreamTabs creation

cf178bc

`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.

Remove disused getSameRoutesCount

784da47

Now that we don't need `getSameRoutesCount` we can remove it together with the accompanying unit-tests.

gnprice force-pushed the rn2 branch from c484f0e to c073c05 Compare August 9, 2019 23:56

gnprice closed this Aug 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to React Navigation v2 #2702

Upgrade to React Navigation v2 #2702

borisyankov commented Jun 20, 2018 •

edited

Loading

gnprice left a comment

gnprice Jul 24, 2018

gnprice Jul 24, 2018

gnprice Jul 25, 2018

gnprice commented Jul 24, 2018 •

edited

Loading

zulipbot commented Aug 1, 2018

jainkuniya left a comment

borisyankov commented Sep 11, 2018

gnprice commented Sep 16, 2018

borisyankov commented Sep 17, 2018

gnprice commented Sep 20, 2018

gnprice commented Sep 20, 2018

borisyankov commented May 29, 2019

borisyankov commented May 29, 2019

gnprice commented May 29, 2019

borisyankov commented May 30, 2019

borisyankov commented May 30, 2019

borisyankov commented May 30, 2019 •

edited

Loading

borisyankov commented May 30, 2019

borisyankov commented May 30, 2019

gnprice commented May 31, 2019

gnprice commented May 31, 2019

gnprice commented Aug 12, 2019

Upgrade to React Navigation v2 #2702

Upgrade to React Navigation v2 #2702

Conversation

borisyankov commented Jun 20, 2018 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

gnprice Jul 24, 2018

Choose a reason for hiding this comment

gnprice Jul 24, 2018

Choose a reason for hiding this comment

gnprice Jul 25, 2018

Choose a reason for hiding this comment

gnprice commented Jul 24, 2018 • edited Loading

zulipbot commented Aug 1, 2018

jainkuniya left a comment

Choose a reason for hiding this comment

borisyankov commented Sep 11, 2018

gnprice commented Sep 16, 2018

borisyankov commented Sep 17, 2018

gnprice commented Sep 20, 2018

gnprice commented Sep 20, 2018

borisyankov commented May 29, 2019

borisyankov commented May 29, 2019

gnprice commented May 29, 2019

ddd8a11 navigation: Upgrade to React Navigation v2

b600c2f flow: Update 'flow-typed' for React Navigation 2

de29b5a flow: Fix NavigateAction type

1c9608b navigation: Update the creation of StackNavigator

05505a5 navigation: Update StreamTabs creation

e18f702 navigation: Update MainTabs creation

b8f3d0b navigation: Replace getSameRoutesCount with popToTop

7d3dbae Remove disused getSameRoutesCount

0476c49 safeAreaView: Ensure MainScreenWithTabs is aligned

borisyankov commented May 30, 2019

borisyankov commented May 30, 2019

borisyankov commented May 30, 2019 • edited Loading

borisyankov commented May 30, 2019

borisyankov commented May 30, 2019

gnprice commented May 31, 2019

gnprice commented May 31, 2019

gnprice commented Aug 12, 2019

borisyankov commented Jun 20, 2018 •

edited

Loading

gnprice commented Jul 24, 2018 •

edited

Loading

`ddd8a11` navigation: Upgrade to React Navigation v2

`b600c2f` flow: Update 'flow-typed' for React Navigation 2

`de29b5a` flow: Fix `NavigateAction` type

`1c9608b` navigation: Update the creation of StackNavigator

`05505a5` navigation: Update StreamTabs creation

`e18f702` navigation: Update `MainTabs` creation

`b8f3d0b` navigation: Replace `getSameRoutesCount` with `popToTop`

`7d3dbae` Remove disused `getSameRoutesCount`

`0476c49` safeAreaView: Ensure `MainScreenWithTabs` is aligned

borisyankov commented May 30, 2019 •

edited

Loading