Skip to content

Commit

Permalink
navActions: Stop dispatching all actions via Redux.
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
chrisbobbe committed Oct 16, 2020
1 parent 8151a3a commit 63ab776
Show file tree
Hide file tree
Showing 25 changed files with 104 additions and 85 deletions.
14 changes: 7 additions & 7 deletions src/account-info/ProfileCard.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React, { PureComponent } from 'react';
import { ScrollView, View } from 'react-native';
import type { NavigationTabProp, NavigationStateRoute } from 'react-navigation-tabs';

import NavigationService from '../nav/NavigationService';
import type { Dispatch, User } from '../types';
import { createStyleSheet } from '../styles';
import { connect } from '../react-redux';
Expand All @@ -28,10 +29,9 @@ const styles = createStyleSheet({
},
});

class SetStatusButton extends PureComponent<{| +dispatch: Dispatch |}> {
class SetStatusButton extends PureComponent<{||}> {
onPress = () => {
const { dispatch } = this.props;
dispatch(navigateToUserStatus());
NavigationService.dispatch(navigateToUserStatus());
};

render() {
Expand All @@ -41,9 +41,9 @@ class SetStatusButton extends PureComponent<{| +dispatch: Dispatch |}> {
}
}

class SwitchAccountButton extends PureComponent<{| +dispatch: Dispatch |}> {
class SwitchAccountButton extends PureComponent<{||}> {
onPress = () => {
this.props.dispatch(navigateToAccountPicker());
NavigationService.dispatch(navigateToAccountPicker());
};

render() {
Expand Down Expand Up @@ -91,10 +91,10 @@ class ProfileCard extends PureComponent<Props> {
<AccountDetails user={selfUserDetail} />
<AwayStatusSwitch />
<View style={styles.buttonRow}>
<SetStatusButton dispatch={this.props.dispatch} />
<SetStatusButton />
</View>
<View style={styles.buttonRow}>
<SwitchAccountButton dispatch={this.props.dispatch} />
<SwitchAccountButton />
<LogoutButton dispatch={this.props.dispatch} />
</View>
</ScrollView>
Expand Down
7 changes: 4 additions & 3 deletions src/account/AccountPickScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import NavigationService from '../nav/NavigationService';
import type { Dispatch } from '../types';
import { connect } from '../react-redux';
import { hasAuth, getAccountStatuses } from '../selectors';
Expand Down Expand Up @@ -32,7 +33,7 @@ class AccountPickScreen extends PureComponent<Props> {
dispatch(accountSwitch(index));
});
} else {
dispatch(navigateToRealmScreen({ realm }));
NavigationService.dispatch(navigateToRealmScreen({ realm }));
}
};

Expand All @@ -53,7 +54,7 @@ class AccountPickScreen extends PureComponent<Props> {
canGoBack = this.props.hasAuth;

render() {
const { accounts, dispatch } = this.props;
const { accounts } = this.props;

return (
<Screen
Expand All @@ -74,7 +75,7 @@ class AccountPickScreen extends PureComponent<Props> {
<ZulipButton
text="Add new account"
onPress={() => {
dispatch(navigateToRealmScreen());
NavigationService.dispatch(navigateToRealmScreen());
}}
/>
</Centerer>
Expand Down
7 changes: 4 additions & 3 deletions src/account/accountActions.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* @flow strict-local */
import NavigationService from '../nav/NavigationService';
import type { Action, Dispatch, GetState } from '../types';
import {
ACCOUNT_SWITCH,
Expand All @@ -16,7 +17,7 @@ const accountSwitchPlain = (index: number): Action => ({
});

export const accountSwitch = (index: number) => (dispatch: Dispatch, getState: GetState) => {
dispatch(resetToLoading());
NavigationService.dispatch(resetToLoading());
dispatch(accountSwitchPlain(index));
};

Expand Down Expand Up @@ -47,7 +48,7 @@ export const loginSuccess = (realm: URL, email: string, apiKey: string) => (
dispatch: Dispatch,
getState: GetState,
) => {
dispatch(resetToLoading());
NavigationService.dispatch(resetToLoading());
dispatch(loginSuccessPlain(realm, email, apiKey));
};

Expand All @@ -56,6 +57,6 @@ const logoutPlain = (): Action => ({
});

export const logout = () => async (dispatch: Dispatch, getState: GetState) => {
dispatch(resetToAccountPicker());
NavigationService.dispatch(resetToAccountPicker());
dispatch(logoutPlain());
};
4 changes: 2 additions & 2 deletions src/boot/AppEventHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class AppEventHandlers extends PureComponent<Props> {
};

notificationListener = new NotificationListener(this.props.dispatch);
shareListener = new ShareReceivedListener(this.props.dispatch);
shareListener = new ShareReceivedListener();

handleMemoryWarning = () => {
// Release memory here
Expand All @@ -124,7 +124,7 @@ class AppEventHandlers extends PureComponent<Props> {
componentDidMount() {
const { dispatch } = this.props;
handleInitialNotification(dispatch);
handleInitialShare(dispatch);
handleInitialShare();

this.netInfoDisconnectCallback = NetInfo.addEventListener(this.handleConnectivityChange);
AppState.addEventListener('change', this.handleAppStateChange);
Expand Down
5 changes: 3 additions & 2 deletions src/compose/ComposeMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Platform, View } from 'react-native';
import type { DocumentPickerResponse } from 'react-native-document-picker';
import ImagePicker from 'react-native-image-picker';

import NavigationService from '../nav/NavigationService';
import type { Dispatch, Narrow } from '../types';
import { connect } from '../react-redux';
import { showErrorAlert } from '../utils/info';
Expand Down Expand Up @@ -154,7 +155,7 @@ class ComposeMenu extends PureComponent<Props> {
});

render() {
const { dispatch, expanded, insertVideoCallLink, onExpandContract } = this.props;
const { expanded, insertVideoCallLink, onExpandContract } = this.props;
const numIcons =
3 + (Platform.OS === 'android' ? 1 : 0) + (insertVideoCallLink !== null ? 1 : 0);

Expand All @@ -171,7 +172,7 @@ class ComposeMenu extends PureComponent<Props> {
style={this.styles.composeMenuButton}
size={24}
onPress={() => {
dispatch(navigateToCreateGroup());
NavigationService.dispatch(navigateToCreateGroup());
}}
/>
{Platform.OS === 'android' && (
Expand Down
19 changes: 6 additions & 13 deletions src/diagnostics/DiagnosticsScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';
import { nativeApplicationVersion } from 'expo-application';

import type { Dispatch } from '../types';
import NavigationService from '../nav/NavigationService';
import { createStyleSheet } from '../styles';
import { connect } from '../react-redux';
import { OptionButton, OptionDivider, Screen, RawLabel } from '../common';
import {
navigateToDebug,
Expand All @@ -28,45 +27,39 @@ type Props = $ReadOnly<{|
// don't invoke it anywhere else at all), we know it gets the
// `navigation` prop for free, with the stack-nav shape.
navigation: NavigationStackProp<NavigationStateRoute>,

dispatch: Dispatch,
|}>;

class DiagnosticsScreen extends PureComponent<Props> {
export default class DiagnosticsScreen extends PureComponent<Props> {
render() {
const { dispatch } = this.props;

return (
<Screen title="Diagnostics">
<RawLabel style={styles.versionLabel} text={`v${nativeApplicationVersion ?? '?.?.?'}`} />
<OptionDivider />
<OptionButton
label="Variables"
onPress={() => {
dispatch(navigateToVariables());
NavigationService.dispatch(navigateToVariables());
}}
/>
<OptionButton
label="Timing"
onPress={() => {
dispatch(navigateToTiming());
NavigationService.dispatch(navigateToTiming());
}}
/>
<OptionButton
label="Storage"
onPress={() => {
dispatch(navigateToStorage());
NavigationService.dispatch(navigateToStorage());
}}
/>
<OptionButton
label="Debug"
onPress={() => {
dispatch(navigateToDebug());
NavigationService.dispatch(navigateToDebug());
}}
/>
</Screen>
);
}
}

export default connect<{||}, _, _>()(DiagnosticsScreen);
3 changes: 2 additions & 1 deletion src/main/HomeTab.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React, { PureComponent } from 'react';
import { View } from 'react-native';
import type { NavigationTabProp, NavigationStateRoute } from 'react-navigation-tabs';

import NavigationService from '../nav/NavigationService';
import type { Dispatch } from '../types';
import { connect } from '../react-redux';
import { HOME_NARROW, MENTIONED_NARROW, STARRED_NARROW } from '../utils/narrow';
Expand Down Expand Up @@ -65,7 +66,7 @@ class HomeTab extends PureComponent<Props> {
<NavButton
name="search"
onPress={() => {
dispatch(navigateToSearch());
NavigationService.dispatch(navigateToSearch());
}}
/>
</View>
Expand Down
9 changes: 5 additions & 4 deletions src/message/__tests__/messageActions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
import configureStore from 'redux-mock-store';
import thunk from 'redux-thunk';

import { navigateToChat } from '../../nav/navActions';
import NavigationService from '../../nav/NavigationService';
import { doNarrow } from '../messagesActions';
import { streamNarrow } from '../../utils/narrow';
import * as eg from '../../__tests__/lib/exampleData';
Expand All @@ -15,6 +17,8 @@ const streamNarrowObj = streamNarrow('some stream');
describe('messageActions', () => {
describe('doNarrow', () => {
test('action to push to nav dispatched', () => {
NavigationService.dispatch = jest.fn();

const store = mockStore<GlobalState, Action>(
eg.reduxState({
accounts: [eg.selfAccount],
Expand All @@ -24,11 +28,8 @@ describe('messageActions', () => {
);

store.dispatch(doNarrow(streamNarrowObj));
const actions = store.getActions();

expect(actions).toHaveLength(1);
const [action0] = actions;
expect(action0.type).toBe('Navigation/PUSH');
expect(NavigationService.dispatch).toHaveBeenCalledWith(navigateToChat(streamNarrowObj));
});
});
});
3 changes: 2 additions & 1 deletion src/message/fetchActions.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* @flow strict-local */
import NavigationService from '../nav/NavigationService';
import type {
Narrow,
Dispatch,
Expand Down Expand Up @@ -173,7 +174,7 @@ export const initialFetchComplete = () => async (dispatch: Dispatch, getState: G
// conditional accordingly, if we found out we're not depending on
// the more general condition; see
// https://github.com/zulip/zulip-mobile/pull/4274#discussion_r505941875
dispatch(resetToMainTabs());
NavigationService.dispatch(resetToMainTabs());
}
dispatch(initialFetchCompletePlain());
};
Expand Down
6 changes: 4 additions & 2 deletions src/message/messageActionSheet.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
/* @flow strict-local */
import { Clipboard, Share, Alert } from 'react-native';

import NavigationService from '../nav/NavigationService';
import type {
Auth,
Dispatch,
Expand Down Expand Up @@ -185,13 +187,13 @@ shareMessage.title = 'Share';
shareMessage.errorMessage = 'Failed to share message';

const addReaction = ({ message, dispatch }) => {
dispatch(navigateToEmojiPicker(message.id));
NavigationService.dispatch(navigateToEmojiPicker(message.id));
};
addReaction.title = 'Add a reaction';
addReaction.errorMessage = 'Failed to add reaction';

const showReactions = ({ message, dispatch }) => {
dispatch(navigateToMessageReactionScreen(message.id));
NavigationService.dispatch(navigateToMessageReactionScreen(message.id));
};
showReactions.title = 'See who reacted';
showReactions.errorMessage = 'Failed to show reactions';
Expand Down
3 changes: 2 additions & 1 deletion src/message/messagesActions.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* @flow strict-local */
import NavigationService from '../nav/NavigationService';
import type { Narrow, Dispatch, GetState } from '../types';
import { getAuth, getUsersById } from '../selectors';
import { getMessageIdFromLink, getNarrowFromLink } from '../utils/internalLinks';
Expand All @@ -17,7 +18,7 @@ export const doNarrow = (narrow: Narrow, anchor: number = FIRST_UNREAD_ANCHOR) =
getState: GetState,
) => {
// TODO: Use `anchor` to open the message list to a particular message.
dispatch(navigateToChat(narrow));
NavigationService.dispatch(navigateToChat(narrow));
};

export const messageLinkPress = (href: string) => async (
Expand Down
26 changes: 20 additions & 6 deletions src/nav/InitialNavigationDispatcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import type { Node as React$Node } from 'react';
import { PureComponent } from 'react';

import NavigationService from './NavigationService';
import type { Dispatch, Account, User } from '../types';
import { resetToAccountPicker, resetToRealmScreen, resetToMainTabs } from '../actions';
import { connect } from '../react-redux';
Expand All @@ -24,33 +25,46 @@ type Props = $ReadOnly<{|
class InitialNavigationDispatcher extends PureComponent<Props> {
componentDidMount() {
if (this.props.isHydrated) {
// `NavigationService` will be ready by the time this is run: a
// `ref` is set in the `ref`fed component's
// `componentDidMount` [1], and a parent's `componentDidMount`
// is run after a child's `componentDidMount` [2].
//
// [1] https://reactjs.org/docs/refs-and-the-dom.html#adding-a-ref-to-a-dom-element
// [2] https://reactnavigation.org/docs/navigating-without-navigation-prop/#handling-initialization
this.doInitialNavigation();
}
}

componentDidUpdate(prevProps) {
if (!prevProps.isHydrated && this.props.isHydrated) {
// `NavigationService` will be ready here (as long as the
// `ref`fed component hasn't unmounted for some reason).
// `componentDidUpdate` won't run before `componentDidMount`,
// and we've established that it's ready by `componentDidMount`
// (see note there).
this.doInitialNavigation();
}
}

/**
* Data has been loaded, so open the app to the right screen.
*
* Not to be called before the REHYDRATE action, and not to be
* called more than once.
* Not to be called before the REHYDRATE action or before
* `NavigationService` is ready, and not to be called more than
* once.
*/
doInitialNavigation = () => {
const { hasAuth, accounts, users, dispatch } = this.props;
const { hasAuth, accounts, users } = this.props;

// If there are accounts but the active account is not logged in,
// show account screen.
if (!hasAuth) {
if (accounts.length > 1) {
dispatch(resetToAccountPicker());
NavigationService.dispatch(resetToAccountPicker());
return;
} else {
dispatch(resetToRealmScreen({ initial: true }));
NavigationService.dispatch(resetToRealmScreen({ initial: true }));
return;
}
}
Expand All @@ -67,7 +81,7 @@ class InitialNavigationDispatcher extends PureComponent<Props> {

// Great: we have an active, logged-in account, and server data for it.
// Show the main UI.
dispatch(resetToMainTabs());
NavigationService.dispatch(resetToMainTabs());
};

render() {
Expand Down
1 change: 1 addition & 0 deletions src/notification/notificationActions.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* @flow strict-local */
import { Platform } from 'react-native';

import type { Account, Dispatch, GetState, Identity, Action } from '../types';
import * as api from '../api';
import {
Expand Down
Loading

0 comments on commit 63ab776

Please sign in to comment.