nav: Fix navigation-prop types; switch most NavigationService use to navigation props #5408

gnprice · 2022-06-10T01:02:23Z

(Note this branch sits atop #5407.)

This branch does several related things in sequence:

Fix the type of useNavigation to contain less information (for accuracy) and of our navigation props to contain more information (also accurate, but for usefulness.)
Switch most uses of NavigationService to instead use a component's navigation prop, or else useNavigation.
Replace all the resulting navigation.dispatch(navigateToFoo(…)) calls with navigation.push(…). (But the NavigationService.dispatch calls that remained after the previous step stay in place.)
Delete all the navigateToFoo nav-action creators (in navActions.js) that are no longer used -- which is 22 out of the 34 -- leaving only the ones that are used with NavigationService.dispatch.

Switching away from NavigationService is recommended by React Navigation upstream, as described at #4417. We also now get type-checking on those navigation.push calls -- Flow is able to check that the route params we pass line up with what the route in question expects -- which wasn't/isn't the case for the implementations of the navActions functions.

Fixes: #4417

chrisbobbe

Thanks, this will be nice to have! See a question, below.

chrisbobbe · 2022-06-10T15:14:38Z

src/nav/AppNavigator.js

+// Confirm that this navigators' screens' navigation props will be valid
+// as the methods type.
+// eslint-disable-next-line
+(n: AppNavigationProp<>): AppNavigationMethods => n;


"navigator's", I think?

chrisbobbe · 2022-06-10T15:35:26Z

src/nav/AppNavigator.js

+  // Intersecting with the methods type should be redundant -- the
+  // StackNavigationProp type should already be a subtype of it.  But when
+  // we check that below, we'd get puzzling errors.  So add it explicitly.
+  AppNavigationMethods;


So I guess we (or I) don't quite know that StackNavigationProp<AppNavigatorParamList, RouteName> is already a subtype of AppNavigationMethods. Is that OK? It might be OK, I'm just not sure.

Some things I notice from the 8 errors when I remove the intersection atop this commit:

I see the familiar "Class instances are not subtypes of object types" from https://medium.com/flow-type/sound-typing-for-this-in-flow-d62db2af969e. The relevant class, I think, is PrivateValueStore. That's the only non-React-component class in types/@react-navigation. On one of its properties, there's a comment saying "UGLY HACK! DO NOT USE THE TYPE!!!" Even if that comment ends up not applying (I'm not sure if it does)…if we really are working with class instances, then it might be correct to change some object types into Flow interfaces, maybe?

Here's one of the errors:

Error ----------------------------------------------------------------------------------- src/nav/AppNavigator.js:113:51 Cannot return `n` because property `gestureCancel` is missing in `EventMapCore` [1] but exists in object type [2] in type argument `EventMap` [3] of the second parameter of property `addListener`. [incompatible-return] src/nav/AppNavigator.js:113:51 113| (n: AppNavigationProp<>): AppNavigationMethods => n; ^ References: src/nav/AppNavigator.js:101:17 101| EventConsumer<EventMapCore<NavigationState<AppNavigatorParamList>>>; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [1] types/@react-navigation/core/lib/typescript/src/types.js.flow:215:19 215| } & EventConsumer<{| ...EventMap, ...EventMapCore<State> |}> & ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2] types/@react-navigation/core/lib/typescript/src/types.js.flow:90:35 90| export type EventListenerCallback<EventMap: EventMapBase, EventName: $Keys<EventMap>> = ( ^^^^^^^^ [3]

"gestureCancel" is one of the stack-nav-specific event names, I think. I haven't yet quite understood why we give AppNavigationMethods the non-stack-nav-specific addListener, while we do give it the stack-nav-specific method push (as mentioned in AppNavigationMethods's jsdoc). When I imagine why that might be, I think vaguely of function subtyping, and things being in "input" and "output" positions. Am I onto something there?

The relevant class, I think, is PrivateValueStore. That's the only non-React-component class in types/@react-navigation. On one of its properties, there's a comment saying "UGLY HACK! DO NOT USE THE TYPE!!!"

Yeah. I think that whole PrivateValueStore type is there to do something that (a) doesn't describe anything about the actual runtime values -- there's no actual object floating around with properties a and b and c -- and (b) is there in order to be able to use a particular TypeScript feature. Namely, in the definition of CompositeNavigationProp, in the "conditional types" that are quoted commented out because TsFlower doesn't support them, you can see things like this:

/** * Event consumer config should refer to the config specified in the first type * This allows typechecking `addListener`/`removeListener` */ A extends NavigationProp<any, any, any, any, infer E> ? E : {}

