Remove direct `react-navigation-tabs` dependency. #4114

chrisbobbe · 2020-05-15T21:58:59Z

This is almost NFC, considering how thin react-navigation's wrapper around react-navigation-tabs's createBottomTabNavigator is.

```
get createBottomTabNavigator() {
  return require('react-navigation-tabs').createBottomTabNavigator;
},
```

chrisbobbe · 2020-05-15T22:21:18Z

~~Oh, oops. There's one more usage of react-navigation-tabs — should be able to take care of that the same way; I'll update the PR and re-post here.~~

EDIT: Fixed!

gnprice · 2020-05-26T20:16:32Z

I like the reasoning for removing the direct dependency on react-navigation-tabs.

We should also remove the corresponding libdef in flow-typed/npm/.

For the fixmes, compare the fixmes that were removed in 0d2066f when we switched to using react-navigation-tabs. They were narrower, which meant we got type-checking on the rest of what we were passing to react-navigation there. That seems worth preserving -- there's a bunch of other options involved that it's good to check on.

In the analysis of what's causing the need for the fixmes:

One summary of the error on, e.g., `HomeTab`, is, "Hey, you should
write down the fact that HomeTab gets special treatment because it's
passed into `createBottomTabNavigator`. It gets certain props, for
example, like the `navigation` prop."

I don't think this is quite it, because if you look at the error it doesn't mention a name navigation. It seems to be entirely about the navigationOptions property. Which in particular is a property on the component type, i.e. a property on the value HomeTab itself, i.e. if it's a class then a static property on the class.

It's actually kind of puzzling to me what Flow is doing there, because the libdef says the property is optional, so it should be fine to simply not have it. (As indeed we don't.) I suspect the story involves something like React.ComponentType<…> being handled specially and not as "an object type" at all.

I think I can fix the issue that our typed connect doesn't pass through the information about static properties on the component type. The main annoying part will be testing that the fix doesn't cause Flow to lose type-checking precision somewhere else.

But also, on testing one possible fix for that, I find that the react-navigation libdef doesn't actually allow a useful navigationOptions anyway! I tried putting

 class HomeTab extends PureComponent<Props> {
+  static navigationOptions = { tabBarLabel: 'Home' };
+

and that produces an error saying tabBarLabel isn't allowed. And in fact it's the same error if I cut out connect entirely. So... we wouldn't get useful type-checking if we actually used this feature of the react-navigation API, which rather lessens the motivation to fix the connect type so that we could try.

chrisbobbe · 2020-05-28T04:46:43Z

Oh no! I'm about 80% sure I posted a lengthy reply to this, referring to my earlier thoughts on this extra property on the component, here. Hmm, maybe I didn't follow through, or it got lost somehow.

chrisbobbe · 2020-05-28T05:23:52Z

Oh no! I'm about 80% sure I posted a lengthy reply to this, referring to my earlier thoughts on this extra property on the component, here. Hmm, maybe I didn't follow through, or it got lost somehow.

Ah! I think @gnprice and I discussed it on the phone; that must have been it.

The next commit's message is quite long, so here's the motivation for the FlowFixMe. Importing `createBottomTabNavigator` from `react-navigation` is good because it means more type checking! Sadly, getting that to really work in all the ways we want won't be easy. To sum up, we could get *part* of the way there by making tweaks to our typed `connect` (and making sure those tweaks didn't break something or lose type-checking elsewhere!). But there's an additional hurdle in something that's not quite right in the `react-navigation` libdef. Greg goes into more detail at the PR thread [1]. See also discussion [2]. [1]: zulip#4114 (comment) [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878259

