From c07555969fe94c81ae44b2565f690a7ca4a66246 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 22 Jan 2021 15:21:33 -0800 Subject: [PATCH 1/3] type tests: Add a README explaining how these work. We don't have many of these at present, and should probably have more. We'll add some more in the next commit. Before that, add a bit of developer documentation about what these are and how they work. --- src/__flow-tests__/README.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 src/__flow-tests__/README.md diff --git a/src/__flow-tests__/README.md b/src/__flow-tests__/README.md new file mode 100644 index 00000000000..e1d3944e119 --- /dev/null +++ b/src/__flow-tests__/README.md @@ -0,0 +1,24 @@ +# Type-tests + +This directory contains tests of our types. + +This code never gets run, but it does get type-checked. We use it to +test that some of our more complex types behave in the way we expect. + +Mostly this means checking that using the types in certain ways that +should give errors, does give errors. (Checking that things that +should be OK, are OK, is generally well covered by where we simply use +the types, in our main app code.) + +The key tool for confirming that something does give an error is to +write `$FlowExpectedError`. As far as Flow is concerned (see [Flow +docs][https://flow.org/en/docs/errors/]), this has exactly the same +effect as `$FlowFixMe`: it suppresses the error, and conversely it +means that if we ever stop getting an error at that spot, we start +getting a warning about the unused suppression. + +The difference between `$FlowFixMe` and `$FlowExpectedError` is purely +for the human reader: the latter communicates that the error is +intended, not something we want to get rid of, and in particular that +if it ever goes away then that's itself a problem which we should +investigate. From d2ab4e32e48cef0aeed72f29750c2a108f60cf35 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 13 Jan 2021 17:58:13 -0800 Subject: [PATCH 2/3] nav types: Add and demo RouteProp, to keep route-param types next to props. The type of `props.route.params` is very much like the type of a React component's `props` itself: it's part of the definition of the component's interface, and the most natural place for it to live is next to the definition of the component. With React Navigation upstream's guide for using type-checking: https://reactnavigation.org/docs/typescript/ (of which I'm looking at the 5.x version; sadly those docs don't allow permalinks to the current version, only to versions which are already outdated), this unfortunately gets flipped around, so that the route-params types are all defined in a central place associated with the navigator, and the individual components' definitions refer to that to get their respective types. That works in that it does at least get type-checked, but it makes it harder than it should be to read and understand the components, either to use them or to make changes to them. The effect is a lot like if for an ordinary function, the types of its parameters weren't defined up at the top but instead off in some other file. So, let's do things the other way around: the route-params type gets defined right there among the other props, next to the component's body; and the navigator's central list of params types refers to the components' types to get their respective route-params types. In this commit, we add a couple of small type aliases to use for that arrangement, and demonstrate using them on one of our smaller navigators. There are also some type-tests to check that these aliases behave as expected. --- src/__flow-tests__/nav-test.js | 78 ++++++++++++++++++++++++++++++++++ src/react-navigation.js | 61 +++++++++++++++++++++++++- src/sharing/ShareToPm.js | 7 +-- src/sharing/ShareToStream.js | 7 +-- src/sharing/SharingScreen.js | 12 ++---- 5 files changed, 149 insertions(+), 16 deletions(-) create mode 100644 src/__flow-tests__/nav-test.js diff --git a/src/__flow-tests__/nav-test.js b/src/__flow-tests__/nav-test.js new file mode 100644 index 00000000000..73540375be5 --- /dev/null +++ b/src/__flow-tests__/nav-test.js @@ -0,0 +1,78 @@ +/** + * Type-tests for navigation. + * + * @flow strict-local + */ + +import React, { type ComponentType } from 'react'; +import { + createStackNavigator, + type StackNavigationProp, + TransitionPresets, +} from '@react-navigation/stack'; + +import { type RouteProp, type RouteParamsOf } from '../react-navigation'; + +/* eslint-disable flowtype/generic-spacing */ + +// Test that `RouteProp` gives route.params the right type. +function testRouteParamTypes() { + type ProfileProps = {| + // skip navigation + +route: RouteProp<'Profile', {| +userId: string |}>, + |}; + + function Profile(props: ProfileProps) { + const { params } = props.route; + + (params.userId: string); + // $FlowExpectedError + (params.userId: empty); + + (('a': string): typeof params.userId); + // $FlowExpectedError + (('a': mixed): typeof params.userId); + + // $FlowExpectedError + params.nonsense; + } +} + +// Test that `RouteProp` gives type-checking of the route name +// at the navigator. +function testNavigatorTypes() { + // (The setup of this one is a bit cumbersome because we need to set up + // the navigator.) + + type ProfileProps = {| + +navigation: NavigationProp<'Profile'>, + +route: RouteProp<'Profile', {| +userId: string |}>, + |}; + declare var Profile: ComponentType; + + declare var Profile12: ComponentType<{| + +navigation: NavigationProp<'Profile1'>, + +route: RouteProp<'Profile2', {| +userId: string |}>, + |}>; + + type NavParamList = {| + Profile: RouteParamsOf, + Profile1: RouteParamsOf, + Profile2: RouteParamsOf, + |}; + + // prettier-ignore + type NavigationProp<+RouteName: $Keys = $Keys> = + StackNavigationProp; + + const Stack = createStackNavigator>(); + + + {/* Happy case is happy */} + + {/* $FlowExpectedError - mismatch of name with route prop */} + + {/* Should error but doesn't! on mismatch of name with navigation prop */} + + ; +} diff --git a/src/react-navigation.js b/src/react-navigation.js index e15dbc79b77..0c35258a447 100644 --- a/src/react-navigation.js +++ b/src/react-navigation.js @@ -1,11 +1,68 @@ -/* @flow strict-local */ +/** + * Helpers for using react-navigation and its relatives. + * + * @flow strict-local + */ -import { useNavigation as useNavigationInner } from '@react-navigation/native'; +import { type ElementConfig } from 'react'; +import { + useNavigation as useNavigationInner, + type LeafRoute, + type ScreenParams, +} from '@react-navigation/native'; import type { GlobalParamList } from './nav/globalTypes'; /* eslint-disable flowtype/generic-spacing */ +/** + * A type to use for the `route` prop on a screen component. + * + * Typically used in the component definition, to annotate its `route` prop. + * + * The type this produces is just like the one produced by `RouteProp` in + * the upstream libdef. The difference is that this one takes `RouteParams` + * as an argument directly, making it suitable for expressing the params + * type directly in the component's own source, just like props types. The + * navigator's central param list can then use `RouteParamsOf` to get the + * appropriate type from the component definition, rather than vice versa. + * + * @param {RouteName} - Must equal the route name this screen will be known + * by at the navigator. (This is type-checked at the navigator.) + * @param {RouteParams} - The type to use for `props.route.params`. + */ +export type RouteProp<+RouteName: string, +RouteParams: ScreenParams> = {| + ...LeafRoute, + +params: RouteParams, +|}; + +/** + * The type of the route params on the given screen component. + * + * This is intended for use in the params-list type for a navigator. + * Sample usage, following the same example as in the upstream docs + * at https://reactnavigation.org/docs/typescript/ : + * + * export type RootStackParamList = {| + * 'Profile': RouteParamsOf, + * |}; + * + * This allows each component's route-params type to be defined within the + * component's own source, just like props types. Continuing the example, + * the definition of the `Profile` component might contain a definition like + * this one: + * + * type Props = {| + * +navigation: StackNavigationProp, + * +route: RouteProp<'Profile', {| +userId: string |}, + * |}; + * + * (Or better yet, next to the params-list type we might define an alias + * to simplify the `navigation` prop's type, to something like + * `RootStackNavigationProp<'Profile'>`.) + */ +export type RouteParamsOf<-C> = $PropertyType<$PropertyType, 'route'>, 'params'>; + /** * Exactly like `useNavigation` upstream, but more typed. * diff --git a/src/sharing/ShareToPm.js b/src/sharing/ShareToPm.js index 6eb1edd4314..d1d13c89194 100644 --- a/src/sharing/ShareToPm.js +++ b/src/sharing/ShareToPm.js @@ -2,9 +2,10 @@ import React from 'react'; import { View, Image, ScrollView, Modal, BackHandler } from 'react-native'; -import type { SharingNavigationProp, SharingRouteProp } from './SharingScreen'; +import type { RouteProp } from '../react-navigation'; +import type { SharingNavigationProp } from './SharingScreen'; import * as NavigationService from '../nav/NavigationService'; -import type { Dispatch, Auth, GetText, UserId } from '../types'; +import type { Dispatch, Auth, GetText, SharedData, UserId } from '../types'; import { createStyleSheet } from '../styles'; import { TranslationContext } from '../boot/TranslationProvider'; import { connect } from '../react-redux'; @@ -56,7 +57,7 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| navigation: SharingNavigationProp<'share-to-pm'>, - route: SharingRouteProp<'share-to-pm'>, + route: RouteProp<'share-to-pm', {| sharedData: SharedData |}>, dispatch: Dispatch, auth: Auth, diff --git a/src/sharing/ShareToStream.js b/src/sharing/ShareToStream.js index 488ba04db28..cd8ae4a3188 100644 --- a/src/sharing/ShareToStream.js +++ b/src/sharing/ShareToStream.js @@ -2,9 +2,10 @@ import React from 'react'; import { View, Image, ScrollView, BackHandler } from 'react-native'; -import type { SharingNavigationProp, SharingRouteProp } from './SharingScreen'; +import type { SharingNavigationProp } from './SharingScreen'; +import type { RouteProp } from '../react-navigation'; import * as NavigationService from '../nav/NavigationService'; -import type { Dispatch, Subscription, Auth, GetText } from '../types'; +import type { Dispatch, Subscription, Auth, GetText, SharedData } from '../types'; import { createStyleSheet } from '../styles'; import { TranslationContext } from '../boot/TranslationProvider'; import { connect } from '../react-redux'; @@ -44,7 +45,7 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| navigation: SharingNavigationProp<'share-to-stream'>, - route: SharingRouteProp<'share-to-stream'>, + route: RouteProp<'share-to-stream', {| sharedData: SharedData |}>, dispatch: Dispatch, subscriptions: Map, diff --git a/src/sharing/SharingScreen.js b/src/sharing/SharingScreen.js index 52ec6ae1632..e8337fe9d3f 100644 --- a/src/sharing/SharingScreen.js +++ b/src/sharing/SharingScreen.js @@ -5,13 +5,13 @@ import { createMaterialTopTabNavigator, type MaterialTopTabNavigationProp, } from '@react-navigation/material-top-tabs'; -import type { RouteProp } from '@react-navigation/native'; import { FormattedMessage } from 'react-intl'; import type { GlobalParamList } from '../nav/globalTypes'; +import type { RouteParamsOf } from '../react-navigation'; import type { AppNavigationProp, AppNavigationRouteProp } from '../nav/AppNavigator'; import * as NavigationService from '../nav/NavigationService'; -import type { Dispatch, Auth, SharedData } from '../types'; +import type { Dispatch, Auth } from '../types'; import { createStyleSheet } from '../styles'; import { materialTopTabNavigatorConfig } from '../styles/tabs'; import { connect } from '../react-redux'; @@ -22,18 +22,14 @@ import ShareToStream from './ShareToStream'; import ShareToPm from './ShareToPm'; export type SharingNavigatorParamList = {| - 'share-to-stream': {| sharedData: SharedData |}, - 'share-to-pm': {| sharedData: SharedData |}, + 'share-to-stream': RouteParamsOf, + 'share-to-pm': RouteParamsOf, |}; export type SharingNavigationProp< +RouteName: $Keys = $Keys, > = MaterialTopTabNavigationProp; -export type SharingRouteProp< - RouteName: $Keys = $Keys, -> = RouteProp; - const Tab = createMaterialTopTabNavigator< GlobalParamList, SharingNavigatorParamList, From b746fb9c3d0cd129d68c198b5b9aa8f1065890a5 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 25 Jan 2021 21:09:39 -0800 Subject: [PATCH 3/3] eslint [nfc]: Add comments explaining each line of .eslintignore. Also discussing a bug in the interaction of `tools/test` with `eslint` that appears when you're on a branch that's touched one of the ignored files. --- .eslintignore | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/.eslintignore b/.eslintignore index 80e74ea438a..40d19e50965 100644 --- a/.eslintignore +++ b/.eslintignore @@ -1,2 +1,18 @@ +# If you run `tools/test` when you've modified a file that's ignored +# here, you might get a warning like this: +# 0:0 warning File ignored because of a matching ignore pattern. […] +# +# Don't worry about that warning; it won't appear in CI, and it won't +# appear on future `tools/test` runs when not editing these files. +# For more discussion, see: +# https://github.com/zulip/zulip-mobile/pull/4430#issuecomment-767297445 + +# These are purely type definitions, no runtime code. Most of them +# are third-party code, too, so naturally don't match our style. **/flow-typed/** -**/__flow-tests__/** \ No newline at end of file + +# These are type-tests, made up of code that gets type-checked but +# never actually run. They're naturally full of dead code which +# ESLint would complain about; and because the code never runs, other +# things it might complain about don't really matter anyway. +**/__flow-tests__/**