Decouple Navigation from Redux (Part 1) #4274

chrisbobbe · 2020-10-06T02:20:54Z

Contributing to #3804.

Following #4273, which was merged today.

Some interesting things happening in this PR:

Add a NavigationService so we can talk directly to React Navigation instead of going through Redux
Use the NavigationService instead of Redux to read the navigation state, in all but a very few cases that will disappear naturally with further work
Transplant logic in our helpers in navReducer that handle some of our custom actions, i.e., actions that don't look like Navigation/POP and friends.

While my observations suggest that actions dispatched using NavigationService still affect state.nav in the ways we'd want them to [1], it seems safest to minimize the places where we read from the state.nav source before we ~~stop updating it~~ potentially alter how we update it to an extent that isn't known.

I take it that NavigationService is a safe source for the navigation state because

we've been quite comfortable using the navigation prop to read the navigation state, even with our react-navigation-redux-helpers setup; we use props.navigation.state.params quite often
the docs suggest that NavigationService can reasonably be considered a way to grab things from the navigation prop
empirically, I haven't yet seen NavigationService give the wrong nav state

The next main piece of work after this PR will be to use NavigationService to dispatch navigation actions, then it should be a pretty easy path to removing our react-navigation-redux-helpers setup completely.

[1] Also, an example app with react-navigation v2 and react-navigation-redux-helpers seems just as happy to dispatch navigation actions with the navigation prop (which is interesting for us and our NavigationService) as they are to do so with Redux's dispatch.

gnprice

Thanks @chrisbobbe! I like what this branch is doing. Some comments below.

I haven't read the REHYDRATE commit in detail, because I believe you're going to rework that one a fair amount.

src/nav/NavigationService.js

gnprice · 2020-10-06T23:22:15Z

src/nav/NavigationService.js

+    reduxContainerRef.current
+      // $FlowFixMe - how to tell Flow about this method?
+      .getCurrentNavigation().state


Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/nav/NavigationService.js:34:8 Cannot call reduxContainerRef.current.getCurrentNavigation because property getCurrentNavigation is missing in React.Component [1].

// Should mimic return type of react-navigation-redux-helpers's // `createReduxContainer` … type ReduxContainer = React$Component<ReduxContainerProps>; const reduxContainerRef = React.createRef<ReduxContainer>();

Seems like what's missing is that Flow only knows this is a React component with certain props, and doesn't know about any component-specific methods. I.e., ReduxContainer would need to be more specific.

Looks like the existing libdef for react-navigation-redux-helpers is a stub, so that's no help. Given that we're aiming to rip that out soon, it may not be worth the effort to prepare a real one.

Looks like the existing libdef for react-navigation-redux-helpers is a stub, so that's no help. Given that we're aiming to rip that out soon, it may not be worth the effort to prepare a real one.

Makes sense. I expect we'll have a very similar problem with the ref we'll set to React Navigation's own component (when r-n-r-h's createReduxContainer out of the picture), but we can cross that bridge when we come to it.

Looks like the existing libdef for react-navigation-redux-helpers is a stub, so that's no help.

True, but there are some Flow types in node_modules/react-navigation-redux-helpers/src/create-redux-container.js. But I didn't glean anything that I found particularly helpful for this.

src/events/doEventActionSideEffects.js

src/nav/navActions.js

gnprice · 2020-10-07T01:21:42Z

src/start/LoadingScreen.js