What that's doing is taking a type A (a type parameter of CompositeNavigationProp), which is expected to be some NavigationProp<…> type, and extracting out from it what its EventMap type should be, i.e. the fifth type parameter of NavigationProp.

And then what that comment in PrivateValueStore is saying:

/** * UGLY HACK! DO NOT USE THE TYPE!!! * * TypeScript requires a type to be used to be able to infer it. * The type should exist as its own without any operations such as union. * So we need to figure out a way to store this type in a property.

is that that infer E is only going to work if the thing being inferred appears in a nice direct way in the definition of the NavigationProp type. If you had e.g.

type Union<T, U> = T | U;

then saying A extends Union<any, infer U> ? U : {} couldn't usefully work, because for any given type A there may be all kinds of different types U that would fit. I'm not sure what TS's exact rules are here, but apparently they don't allow inferring the EventMap type from the way it's used for real in NavigationProp.

So the react-navigation authors' solution has been to introduce this PrivateValueStore type, and end the NavigationProp definition with & PrivateValueStore<ParamList, RouteName, EventMap>. Then

A extends NavigationProp<any, any, any, any, infer E> ? E : {}

becomes basically just a shorthand for saying

A extends NavigationProp<any, any, any, any, any> ? A['']['c'] : {}

with no infer.

Anyway, so that's what I've worked out PrivateValueStore is doing. Given that, I think for any type errors that involve it, we shouldn't worry about them potentially being signs of a real problem.

I haven't yet quite understood why we give AppNavigationMethods the non-stack-nav-specific addListener, while we do give it the stack-nav-specific method push (as mentioned in AppNavigationMethods's jsdoc).

Basically, I know that the push method is available when you're on a screen inside a child navigator (we've been using the functionality since forever via NavigationService.dispatch; and empirically it is there, and works, as a method on the navigation prop too, as seen by manually testing the tip of this branch); but I'm not sure whether events like gestureCancel are. I don't have as clear an idea of what those are meant to do, to understand whether I'd think they should be available there, either.

I did find one indication that they're not supposed to be available -- it's that CompositeNavigationProp type I discussed above in a different context. Here's the definition, from node_modules/@react-navigation/core/src/types.tsx:

export type CompositeNavigationProp< A extends NavigationProp<ParamListBase, string, any, any>, B extends NavigationHelpersCommon<ParamListBase, any> > = Omit<A & B, keyof NavigationProp<any>> & NavigationProp< /** * Param list from both navigation objects needs to be combined * For example, we should be able to navigate to screens in both A and B */ (A extends NavigationHelpersCommon<infer T> ? T : never) & (B extends NavigationHelpersCommon<infer U> ? U : never), // … /** * Event consumer config should refer to the config specified in the first type * This allows typechecking `addListener`/`removeListener` */ A extends NavigationProp<any, any, any, any, infer E> ? E : {} >;

And here's the docs on what that CompositeNavigationProp type is supposed to mean:
https://reactnavigation.org/docs/typescript/#combining-navigation-props

So that definition takes the trouble to ensure that the routes from both parent and child are available; but for the events, it only includes those from the child.

When I imagine why that might be, I think vaguely of function subtyping, and things being in "input" and "output" positions. Am I onto something there?

And yeah, I think that's the kind of issue that causes this to be a type error. Specifically:

The AppNavigationProp types have a larger set of events supported by their addListener method.

The AppNavigationMethods type offers a smaller set of events.

That means that an AppNavigationMethods value truly shouldn't be usable as an AppNavigationProp value. But ideally it shouldn't be an obstacle to taking an AppNavigationProp value and using it as an AppNavigationMethods -- the prop type should be a subtype of the methods type.

That ideal would be met if all the relevant pieces of each of these types had the right things covariant and contravariant, aka had the right things marked as "output positions" and "input positions". Oh also: and had inexact object types in the necessary places, so that a type with more properties is a subtype of one with a subset of them.

But apparently that's not the case.

Thanks for that explanation!

gnprice · 2022-06-10T23:14:43Z

Thanks for the review! Revision pushed, with comments added based on the discussion above.

chrisbobbe · 2022-06-14T00:16:03Z

Thanks, LGTM! Please merge at will. You may want to see the commit I added to #5412 first; it's a correction to my recent #5411.

See patch commit message for more discussion.

After all, it can be used from within a child navigator, where the navigation object will look different from how it looks on screens that belong to AppNavigator.

See patch commit message for discussion.

