Skip to content

Commit

Permalink
nav: Tell all screens on AppNavigator that they get the navigation
Browse files Browse the repository at this point in the history
…prop.

This has been done on an as-needed basis in the past, but in a few
different ways, and it would be good to be consistent.

With zulip#3804 approaching, it makes sense to proactively furnish all
these components with the knowledge that they have the `navigation`
prop if they ever need it.

In doing so, we're making an unchecked assumption: that the
components will in fact be passed the `navigation` prop, in the
shape we say it will take. It's unchecked because of the
`$FlowFixMe` from aaaf847 on the route config passed to
`createStackNavigator` [0].

-----

There is a doc for doing this in TypeScript [1], but it's clearly
wrong about how to handle it in Flow (and I believe it's also wrong
about TypeScript in ways that would matter to us if we were using
TypeScript).

In particular, `NavigationStackScreenProps` is marketed (under "Type
checking all props for a screen") as something you can spread in
your `Props` type to get `theme` and `screenProps` along with
`navigation`. Sounds great.

But it doesn't exist in the Flow libdef, and I haven't found a clear
alternative. In zulip#4127, a demo PR branch that informed 17f73d8 --
in particular,

f482827 [FOR DISCUSSION] One way to get `sharedData` into
  ShareToStream, ShareToPm

-- I seemed [2] to have favored something called
`NavigationNavigatorProps` for this purpose. But it has at least two
points against it:

1. I don't see documentation that we're supposed to use it, or how.
2. It's not tailored (or conveniently tailorable) to screens that
   get put on a particular type of navigator (stack, drawer, tabs,
   etc.)

So, be conservative and just type the `navigation` prop, and in a
way that says we know it comes from a stack navigator.

The main example in the TypeScript doc is the following:

```
type Props = {
  navigation: NavigationStackProp<{ userId: string }>;
};
```

We find `NavigationStackProp` is a good thing to use [3], and it
exists in the Flow libdef, but `{ userId: string }` is bad for a
couple of reasons:

- It's off by a level of nesting; surely they mean to describe
  `navigation.state.params`. To do that, the type needs to look
  something like `{ params: { userId: string } }`. This is also the
  case in the TypeScript.

- It leaves out all the other things on `navigation.state` including
  `key` and `routeName`, which might be nice to have someday.
  Experimentally, these are conveniently filled in by saying
  `NavigationStackProp<NavigationStateRoute>` [4].

So, account for those deficiencies straightforwardly.

[0] Initial thoughts on this were written in a36814e, but see
    zulip#4114 (comment)
    for more recent concerns about typing our component classes with
    a static `navigationOptions` property.

    I'm inclined to not revisit these suppressions until we're on
    React Navigation v5, which has quite a different,
    component-based API, and a particular guide for type-checking
    the route config and the screen components in a unified way;
    that's at https://reactnavigation.org/docs/typescript/.

[1] https://reactnavigation.org/docs/4.x/typescript/

[2] Possibly following a train of thought from
    https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878262.

[3] In one or two places, we've been using `NavigationScreenProp` --
    the doc for upgrading from v3 (which we did in f3b6c1f) says
    to replace that with `NavigationStackProp` for stack navigators.
    This didn't catch my eye at the time because it's under the
    "TypeScript" heading and we don't use TypeScript. That doc is at
    https://reactnavigation.org/docs/4.x/upgrading-from-3.x/#typescript.

[4] Like at
    react-navigation/react-navigation#3643 (comment),
    but with `NavigationStackProp` instead of
    `NavigationScreenProp`.
  • Loading branch information
chrisbobbe committed Oct 2, 2020
1 parent 5a4a941 commit 5fba56b
Show file tree
Hide file tree
Showing 31 changed files with 243 additions and 34 deletions.
8 changes: 6 additions & 2 deletions src/account-info/AccountDetailsScreen.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { NavigationScreenProp } from 'react-navigation';
import type { Dispatch, UserOrBot } from '../types';
import { createStyleSheet } from '../styles';
import { connect } from '../react-redux';
Expand Down Expand Up @@ -30,7 +30,11 @@ type SelectorProps = $ReadOnly<{|
|}>;

type Props = $ReadOnly<{|
navigation: NavigationScreenProp<{ params: {| userId: number |} }>,
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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, params: {| userId: number |} |}>,

dispatch: Dispatch,
...SelectorProps,
Expand Down
7 changes: 7 additions & 0 deletions src/account/AccountPickScreen.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* @flow strict-local */

import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { Dispatch } from '../types';
import { connect } from '../react-redux';
Expand All @@ -11,6 +12,12 @@ import AccountList from './AccountList';
import { navigateToRealmScreen, accountSwitch, removeAccount } from '../actions';

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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>,

accounts: AccountStatus[],
dispatch: Dispatch,
hasAuth: boolean,
Expand Down
8 changes: 6 additions & 2 deletions src/chat/ChatScreen.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { View } from 'react-native';
import type { NavigationScreenProp } from 'react-navigation';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';
import { withNavigationFocus } from 'react-navigation';
import { ActionSheetProvider } from '@expo/react-native-action-sheet';
import { compose } from 'redux';
Expand Down Expand Up @@ -34,7 +34,11 @@ type SelectorProps = {|
|};

type Props = $ReadOnly<{|
navigation: NavigationScreenProp<{ params: {| narrow: Narrow |} }>,
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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, params: {| narrow: Narrow |} |}>,
dispatch: Dispatch,

// From React Navigation's `withNavigationFocus` HOC. Type copied
Expand Down
12 changes: 10 additions & 2 deletions src/chat/GroupDetailsScreen.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { FlatList } from 'react-native';
import type { NavigationScreenProp } from 'react-navigation';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { Dispatch, UserOrBot } from '../types';
import { connect } from '../react-redux';
Expand All @@ -10,7 +10,15 @@ import UserItem from '../users/UserItem';
import { navigateToAccountDetails } from '../actions';

type Props = $ReadOnly<{|
navigation: NavigationScreenProp<{ params: {| recipients: UserOrBot[] |} }>,
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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,
params: {| recipients: UserOrBot[] |},
|}>,

dispatch: Dispatch,
|}>;