The next commit's message is quite long, so here's the motivation for the FlowFixMes. Importing `createBottomTabNavigator` from `react-navigation` is good because it means more type checking! Sadly, getting that to really work in all the ways we want won't be easy. To sum up, we could get *part* of the way there by making tweaks to our typed `connect` (and making sure those tweaks didn't break something or lose type-checking elsewhere!). But there's an additional hurdle in something that's not quite right in the `react-navigation` libdef. The same goes for `createMaterialTopTabNavigator`. Greg goes into more detail at the PR thread [1]. See also discussion [2]. [1]: zulip#4114 (comment) [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878259

chrisbobbe · 2020-06-04T23:31:56Z

Thanks for the review! Just pushed my latest changes.

The next commit's message is quite long, so here's the motivation for the FlowFixMes. Importing `createBottomTabNavigator` from `react-navigation` is good because it means more type checking! Sadly, getting that to really work in all the ways we want won't be easy. To sum up, we could get *part* of the way there by making tweaks to our typed `connect` (and making sure those tweaks didn't break something or lose type-checking elsewhere!). But there's an additional hurdle in something that's not quite right in the `react-navigation` libdef. The same goes for `createMaterialTopTabNavigator`. Greg goes into more detail at the PR thread [1]. See also discussion [2]. [1]: zulip#4114 (comment) [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878259

chrisbobbe · 2020-06-25T01:09:16Z

We should also remove the corresponding libdef in flow-typed/npm/.

Ah, just did this, and rebased.

The next commit's message is quite long, so here's the motivation for the FlowFixMes. Importing `createBottomTabNavigator` from `react-navigation` is good because it means more type checking! Sadly, getting that to really work in all the ways we want won't be easy. To sum up, we could get *part* of the way there by making tweaks to our typed `connect` (and making sure those tweaks didn't break something or lose type-checking elsewhere!). But there's an additional hurdle in something that's not quite right in the `react-navigation` libdef. The same goes for `createMaterialTopTabNavigator`. Greg goes into more detail at the PR thread [1]. See also discussion [2]. [1]: zulip#4114 (comment) [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/react-navigation.20types/near/878259

See the previous commit for the reason for the $FlowFixMes. In 0d2066f, we followed a warning to use the `react-native-tabs` package: ``` console.warn node_modules/react-navigation/src/react-navigation.js:180 TabBarBottom is deprecated. Please use the react-navigation-tabs package instead: https://github.com/react-navigation/react-navigation-tabs ``` An important bit of context that's missing from this warning is that `react-navigation-tabs`'s function `createBottomTabNavigator`, specifically, is what replaces what was previously done with `TabBarBottom`. `react-navigation` also exports another function `createBottomTabNavigator`. So, is it so important to use the export from `react-navigation-tabs`? Turns out it's not; that function in `react-navigation` is an extremely thin wrapper on the one in `react-navigation-tabs`: In node_modules/react-navigation/src/react-navigation.js:69: ``` get createBottomTabNavigator() { return require('react-navigation-tabs').createBottomTabNavigator; }, ``` So, `react-navigation` depends on `react-navigation-tabs`, so we can remove the latter as a direct dependency in our project. The `react-navigation` v2 doc [1] also doesn't say anything about needing to depend directly on `react-navigation-tabs`. See also further discussion [2]. It would also have been possible to remove the direct dependency on `react-native-screens`, but we're planning to use that later; that's issue zulip#4111. The same story applies to `createMaterialTopTabNavigator`. [1]: https://reactnavigation.org/docs/2.x/tab-based-navigation/ [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877185

chrisbobbe · 2020-09-02T22:29:00Z

I'm closing this, as we'll need to depend on react-navigation-tabs directly in react-navigation v4: https://reactnavigation.org/docs/4.x/upgrading-from-3.x/#install-the-new-packages (see the first subheader, "Install the new packages").

…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 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, take a conservative approach 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, 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 } }`. - 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>` [3]. 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] Like at react-navigation/react-navigation#3643 (comment), but with `NavigationStackProp` instead of `NavigationScreenProp`.