Since the recent commit 067a450 "types: Switch to TsFlower for react-navigation types", `props.route.params` has been ending up with a type of `empty`, in our screens where we consume that. Yikes! That means it's treated as a magic shapeshifter value -- the uses of it are effectively not type-checked. Because that state of affairs only developed recently, no genuine type errors snuck in past it. It did have the effect of suppressing an error we'd have otherwise gotten at the initialParams uses in SharingScreen, which was due to the oddity in the library type definition fixed in the preceding commit. The type of `props.route` on each screen comes from `RouteProp`, so that's where we need to make a fix. Applying `$Exact` to the inexact `Route` type, before we go and spread it into this exact object type, fixes the issue. After doing that, we can also remove the redundant intersection we'd had here; those puzzling errors no longer recur.

For example, in HomeScreen we have a `navigation` prop that is a MainTabsNavigationProp, because HomeScreen appears under the navigator created in MainTabsScreen. That prop has a method `navigation.push` -- a stack-navigation method -- even though the navigator is a tab navigator. That's because the navigator's parent is our main app navigator, a stack navigator. Our type MainTabsNavigationProp hadn't reflected that, so the types had said that `navigation.push` method didn't exist there. Amend that type, and similar ones, to reflect that it does.

…prop Now that we have this AppNavigationMethods type, it better reflects what EditStreamCard (and its dependency InputRowRadioButtons) really need from the `navigation` prop they're handed: they need the ability to invoke methods like `addListener` and `navigate`.

We moved this screen a while ago from being one of the main tabs to being a screen reached by stack navigation. Since then, this type has been invalid because 'settings' is not among the route keys in the "main tabs" navigator. Flow hasn't been giving us an error about it, because we haven't had any actual value expressions around that have this type -- we never look at the `navigation` prop in this component. We do so in the next commit, which caused me to notice this error. Fix it.

React Navigation upstream has since the 5.x release been discouraging the use of the strategy we call NavigationService. See discussion at 140c28c and: zulip#4417 Instead, the recommendation is to use the `navigation` prop where available, and failing that the `useNavigation` hook. In this commit, we make that change for all sites where we were using NavigationService in a function component that is a screen in a navigator, and gets its own navigation prop.

As described earlier in this series, we're following React Navigation upstream's recommendation by migrating from NavigationService. In this commit, we take care of NavigationService use sites that are in class components that are screens in our navigators, and so get their own navigation props.

This covers all sites where we were in a Hooks-based component (so could use useNavigation) but one that isn't a screen directly in a navigator (so doesn't have its own navigation prop to use.) Still using NavigationService are * One component (ShareWrapper) which is a class (so can't use useNavigation) but not a screen (so doesn't get a navigation prop directly.) We'll take care of this next. * Sites that aren't directly on a React component. We'll let these be for the present. Many of these (like src/webview/handleOutboundEvents.js, and action sheets) are straightforwardly invoked from particular components, so should probably just take a navigation object as a parameter. For others (like doEventActionSideEffects) it's less direct; but they're still ultimately invoked from some React component, so the same solution can be applied, just with a bit more plumbing.

This replaces all the call sites of the form `navigation.dispatch(navigateToFoo(…))`. We effectively inline the definitions of the respective `navigateToFoo` nav-action creators, to directly invoke `push` on the navigation prop. A nice feature of this change is that while the definitions of these nav-action creators in `navActions.js` have no effective type-checking -- in particular, there's no check that the given route params correspond to the types the given route expects -- these `push` calls do get such type-checking, because these `navigation` objects have appropriate types. This commit doesn't cover all the call sites of these `navigateToFoo`. Where we're dispatching the result on `NavigationService` instead of on a component's `navigation` prop (whether gotten directly, passed down from a parent component, or gotten equivalently through `useNavigation`), there is no `push` method, so in those places we stick for now with `dispatch`.

This covers most of the `navigateToFoo` nav-action creators, but sadly not all of them: as discussed in the previous commit where we simplified away most of the call sites, there are still some that remain in places where we still use NavigationService. So for now we keep the nav-action creators that are used there. Fixes: zulip#4417

This way, we can stop using NavigationService in these action sheets, and instead use `navigation.push` calls. React Navigation upstream recommends avoiding the NavigationService approach where possible, and this goes in that direction; see more in zulip#5408.