+   * Not to be called before the REHYDRATE action, and not to be
+   * called more than once.
+   */
+  doInitialNavigation = () => {


(We discussed this commit today by video chat. One thing we figured out is that the use of a ref on the navigator means that it belongs in an ancestor component of the navigator, not a descendant like LoadingScreen.)

src/start/AuthScreen.js

src/account-info/ProfileCard.js

gnprice · 2020-10-07T01:43:02Z

src/nav/navReducer.js

@@ -29,7 +30,7 @@ export const initialState = getStateForRoute('loading');
 export default (state: NavigationState = initialState, action: Action): NavigationState => {
  switch (action.type) {
    case INITIAL_FETCH_COMPLETE:
-      return state.routes[0].routeName === 'main' ? state : getStateForRoute('main');
+      return getCurrentRouteName() === 'main' ? state : getStateForRoute('main');


This is actually changing the behavior here, I think -- the [0] was intentional. Probably deserved a comment, or something in the code making it explicit.

The idea of this code:

return state.routes[0].routeName === 'main' ? state : getStateForRoute('main');

was: if we're anywhere in the normal UI of the app when an initial fetch completes, then remain there. Only if we're elsewhere -- which I think is just a way of saying, only if we're on the loading screen -- do we navigate, and in that case go to the main screen.

So in particular if you reopen the app after it was in the background for a while, and you were on some conversation, then it shows a loading animation there for a while, and when the fetch completes it shows you the conversation with its data. Similarly if you launch the app by opening a notification.

I could go further and just make it check whether it's on the loading screen?

That'd probably be good; we'd just want to check to be confident that there isn't another case where we'd want this behavior (and where the current code is getting it.) Probably best as a separate commit, either within this branch or afterward.

src/nav/AppWithNavigation.js

Also add a comment, with Greg's description [1] of what's going on here. [1] zulip#4274 (comment)

chrisbobbe · 2020-10-13T02:40:59Z

Thanks for the review! Just pushed my latest revision.

Also add a comment, with Greg's description [1] of what's going on here. [1] zulip#4274 (comment)

Instead, dispatch them via our recently added `NavigationService`. Add a few notes in `InitialNavigationDispatcher` about why we expect `NavigationService` to be ready in time. We chose to do the initial navigation here, rather than in a descendant of the `NavigationService`'s `ref`fed component for those reasons [1]. [1] zulip#4274 (comment)

src/start/DevAuthScreen.js

gnprice · 2020-10-16T00:41:23Z

src/nav/navReducer.js

@@ -29,7 +30,7 @@ export const initialState = getStateForRoute('loading');
 export default (state: NavigationState = initialState, action: Action): NavigationState => {
  switch (action.type) {
    case INITIAL_FETCH_COMPLETE:
-      return state.routes[0].routeName === 'main' ? state : getStateForRoute('main');
+      return getCurrentRouteName() === 'main' ? state : getStateForRoute('main');


That'd probably be good; we'd just want to check to be confident that there isn't another case where we'd want this behavior (and where the current code is getting it.) Probably best as a separate commit, either within this branch or afterward.

gnprice

Thanks @chrisbobbe! These revisions look good, with some small comments below (and above.)

The big gap is that I haven't yet read the new commit for the REHYDRATE case. I'll return to that; but it's dinnertime, and I wanted to send the rest of this off so you'll have it.

src/message/fetchActions.js

src/nav/navReducer.js

Also add a comment, with Greg's description [1] of what's going on here. We have this handy function for grabbing what we want from the navigation state, so we might as well use it. When we use this function, the navigation state is gotten from `NavigationService`, not from Redux, which moves us closer to zulip#3804. To be more rigorous, though, we wouldn't want this to stay this way in `navReduce` forever: reducers are supposed to be pure functions of state and actions. But the incorrectness isn't obviously harmful here, and we're about to transplant this code somewhere else, and it's helpful to separate a small tweak like this from the commit where we move the code. [1] zulip#4274 (comment)

chrisbobbe · 2020-10-16T01:47:04Z

Thanks for the review! I've just addressed those comments.

Instead, dispatch them via our recently added `NavigationService`. Add a few notes in `InitialNavigationDispatcher` about why we expect `NavigationService` to be ready in time. We chose to do the initial navigation here, rather than in a descendant of the `NavigationService`'s `ref`fed component for those reasons [1]. [1] zulip#4274 (comment)

gnprice

Thanks again! All those commits LGTM now, and I've just read the remaining one. A few small comments below, mostly about comments; then please merge at will.

src/nav/navReducer.js

src/nav/InitialNavigationDispatcher.js

gnprice · 2020-10-16T21:41:21Z

src/nav/InitialNavigationDispatcher.js

+    // If there are accounts but the active account is not logged in,
+    // show account screen.
+    if (!hasAuth) {


The "If there are accounts" part no longer applies -- in the old code, it's there to make a contrast with the preceding condition:

// If there's no data to rehydrate, or no account data, show login screen. if (!action.payload || !action.payload.accounts) { return getStateForRoute('realm'); }

but that's dissolved by the fact that with this design we're looking at a nice well-formed state post-REHYDRATE, rather than the REHYDRATE payload which is a bunch of pieces in roughly the shape of our state.

You're saying the comment should be adjusted, right:

- // If there are accounts but the active account is not logged in, - // show account screen. + // If the active account is not logged in, show account screen.

I've stared at this a bit, and I think something is still off:

// If the active account is not logged in, show account screen. if (!hasAuth) { if (accounts.length > 1) { dispatch(resetToAccountPicker()); return; } else { dispatch(resetToRealmScreen({ initial: true })); return; } }

I think maybe

- if (accounts.length > 1) { + if (accounts.length >= 1) {

?

Still doesn't quite line up with the comment; maybe something like this, to replace the code in the multi-line quote above:

// If we don't know about any accounts, go to the realm-input screen. if (accounts.length === 0) { dispatch(resetToRealmScreen({ initial: true })); return; } // If the active account is not logged in, show account screen. if (!hasAuth) { dispatch(resetToAccountPicker()); return; }

(Discussed in chat, and the result is in your new tip commit.)

Also add a comment, with Greg's description [1] of what's going on here. We have this handy function for grabbing what we want from the navigation state, so we might as well use it. When we use this function, the navigation state is gotten from `NavigationService`, not from Redux, which moves us closer to zulip#3804. To be more rigorous, though, we wouldn't want this to stay this way in `navReduce` forever: reducers are supposed to be pure functions of state and actions. But the incorrectness isn't obviously harmful here, and we're about to transplant this code somewhere else, and it's helpful to separate a small tweak like this from the commit where we move the code. [1] zulip#4274 (comment)

Staring at the code before this change, it's not clear why we'd want to go to the realm-input screen in the case where we know about exactly one account. That's the place we go when we don't have any idea what account the user might want to log in as. I suppose one might say it's weird to land on the account-picker screen when we only know about one account -- but we know that the account isn't logged in, so we can't go ahead and show its data. It also helps the user get where they're going faster by giving them something to tap on, which will take them to the auth screen to log in. See discussion [1]. [1] zulip#4274 (comment)

chrisbobbe · 2020-10-20T02:43:08Z

Thanks again! I just responded to your comments, with one small question, and I also added one additional commit at the tip.

Instead, dispatch them via our recently added `NavigationService`. Add a few notes in `InitialNavigationDispatcher` about why we expect `NavigationService` to be ready in time. We chose to do the initial navigation here, rather than in a descendant of the `NavigationService`'s `ref`fed component for those reasons [1]. [1] zulip#4274 (comment)

Also add a comment, with Greg's description [1] of what's going on here. We have this handy function for grabbing what we want from the navigation state, so we might as well use it. When we use this function, the navigation state is gotten from `NavigationService`, not from Redux, which moves us closer to zulip#3804. To be more rigorous, though, we wouldn't want this to stay this way in `navReduce` forever: reducers are supposed to be pure functions of state and actions. But the incorrectness isn't obviously harmful here, and we're about to transplant this code somewhere else, and it's helpful to separate a small tweak like this from the commit where we move the code. [1] zulip#4274 (comment)

Following a doc from React Navigation [1] that outlines a way to navigate without access to the `navigation` prop. The integration of React Navigation with our Redux setup has meant that we don't regularly use the `navigation` prop. We've gotten used to two essential functions being provided by Redux through `react-redux`, much like how we use Redux for other bits of state management: - getting information based on the navigation state - carrying out navigation actions It would be quite disruptive to suddenly start using the `navigation` prop everywhere (especially in hard-to-reach places like the message action sheet). This guide suggests a place we could go that turns out to be a useful stepping-stone. We take the main idea of the guide -- set up a React ref to the "app container" (the result of invoking `createAppContainer`) and call methods on that -- and make a few departures, in both the interface and the implementation. Notable differences in the interface: - Instead of a `navigate` function that takes `routeName` and `params`, we expose `dispatch`. This is `dispatch` from React Navigation's lexicon, not Redux's, but it will take the exact same set of actions as we create in navActions.js -- those action creators come to us, after all, straight from `StackActions` in react-navigation. - We also expose a way to get the current navigation state; this isn't suggested in the doc, but it's perfectly possible. We call it `getState` because that's a perfectly clear, logical name; it also happens to be familiar from the Redux world. And in the implementation: - The doc suggests that the ref gets set on the root navigator component; for us, that would be our invocation of `createStackNavigator` in AppNavigator.js. When one actually follows their instructions, though, the ref gets set to the "app container": the result of calling react-navigation's `createAppContainer` (passing the root nav to that). This is seen in logging, and also in the fact that the special, reserved `ref` attribute is set directly on the app container component. It's not too hard to see why the doc makes this mistake -- it went through 2.x to 3.x to 4.x without dramatic changes, and `createAppContainer` didn't exist in 2.x. So they probably just forgot to update it properly. - The ref isn't set on React Navigation's "app container" component, but rather on the "Redux container" component that react-navigation-redux-helpers has asked us to replace it with, with `createReduxContainer`. (see 99be838 for more on that replacement). We'll move to `createAppContainer` later in the process, and when we do so, we expect to make slight tweaks to the implementation to adapt. But we've confirmed that it'll still be quite possible to expose `getState` and `dispatch`. [2] [1] https://reactnavigation.org/docs/4.x/navigating-without-navigation-prop [2] See more discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/decouple.20nav.20from.20redux.20%28.23M3804%29/near/1025422.

Working toward zulip#3804. We're about to reimplement most of the functions in navSelectors.js so that they grab navigation state from the recently added `NavigationService` instead of from `state.nav` in Redux. For now, it's convenient to keep sourcing data from `state.nav` in just a few places. In this case, the one call site of `getCanGoBack` (the `connect` wrapper for `BackNavigationHandler`) is placed such that it's not convenient for it to get navigation state from `NavigationService`. In particular, `NavigationService` relies on a ref being set -- to a component that's deeper down in the component hierarchy. If we ask `NavigationService` for the navigation state, that component won't have been constructed, the ref will come up empty, and we'll have an error. `BackNavigationHandler` can disappear as zulip#3804 is completed; you can see how and why this is done in an upcoming commit.

To ensure that the `state` prop passed to the component returned by `createReduxContainer` [1] still comes from Redux, even as we reimplement `getNav` to get the navigation state from the newly added `NavigationService`. [1] https://github.com/react-navigation/redux-helpers#createreduxcontainer-required

As in the previous few commits, this is one of a small handful of places where we need to write code to explicitly get the navigation state from Redux, before we reimplement `getNav` to get the state from the recently added `NavigationService`. Like in those previous few commits, this is a place where we'll no longer need the navigation state, when zulip#3804 is completed.

We haven't been using any of these functions in react-redux `connect` calls; if we had, we might have been relying on them for the important task of triggering updates (re-`render`s) in some components, when the nav state changes. We've recently set up the `NavigationService`, which should provide the nav state just as readily as Redux could. Using React Navigation's types, instead of our own, reveals an unchecked assumption we've been making, at the single call site of `getChatScreenParams`. The value at `.narrow` in the returned object, if that value is not undefined, won't obviously be a `Narrow`. So, add a FlowFixMe there.

Now, conveniently, all of the functions in this file create plain-object actions that can be passed directly to the `dispatch` function on the recently added `NavigationService` [1]. [1] This is documented in the case of using `dispatch` on the `navigation` prop (https://reactnavigation.org/docs/4.x/navigation-prop/#dispatch---send-an-action-to-the-router) but it also applies to our `NavigationService.dispatch`.

This is a string that's passed to `SmartUrlInput` as its `defaultValue`. The use of react-redux's `connect` appears to be done just for convenience; the function we pass as `mapStateToProps` doesn't use the Redux state.

We're about to have it take an optional `initial` argument. When a function has multiple optional positional parameters, there is unnecessary confusion about what a caller should do when it doesn't want to pass one of them.

Our current nav-state logic has a case where `RealmScreen` has `params.initial` in its navigation state. So, allow this to be passed in the action creator. The case is a weird one: it came about in 7a04a56, when `getStateForRoute` started including `initial: true` when the passed `route` was 'realm'. This has coincided with the fact that `getStateForRoute`'s only call site with 'realm' for `route` has been one where it's appropriate to have `initial: true`. Anyway, that call site is in the `rehydrate` handler in `navReducer`, which we're going to dissolve quite soon, using the `navigateToRealmScreen` action creator instead.

Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). The various handlers will be transplanted into instructions to dispatch navigation actions at the appropriate times. These new functions will create those actions. It makes sense for them to be "reset" actions because the effect of those handlers in `navReducer` was to construct a state based on no previous state; this was ensured by passing no second argument (`lastState`) to `AppNavigator.router.getStateForAction` in our `getStateForRoute`. It also makes sense intuitively; if we're doing something like logging out, we don't want to allow back navigation. [1] https://reactnavigation.org/docs/4.x/stack-actions/#reset

I'm not sure if this actually changes anything; 'loading' has effectively (and intentionally) been the initial route for quite some time. See `navReducer`: ``` export const initialState = getStateForRoute('loading'); ```

Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). So, convert the `navReducer`'s `rehydrate` handler into instructions to dispatch the right navigation actions at the right time.

In the previous commit, we removed our last call site of `getStateWithRoute` where 'realm' was passed for `route`. Therefore, this conditional can disappear. See more about this hack at zulip#4273 (comment).

Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). So, convert the `navReducer`'s `LOGOUT` case into instructions to navigate to the account-picker screen. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition.

Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). So, convert the `navReducer`'s `ACCOUNT_SWITCH` case into instructions to navigate to the loading screen. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition.

Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). So, convert the `navReducer`'s `LOGIN_SUCCESS` case into instructions to navigate to the loading screen. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition.

Also add a comment, with Greg's description [1] of what's going on here. We have this handy function for grabbing what we want from the navigation state, so we might as well use it. When we use this function, the navigation state is gotten from `NavigationService`, not from Redux, which moves us closer to zulip#3804. To be more rigorous, though, we wouldn't want this to stay this way in `navReduce` forever: reducers are supposed to be pure functions of state and actions. But the incorrectness isn't obviously harmful here, and we're about to transplant this code somewhere else, and it's helpful to separate a small tweak like this from the commit where we move the code. [1] zulip#4274 (comment)

Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). So, convert the `navReducer`'s `INITIAL_FETCH_COMPLETE` case into instructions to navigate to the main-tabs screen, if not already there. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition.

I think we generally expect the 'main' route to be index zero of the routes, when it's among the routes. But it's not obvious that this will always hold, and it's quite easy to check all the routes.

When the active account isn't logged in and there's just one account in total, we can reasonably pre-fill that account's realm in the realm-input screen. It seems like this has been the expected behavior [1], but I just tested it now, and it hasn't been happening. Also, for readability, separate the `accounts.length === 1` check into its own `else if`. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/decouple.20nav.20from.20redux.20%28.23M3804%29/near/1042602

chrisbobbe · 2020-10-22T03:50:31Z

Thanks for the review and merge!

I seemed to have dropped a ball here, and I didn't mark this PR as blocked by #4222 before this PR was merged.

In discussion, I'd said:

OK, I've just realized that #M4222 will make this a lot easier. With navigation state stored in Redux, it doesn't matter that the key hack in TranslationProvider unmounts and remounts the root navigator. But when we solely rely on that navigator for the navigation state, that key hack becomes a problem—changing the language puts you on LoadingScreen, 'loading' being the initialRouteName of AppNavigator, and you stay there indefinitely.

But having said that, I didn't then go and mark this PR as blocked, either on #4222 or a more complicated solution. Oops.

The effect is that, on master, changing the language is broken. The symptom I quoted above...isn't the one I'm seeing now, but it's still unstable and something we should fix.

Now, if I select a different language, it resets to the main-tabs screen. A lot (!) better than getting stuck indefinitely on the loading screen, but I'm not currently sure why it's not doing that anymore. I've pushed a revision for #4222 just now after resolving some conflicts.

Instead, dispatch them via our recently added `NavigationService`. Add a few notes in `InitialNavigationDispatcher` about why we expect `NavigationService` to be ready in time. We chose to do the initial navigation here, rather than in a descendant of the `NavigationService`'s `ref`fed component for those reasons [1]. [1] zulip#4274 (comment)

chrisbobbe added a-redux a-navigation labels Oct 6, 2020

chrisbobbe requested a review from gnprice October 6, 2020 02:20

gnprice reviewed Oct 7, 2020

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Oct 13, 2020

navReducer: Use getNavigationRoutes selector, for simplicity.

21c64e7

Also add a comment, with Greg's description [1] of what's going on here. [1] zulip#4274 (comment)

chrisbobbe force-pushed the pr-decouple-nav-1 branch from a163761 to 61607be Compare October 13, 2020 02:40

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Oct 13, 2020

navReducer: Use getNavigationRoutes selector, for simplicity.

2f958ac

Also add a comment, with Greg's description [1] of what's going on here. [1] zulip#4274 (comment)

chrisbobbe force-pushed the pr-decouple-nav-1 branch from 61607be to 3ebed4c Compare October 13, 2020 18:41

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Oct 13, 2020

navReducer: Use getNavigationRoutes selector, for simplicity.

cf1b65a

Also add a comment, with Greg's description [1] of what's going on here. [1] zulip#4274 (comment)

chrisbobbe force-pushed the pr-decouple-nav-1 branch from 3ebed4c to 79aff10 Compare October 13, 2020 19:29

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Oct 13, 2020

navReducer: Use getNavigationRoutes selector, for simplicity.

0c31e33

Also add a comment, with Greg's description [1] of what's going on here. [1] zulip#4274 (comment)

chrisbobbe force-pushed the pr-decouple-nav-1 branch from 79aff10 to d9c8920 Compare October 13, 2020 19:32

chrisbobbe mentioned this pull request Oct 13, 2020

Decouple Navigation from Redux (Part 2) #4278

Merged

gnprice reviewed Oct 16, 2020

View reviewed changes

src/message/fetchActions.js Outdated Show resolved Hide resolved

src/nav/navReducer.js Outdated Show resolved Hide resolved

src/nav/navReducer.js Show resolved Hide resolved

chrisbobbe force-pushed the pr-decouple-nav-1 branch from d9c8920 to aa0002c Compare October 16, 2020 01:46

gnprice reviewed Oct 16, 2020

View reviewed changes

chrisbobbe force-pushed the pr-decouple-nav-1 branch from aa0002c to 94803bc Compare October 20, 2020 02:42

chrisbobbe force-pushed the pr-decouple-nav-1 branch from 94803bc to 900f619 Compare October 20, 2020 20:42

chrisbobbe added 20 commits October 21, 2020 18:10

RealmScreen [nfc]: Give initialRealm a better name.

f5ca194

This is a string that's passed to `SmartUrlInput` as its `defaultValue`. The use of react-redux's `connect` appears to be done just for convenience; the function we pass as `mapStateToProps` doesn't use the Redux state.

AppNavigator: Set initialRouteName to 'loading'.

5e409a5

I'm not sure if this actually changes anything; 'loading' has effectively (and intentionally) been the initial route for quite some time. See `navReducer`: ``` export const initialState = getStateForRoute('loading'); ```

navReducer [nfc]: Remove an initial: true params hack.

a34d3ba

In the previous commit, we removed our last call site of `getStateWithRoute` where 'realm' was passed for `route`. Therefore, this conditional can disappear. See more about this hack at zulip#4273 (comment).

fetchActions: Compare all routes to 'main', not just the first.

2821bea

I think we generally expect the 'main' route to be index zero of the routes, when it's among the routes. But it's not obvious that this will always hold, and it's quite easy to check all the routes.

gnprice force-pushed the pr-decouple-nav-1 branch from 016ba6a to 2e049c4 Compare October 22, 2020 01:10

gnprice merged commit 2e049c4 into zulip:master Oct 22, 2020

chrisbobbe mentioned this pull request Oct 22, 2020

Upgrade react-intl to a version that uses the new Context API, then to the latest. #4222

Merged

chrisbobbe mentioned this pull request Jan 11, 2021

React Navigation v5 upgrade (part 2) #4393

Merged

chrisbobbe deleted the pr-decouple-nav-1 branch April 15, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple Navigation from Redux (Part 1) #4274

Decouple Navigation from Redux (Part 1) #4274

chrisbobbe commented Oct 6, 2020

gnprice left a comment

gnprice Oct 6, 2020

chrisbobbe Oct 8, 2020

chrisbobbe Oct 9, 2020

gnprice Oct 7, 2020

gnprice Oct 7, 2020

chrisbobbe Oct 12, 2020

gnprice Oct 16, 2020

chrisbobbe commented Oct 13, 2020

gnprice Oct 16, 2020

gnprice left a comment

chrisbobbe commented Oct 16, 2020

gnprice left a comment

gnprice Oct 16, 2020

chrisbobbe Oct 20, 2020

chrisbobbe Oct 20, 2020

chrisbobbe Oct 20, 2020

gnprice Oct 22, 2020

chrisbobbe commented Oct 20, 2020

chrisbobbe commented Oct 22, 2020

Decouple Navigation from Redux (Part 1) #4274

Decouple Navigation from Redux (Part 1) #4274

Conversation

chrisbobbe commented Oct 6, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Oct 13, 2020

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

chrisbobbe commented Oct 16, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Oct 20, 2020

chrisbobbe commented Oct 22, 2020