And leave in place the actual, meaningful libdef, so that that one will prevail. Now, we get proper type-checking for our calls of `createMaterialTopTabNavigator` and `createBottomTabNavigator`. This means a return of the kind of `$FlowFixMe`s [1] we removed in 0d2066f, at a time when there wasn't a suitable libdef for react-navigation-tabs. [1] Some 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/.

…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`.

And leave in place the actual, meaningful libdef, so that that one will prevail. Now, we get proper type-checking for our calls of `createMaterialTopTabNavigator` and `createBottomTabNavigator`. This means a return of the kind of `$FlowFixMe`s [1] we removed in 0d2066f, at a time when there wasn't a suitable libdef for react-navigation-tabs. [1] Some 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/.

…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`.

chrisbobbe changed the title ~~Remove direct react-native-tabs dependency.~~ Remove direct react-navigation-tabs dependency. May 15, 2020

chrisbobbe force-pushed the pr-remove-r-n-tabs branch from e04d27b to d71abdd Compare May 15, 2020 22:00

chrisbobbe marked this pull request as draft May 15, 2020 22:20

chrisbobbe force-pushed the pr-remove-r-n-tabs branch from d71abdd to c0c50d5 Compare May 20, 2020 17:06

chrisbobbe marked this pull request as ready for review May 20, 2020 17:12

chrisbobbe force-pushed the pr-remove-r-n-tabs branch from c0c50d5 to 40ffc19 Compare June 4, 2020 23:28

chrisbobbe force-pushed the pr-remove-r-n-tabs branch from 40ffc19 to 8b9f5ab Compare June 4, 2020 23:29

chrisbobbe mentioned this pull request Jun 25, 2020

[android] Handle receiving shares from other apps #4124

Merged

chrisbobbe force-pushed the pr-remove-r-n-tabs branch from 8b9f5ab to 8c39c5f Compare June 25, 2020 01:07

chrisbobbe requested a review from gnprice June 25, 2020 01:09

chrisbobbe mentioned this pull request Jul 3, 2020

home screen: Use material tabs for bottom navigation. #4176

Open

chrisbobbe added 2 commits August 25, 2020 15:47

chrisbobbe force-pushed the pr-remove-r-n-tabs branch from 8c39c5f to 31b59f0 Compare August 25, 2020 22:49

chrisbobbe closed this Sep 2, 2020

This was referenced Sep 3, 2020

Upgrade react-navigation to v3 #3649

Closed

Upgrade React Navigation v3 and v4. #4249

Merged

chrisbobbe deleted the pr-remove-r-n-tabs branch November 5, 2021 00:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove direct `react-navigation-tabs` dependency. #4114

Remove direct `react-navigation-tabs` dependency. #4114

chrisbobbe commented May 15, 2020

chrisbobbe commented May 15, 2020 •

edited

Loading

gnprice commented May 26, 2020

chrisbobbe commented May 28, 2020 •

edited

Loading

chrisbobbe commented May 28, 2020

chrisbobbe commented Jun 4, 2020

chrisbobbe commented Jun 25, 2020

chrisbobbe commented Sep 2, 2020 •

edited

Loading

Remove direct react-navigation-tabs dependency. #4114

Remove direct react-navigation-tabs dependency. #4114

Conversation

chrisbobbe commented May 15, 2020

chrisbobbe commented May 15, 2020 • edited Loading

gnprice commented May 26, 2020

chrisbobbe commented May 28, 2020 • edited Loading

chrisbobbe commented May 28, 2020

chrisbobbe commented Jun 4, 2020

chrisbobbe commented Jun 25, 2020

chrisbobbe commented Sep 2, 2020 • edited Loading

Remove direct `react-navigation-tabs` dependency. #4114

Remove direct `react-navigation-tabs` dependency. #4114

chrisbobbe commented May 15, 2020 •

edited

Loading

chrisbobbe commented May 28, 2020 •

edited

Loading

chrisbobbe commented Sep 2, 2020 •

edited

Loading