Further in the direction of zulip#5408, which switched most of our then-existing uses of NavigationService to use `navigation` instead. As noted there: > Switching away from `NavigationService` is recommended by React > Navigation upstream, as described at zulip#4417. We also now get > type-checking on those `navigation.push` calls -- Flow is able to > check that the route params we pass line up with what the route in > question expects -- which wasn't/isn't the case for the > implementations of the `navActions` functions. This causes a few of the navActions functions to lose their last remaining caller, so delete those.

This way, we can stop using NavigationService in these action sheets, and instead use `navigation.push` calls. React Navigation upstream recommends avoiding the NavigationService approach where possible, and this goes in that direction; see more in zulip#5408.

Further in the direction of zulip#5408, which switched most of our then-existing uses of NavigationService to use `navigation` instead. As noted there: > Switching away from `NavigationService` is recommended by React > Navigation upstream, as described at zulip#4417. We also now get > type-checking on those `navigation.push` calls -- Flow is able to > check that the route params we pass line up with what the route in > question expects -- which wasn't/isn't the case for the > implementations of the `navActions` functions. This causes a few of the navActions functions to lose their last remaining caller, so delete those.

…etc.) This way, we can stop using NavigationService in these action sheets, and instead use `navigation.push` calls. React Navigation upstream recommends avoiding the NavigationService approach where possible, and this goes in that direction; see more in zulip#5408.

Further in the direction of zulip#5408, which switched most of our then-existing uses of NavigationService to use `navigation` instead. As noted there: > Switching away from `NavigationService` is recommended by React > Navigation upstream, as described at zulip#4417. We also now get > type-checking on those `navigation.push` calls -- Flow is able to > check that the route params we pass line up with what the route in > question expects -- which wasn't/isn't the case for the > implementations of the `navActions` functions. This causes a few of the navActions functions to lose their last remaining caller, so delete those.

…etc.) This way, we can stop using NavigationService in these action sheets, and instead use `navigation.push` calls. React Navigation upstream recommends avoiding the NavigationService approach where possible, and this goes in that direction; see more in zulip#5408.

Further in the direction of zulip#5408, which switched most of our then-existing uses of NavigationService to use `navigation` instead. As noted there: > Switching away from `NavigationService` is recommended by React > Navigation upstream, as described at zulip#4417. We also now get > type-checking on those `navigation.push` calls -- Flow is able to > check that the route params we pass line up with what the route in > question expects -- which wasn't/isn't the case for the > implementations of the `navActions` functions. This causes a few of the navActions functions to lose their last remaining caller, so delete those.

gnprice added the a-navigation label Jun 10, 2022

chrisbobbe reviewed Jun 10, 2022

View reviewed changes

gnprice force-pushed the pr-nav-props branch from 651b1aa to 9e5629c Compare June 10, 2022 23:14

gnprice added 13 commits June 13, 2022 22:48

dep types: Fix up type of push on react-navigation stack-nav prop

ea7a6aa

See patch commit message for more discussion.

nav types: Have useNavigation return only what's common across the app

8325afb

After all, it can be used from within a child navigator, where the navigation object will look different from how it looks on screens that belong to AppNavigator.

dep types: Don't let initialParams be partial in react-navigation

d4a3cd0

See patch commit message for discussion.

nav: Pass navigation prop to ShareWrapper, replacing NavigationService

0d2ad29

gnprice force-pushed the pr-nav-props branch from 9e5629c to fc78777 Compare June 14, 2022 05:50

gnprice merged commit fc78777 into zulip:main Jun 14, 2022

gnprice deleted the pr-nav-props branch June 14, 2022 05:51

gnprice mentioned this pull request Jun 14, 2022

Remove most or all definitions in navActions. #4417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

nav: Fix navigation-prop types; switch most NavigationService use to navigation props #5408

nav: Fix navigation-prop types; switch most NavigationService use to navigation props #5408

Uh oh!

gnprice commented Jun 10, 2022

Uh oh!

chrisbobbe left a comment

Uh oh!

chrisbobbe Jun 10, 2022

Uh oh!

chrisbobbe Jun 10, 2022

Uh oh!

gnprice Jun 10, 2022

Uh oh!

gnprice Jun 10, 2022

Uh oh!

gnprice Jun 10, 2022

Uh oh!

chrisbobbe Jun 13, 2022

Uh oh!

gnprice commented Jun 10, 2022

Uh oh!

chrisbobbe commented Jun 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

nav: Fix navigation-prop types; switch most NavigationService use to navigation props #5408

nav: Fix navigation-prop types; switch most NavigationService use to navigation props #5408

Uh oh!

Conversation

gnprice commented Jun 10, 2022

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jun 10, 2022

Uh oh!

chrisbobbe commented Jun 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants