Skip to content

Commit

Permalink
nav: Stop using react-navigation-redux-helpers.
Browse files Browse the repository at this point in the history
We've long used Redux to manage our navigation state, with a helper
package called react-navigation-redux-helpers. React Navigation has
recommended against this for quite some time.

In this commit:

- Stop using r-n-r-h's `createReduxContainer`, and use React
  Navigation's own `createAppContainer` instead.

  - Rename `AppWithNavigation` (arguably better named
    `ReduxContainer` before this commit) to `AppContainer`

  - Rename `NavigationService.reduxContainerRef` to
    `NavigationService.appContainerRef` and change some of
    `NavigationService`'s implementation details to handle the new
    shape.

- Remove use of `createReactNavigationReduxMiddleware`.

- Remove `state.nav`.

  - Remove `navReducer`.

  - Adjust types as necessary.

  - Remove 'nav' from `discardKeys` in src/boot/store.js

Fixes: zulip#3804
  • Loading branch information
chrisbobbe committed Oct 23, 2020
1 parent 914ae48 commit dab9f5e
Show file tree
Hide file tree
Showing 8 changed files with 28 additions and 99 deletions.
9 changes: 7 additions & 2 deletions src/ZulipMobile.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import AppEventHandlers from './boot/AppEventHandlers';
import AppDataFetcher from './boot/AppDataFetcher';
import BackNavigationHandler from './nav/BackNavigationHandler';
import InitialNavigationDispatcher from './nav/InitialNavigationDispatcher';
import AppWithNavigation from './nav/AppWithNavigation';
import AppContainer from './nav/AppContainer';
import NavigationService from './nav/NavigationService';

import './i18n/locale';
Expand All @@ -31,7 +31,12 @@ export default (): React$Node => (
<ThemeProvider>
<InitialNavigationDispatcher>
<BackNavigationHandler>
<AppWithNavigation ref={NavigationService.reduxContainerRef} />
<AppContainer
// `static navigationOptions` and `static router` not
// being handled properly
// $FlowFixMe
ref={NavigationService.appContainerRef}
/>
</BackNavigationHandler>
</InitialNavigationDispatcher>
</ThemeProvider>
Expand Down
2 changes: 0 additions & 2 deletions src/boot/reducers.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import flags from '../chat/flagsReducer';
import narrows from '../chat/narrowsReducer';
import messages from '../message/messagesReducer';
import mute from '../mute/muteReducer';
import nav from '../nav/navReducer';
import outbox from '../outbox/outboxReducer';
import presence from '../presence/presenceReducer';
import realm from '../realm/realmReducer';
Expand Down Expand Up @@ -46,7 +45,6 @@ const reducers = {
messages,
narrows,
mute,
nav,
outbox,
presence,
realm,
Expand Down
7 changes: 1 addition & 6 deletions src/boot/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import type { Store } from 'redux';
import thunkMiddleware from 'redux-thunk';
import { createLogger } from 'redux-logger';
import createActionBuffer from 'redux-action-buffer';
import { createReactNavigationReduxMiddleware } from 'react-navigation-redux-helpers';
import Immutable from 'immutable';
import * as Serialize from 'remotedev-serialize';
import { persistStore, autoRehydrate } from '../third/redux-persist';
Expand All @@ -29,7 +28,7 @@ import createMigration from '../redux-persist-migrate/index';
// prettier-ignore
export const discardKeys: Array<$Keys<GlobalState>> = [
'alertWords', 'caughtUp', 'fetching',
'nav', 'presence', 'session', 'topics', 'typing', 'userStatus',
'presence', 'session', 'topics', 'typing', 'userStatus',
];

/**
Expand Down Expand Up @@ -209,10 +208,6 @@ const migrations: { [string]: (GlobalState) => GlobalState } = {
*/
function listMiddleware() {
const result = [
// Allow us to cause navigation by dispatching Redux actions.
// See docs: https://github.com/react-navigation/redux-helpers
createReactNavigationReduxMiddleware((state: GlobalState) => state.nav, 'root'),

// Delay ("buffer") actions until a REHYDRATE action comes through.
// After dispatching the latter, this will go back and dispatch
// all the buffered actions. See docs:
Expand Down
7 changes: 7 additions & 0 deletions src/nav/AppContainer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/* @flow strict-local */
import { createAppContainer } from 'react-navigation';
import type { NavigationState } from 'react-navigation';

import AppNavigator from './AppNavigator';

export default createAppContainer<NavigationState, { ... }>(AppNavigator);
16 changes: 0 additions & 16 deletions src/nav/AppWithNavigation.js

This file was deleted.

41 changes: 13 additions & 28 deletions src/nav/NavigationService.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,53 +2,38 @@
import React from 'react';
import type {
NavigationAction,
NavigationNavigatorProps,
NavigationState,
NavigationDispatch,
SupportedThemes,
NavigationContainer,
NavigationContainerProps,
} from 'react-navigation';

type ReduxContainerProps = {
...$Diff<NavigationNavigatorProps<{ ... }, NavigationState>, { navigation: mixed }>,
state: NavigationState,
dispatch: NavigationDispatch,
theme: SupportedThemes | 'no-preference',
};

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

// TODO: This will eventually be a ref to the component instance
// created by React Navigation's `createAppContainer`, not
// react-navigation-redux-helpers's `createReduxContainer`.
const reduxContainerRef = React.createRef<ReduxContainer>();
const appContainerRef = React.createRef<
NavigationContainer<NavigationState, { ... }, NavigationContainerProps<{ ... }, NavigationState>>,
>();

const getState = (): NavigationState => {
if (reduxContainerRef.current === null) {
throw new Error('Tried to use NavigationService before reduxContainerRef was set.');
if (appContainerRef.current === null) {
throw new Error('Tried to use NavigationService before appContainerRef was set.');
}
return (
reduxContainerRef.current
// $FlowFixMe - how to tell Flow about this method?
.getCurrentNavigation().state
// $FlowFixMe - how to tell Flow about `.state`?
appContainerRef.current.state.nav
);
};

const dispatch = (navigationAction: NavigationAction): boolean => {
if (reduxContainerRef.current === null) {
throw new Error('Tried to use NavigationService before reduxContainerRef was set.');
if (appContainerRef.current === null) {
throw new Error('Tried to use NavigationService before appContainerRef was set.');
}
return (
reduxContainerRef.current
appContainerRef.current
// $FlowFixMe - how to tell Flow about this method?
.getCurrentNavigation()
.dispatch(navigationAction)
);
};

export default {
getState,
dispatch,
reduxContainerRef,
appContainerRef,
};
37 changes: 0 additions & 37 deletions src/nav/navReducer.js

This file was deleted.

8 changes: 0 additions & 8 deletions src/reduxTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,6 @@ export type NavigationRouteState = {
},
};

export type NavigationState = {|
index: number,
isTransitioning: boolean,
key: string,
routes: NavigationRouteState[],
|};

export type OutboxState = Outbox[];

/**
Expand Down Expand Up @@ -358,7 +351,6 @@ export type GlobalState = {|
migrations: MigrationsState,
mute: MuteState,
narrows: NarrowsState,
nav: NavigationState,
outbox: OutboxState,
presence: PresenceState,
realm: RealmState,
Expand Down

0 comments on commit dab9f5e

Please sign in to comment.