Expand Down
8 changes: 7 additions & 1 deletion src/diagnostics/DiagnosticsScreen.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* @flow strict-local */

import React, { PureComponent } from 'react';

import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';
import { nativeApplicationVersion } from 'expo-application';

import type { Dispatch } from '../types';
Expand All @@ -23,6 +23,12 @@ const styles = createStyleSheet({
});

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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,
|}>;

Expand Down
7 changes: 7 additions & 0 deletions src/diagnostics/StorageScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import React, { PureComponent } from 'react';
import { FlatList } from 'react-native';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { GlobalState, Dispatch } from '../types';
import { connect } from '../react-redux';
Expand All @@ -17,6 +18,12 @@ const calculateKeyStorageSizes = obj =>
.sort((a, b) => b.size - a.size);

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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,
state: GlobalState,
|}>;
Expand Down
11 changes: 10 additions & 1 deletion src/diagnostics/TimingScreen.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { FlatList } from 'react-native';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import { Screen } from '../common';
import TimeItem from './TimeItem';
import timing from '../utils/timing';

export default class TimingScreen extends PureComponent<{||}> {
type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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>,
|}>;

export default class TimingScreen extends PureComponent<Props> {
render() {
return (
<Screen title="Timing" scrollEnabled={false}>
Expand Down
11 changes: 10 additions & 1 deletion src/diagnostics/VariablesScreen.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { FlatList } from 'react-native';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import config from '../config';
import { Screen } from '../common';
import InfoItem from './InfoItem';

export default class VariablesScreen extends PureComponent<{||}> {
type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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>,
|}>;

export default class VariablesScreen extends PureComponent<Props> {
render() {
const variables = {
enableReduxLogging: config.enableReduxLogging,
Expand Down
9 changes: 7 additions & 2 deletions src/emoji/EmojiPickerScreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import React, { PureComponent } from 'react';
import { FlatList } from 'react-native';
import type { NavigationScreenProp } from 'react-navigation';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import * as api from '../api';
import { unicodeCodeByName } from './codePointMap';
Expand All @@ -16,10 +16,15 @@ import { navigateBack } from '../nav/navActions';
import zulipExtraEmojiMap from './zulipExtraEmojiMap';

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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, params: {| messageId: number |} |}>,

activeImageEmojiByName: RealmEmojiById,
auth: Auth,
dispatch: Dispatch,
navigation: NavigationScreenProp<{ params: {| messageId: number |} }>,
|}>;

type State = {|
Expand Down
11 changes: 9 additions & 2 deletions src/lightbox/LightboxScreen.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { View } from 'react-native';
import type { NavigationScreenProp } from 'react-navigation';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';
import { ActionSheetProvider } from '@expo/react-native-action-sheet';

import { ZulipStatusBar } from '../common';
Expand All @@ -19,7 +19,14 @@ const styles = createStyleSheet({
});

type Props = $ReadOnly<{|
navigation: NavigationScreenProp<{ params: {| src: string, message: Message |} }>,
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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,
params: {| src: string, message: Message |},
|}>,
|}>;

export default class LightboxScreen extends PureComponent<Props> {
Expand Down
8 changes: 6 additions & 2 deletions src/main/MainScreenWithTabs.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import { View } from 'react-native';
import type { NavigationScreenProp } from 'react-navigation';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { ThemeData } from '../styles';
import styles, { ThemeContext } from '../styles';
import { OfflineNotice, ZulipStatusBar } from '../common';
import MainTabs from './MainTabs';

type Props = $ReadOnly<{|
navigation: NavigationScreenProp<>,
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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>,
|}>;

export default class MainScreenWithTabs extends PureComponent<Props> {
Expand Down
11 changes: 9 additions & 2 deletions src/reactions/MessageReactionList.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import React, { PureComponent } from 'react';
import { View } from 'react-native';
import { createAppContainer } from 'react-navigation';
import type { NavigationScreenProp } from 'react-navigation';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';
import { createMaterialTopTabNavigator } from 'react-navigation-tabs';

import * as logging from '../utils/logging';
Expand Down Expand Up @@ -111,7 +111,14 @@ type SelectorProps = $ReadOnly<{|
|}>;

type Props = $ReadOnly<{|
navigation: NavigationScreenProp<{ params: {| reactionName?: string, messageId: number |} }>,
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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,
params: {| reactionName?: string, messageId: number |},
|}>,

dispatch: Dispatch,
...SelectorProps,
Expand Down
7 changes: 7 additions & 0 deletions src/search/SearchMessagesScreen.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* @flow strict-local */
import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { Auth, Dispatch, Message } from '../types';
import { Screen } from '../common';
Expand All @@ -12,6 +13,12 @@ import { getAuth } from '../account/accountsSelectors';
import { fetchMessages } from '../message/fetchActions';

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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>,

auth: Auth,
dispatch: Dispatch,
// Warning: do not add new props without considering their effect on the
Expand Down
7 changes: 7 additions & 0 deletions src/settings/DebugScreen.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* @flow strict-local */

import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { Debug, Dispatch } from '../types';
import { connect } from '../react-redux';
Expand All @@ -9,6 +10,12 @@ import { OptionRow, Screen } from '../common';
import { debugFlagToggle } from '../actions';

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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>,

debug: Debug,
dispatch: Dispatch,
|}>;
Expand Down
7 changes: 7 additions & 0 deletions src/settings/LanguageScreen.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* @flow strict-local */

import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { Dispatch } from '../types';
import { connect } from '../react-redux';
Expand All @@ -10,6 +11,12 @@ import { getSettings } from '../selectors';
import { settingsChange } from '../actions';

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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,
locale: string,
|}>;
Expand Down
7 changes: 7 additions & 0 deletions src/settings/LegalScreen.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* @flow strict-local */

import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { Dispatch } from '../types';
import { connect } from '../react-redux';
Expand All @@ -9,6 +10,12 @@ import openLink from '../utils/openLink';
import { getCurrentRealm } from '../selectors';

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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,
realm: URL,
|}>;
Expand Down
7 changes: 7 additions & 0 deletions src/settings/NotificationsScreen.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* @flow strict-local */

import React, { PureComponent } from 'react';
import type { NavigationStackProp, NavigationStateRoute } from 'react-navigation-stack';

import type { Auth, Dispatch } from '../types';
import { connect } from '../react-redux';
Expand All @@ -10,6 +11,12 @@ import * as api from '../api';
import { settingsChange } from '../actions';

type Props = $ReadOnly<{|
// Since we've put this screen in a stack-nav route config, and we
// don't invoke it without type-checking anywhere else (in fact, we
// 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>,

auth: Auth,
dispatch: Dispatch,
offlineNotification: boolean,
Expand Down
Loading

0 comments on commit 5fba56b

Please sign in to comment.