-
-
Notifications
You must be signed in to change notification settings - Fork 657
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Decouple navigation from Redux #3804
Comments
Possibly also relevant: the existing "upgrade to v3" issue, #3649. |
As @gnprice mentions here, fixing this issue may help us in a somewhat surprising way: it appears that many of our import cycles go through Concretely, I think one place where incremental work could be done is in gradually reducing our consumption of the The exports of
|
When we eventually remove react-navigation-redux-helpers, we should also remove @react-navigation/core from our direct dependencies. See this note on the version of react-navigation-redux-helpers we're using, saying we need to depend on it (they've also made it a peer-dep):
And react-navigation's note stating their preference for not having people depend on it directly:
|
v4 includes lots of housekeeping changes that aim to make maintenance easier [1]. In particular, we no longer grab stack, tab, or drawer navigation logic from react-navigation itself; we must now depend directly on separate libraries that provide these and import from them. - We got a head start on this in the v3 upgrade (in a recent commit) by ensuring we had react-navigation-{tabs,stack,drawer} installed at v3-compatible versions. - Now, we bump these to their latest versions (no peer-dep warnings when we do this!) and default to using carets in their version ranges since we see no indication that we can't. - This means installing react-native-reanimated, as we knew we'd need to. (We mock it in our Jest setup, following some instructions [2].) For react-navigation-stack, we add peer dependencies react-native-safe-area-context and @react-native-community/masked-view. The upgrade guide asks us to stop depending on @react-navigation/core and @react-navigation/native and change any imports of those to react-navigation imports instead. We don't have any imports to change, and we can freely remove @react-navigation/native. We can't yet remove @react-navigation/core because it's still a peer dependency of react-navigation-redux-helpers [3]. So we keep it, and align its version range with the one in react-navigation's dependencies. There's a long list of identifiers whose usage has changed. Searching our code for each one, these are the interesting ones: - `create*Navigator` being imported from the respective react-navigation-{tabs,stack,drawer} library; quite easy to handle - `cardStyle` (in stack nav config) being moved to `navigationOptions` (or `defaultNavigationOptions`). We deleted our only use of `cardStyle` in the recent react-navigation v3 upgrade because we only used it to make the background color white and, white became the default background color in that upgrade. Finally, attempt to upgrade libdefs for the dependencies we touched, if we import from them directly. We get new version-appropriate libdefs for react-navigation, react-navigation-drawer, and react-navigation-tabs. Unfortunately react-navigation-stack only has a v1 libdef. [1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/ [2] See https://reactnavigation.org/docs/testing/. We don't include the following line also recommended there: ``` // Silence the warning: Animated: `useNativeDriver` is not // supported because the native animated module is missing jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper'); ``` We don't get that warning in the first place, and it's not clear what mock is being applied (apart from React Native's own default one). If we need to mock `NativeAnimatedHelper` in the future, we shouldn't point to the internal path in react-native; it should go in our existing react-native mock (alongside NativeModules, etc.). [3] zulip#3804 (comment)
v4 includes lots of housekeeping changes that aim to make maintenance easier [1]. In particular, we no longer grab stack, tab, or drawer navigation logic from react-navigation itself; we must now depend directly on separate libraries that provide these and import from them. - We got a head start on this in the v3 upgrade (in a recent commit) by ensuring we had react-navigation-{tabs,stack,drawer} installed at v3-compatible versions. - Now, we bump these to their latest versions (no peer-dep warnings when we do this!) and default to using carets in their version ranges since we see no indication that we can't. - This means installing react-native-reanimated, as we knew we'd need to. (We mock it in our Jest setup, following some instructions [2].) For react-navigation-stack, we add peer dependencies react-native-safe-area-context and @react-native-community/masked-view. The upgrade guide asks us to stop depending on @react-navigation/core and @react-navigation/native and change any imports of those to react-navigation imports instead. We don't have any imports to change, and we can freely remove @react-navigation/native. We can't yet remove @react-navigation/core because it's still a peer dependency of react-navigation-redux-helpers [3]. So we keep it, and align its version range with the one in react-navigation's dependencies. There's a long list of identifiers whose usage has changed. Searching our code for each one, these are the interesting ones: - `create*Navigator` being imported from the respective react-navigation-{tabs,stack,drawer} library; quite easy to handle - `cardStyle` (in stack nav config) being moved to `navigationOptions` (or `defaultNavigationOptions`). We deleted our only use of `cardStyle` in the recent react-navigation v3 upgrade because we only used it to make the background color white and, white became the default background color in that upgrade. Finally, attempt to upgrade libdefs for the dependencies we touched, if we import from them directly. We get new version-appropriate libdefs for react-navigation, react-navigation-drawer, and react-navigation-tabs. Unfortunately react-navigation-stack only has a v1 libdef. [1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/ [2] See https://reactnavigation.org/docs/testing/. We don't include the following line also recommended there: ``` // Silence the warning: Animated: `useNativeDriver` is not // supported because the native animated module is missing jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper'); ``` We don't get that warning in the first place, and it's not clear what mock is being applied (apart from React Native's own default one). If we need to mock `NativeAnimatedHelper` in the future, we shouldn't point to the internal path in react-native; it should go in our existing react-native mock (alongside NativeModules, etc.). [3] zulip#3804 (comment) Fixes: zulip#4248
v4 includes lots of housekeeping changes that aim to make maintenance easier [1]. In particular, we no longer grab stack, tab, or drawer navigation logic from react-navigation itself; we must now depend directly on separate libraries that provide these and import from them. - We got a head start on this in the v3 upgrade (in a recent commit) by ensuring we had react-navigation-{tabs,stack,drawer} installed at v3-compatible versions. - Now, we bump these to their latest versions (no peer-dep warnings when we do this!) and default to using carets in their version ranges since we see no indication that we can't. - This means installing react-native-reanimated, as we knew we'd need to. (We mock it in our Jest setup, following some instructions [2].) For react-navigation-stack, we add peer dependencies react-native-safe-area-context and @react-native-community/masked-view. The upgrade guide asks us to stop depending on @react-navigation/core and @react-navigation/native and change any imports of those to react-navigation imports instead. We don't have any imports to change, and we can freely remove @react-navigation/native. We can't yet remove @react-navigation/core because it's still a peer dependency of react-navigation-redux-helpers [3]. So we keep it, and align its version range with the one in react-navigation's dependencies. There's a long list of identifiers whose usage has changed. Searching our code for each one, these are the interesting ones: - `create*Navigator` being imported from the respective react-navigation-{tabs,stack,drawer} library; quite easy to handle - `cardStyle` (in stack nav config) being moved to `navigationOptions` (or `defaultNavigationOptions`). We deleted our only use of `cardStyle` in the recent react-navigation v3 upgrade because we only used it to make the background color white and, white became the default background color in that upgrade. Finally, attempt to upgrade libdefs for the dependencies we touched, if we import from them directly. We get new version-appropriate libdefs for react-navigation, react-navigation-drawer, and react-navigation-tabs. Unfortunately react-navigation-stack only has a v1 libdef. [1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/ [2] See https://reactnavigation.org/docs/testing/. We don't include the following line also recommended there: ``` // Silence the warning: Animated: `useNativeDriver` is not // supported because the native animated module is missing jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper'); ``` We don't get that warning in the first place, and it's not clear what mock is being applied (apart from React Native's own default one). If we need to mock `NativeAnimatedHelper` in the future, we shouldn't point to the internal path in react-native; it should go in our existing react-native mock (alongside NativeModules, etc.). [3] zulip#3804 (comment) Fixes: zulip#4248
v4 includes lots of housekeeping changes that aim to make maintenance easier [1]. In particular, we no longer grab stack, tab, or drawer navigation logic from react-navigation itself; we must now depend directly on separate libraries that provide these and import from them. - We got a head start on this in the v3 upgrade (in a recent commit) by ensuring we had react-navigation-{tabs,stack,drawer} installed at v3-compatible versions. - Now, we bump these to their latest versions (no peer-dep warnings when we do this!) and default to using carets in their version ranges since we see no indication that we can't. - This means installing react-native-reanimated, as we knew we'd need to. (We mock it in our Jest setup, following some instructions [2].) For react-navigation-stack, we add peer dependencies react-native-safe-area-context and @react-native-community/masked-view. The upgrade guide asks us to stop depending on @react-navigation/core and @react-navigation/native and change any imports of those to react-navigation imports instead. We don't have any imports to change, and we can freely remove @react-navigation/native. We can't yet remove @react-navigation/core because it's still a peer dependency of react-navigation-redux-helpers [3]. So we keep it, and align its version range with the one in react-navigation's dependencies. There's a long list of identifiers whose usage has changed. Searching our code for each one, these are the interesting ones: - `create*Navigator` being imported from the respective react-navigation-{tabs,stack,drawer} library; quite easy to handle - `cardStyle` (in stack nav config) being moved to `navigationOptions` (or `defaultNavigationOptions`). We deleted our only use of `cardStyle` in the recent react-navigation v3 upgrade because we only used it to make the background color white and, white became the default background color in that upgrade. Finally, attempt to upgrade libdefs for the dependencies we touched, if we import from them directly. We get new version-appropriate libdefs for react-navigation, react-navigation-drawer, and react-navigation-tabs. Unfortunately react-navigation-stack only has a v1 libdef. [1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/ [2] See https://reactnavigation.org/docs/testing/. We don't include the following line also recommended there: ``` // Silence the warning: Animated: `useNativeDriver` is not // supported because the native animated module is missing jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper'); ``` We don't get that warning in the first place, and it's not clear what mock is being applied (apart from React Native's own default one). If we need to mock `NativeAnimatedHelper` in the future, we shouldn't point to the internal path in react-native; it should go in our existing react-native mock (alongside NativeModules, etc.). [3] zulip#3804 (comment) Fixes: zulip#4248
v4 includes lots of housekeeping changes that aim to make maintenance easier [1]. In particular, we no longer grab stack, tab, or drawer navigation logic from react-navigation itself; we must now depend directly on separate libraries that provide these and import from them. - We got a head start on this in the v3 upgrade (in a recent commit) by ensuring we had react-navigation-{tabs,stack,drawer} installed at v3-compatible versions. - Now, we bump these to their latest versions (no peer-dep warnings when we do this!) and default to using carets in their version ranges since we see no indication that we can't. - This means installing react-native-reanimated, as we knew we'd need to. (We mock it in our Jest setup, following some instructions [2].) For react-navigation-stack, we add peer dependencies react-native-safe-area-context and @react-native-community/masked-view. The upgrade guide asks us to stop depending on @react-navigation/core and @react-navigation/native and change any imports of those to react-navigation imports instead. We don't have any imports to change, and we can freely remove @react-navigation/native. We can't yet remove @react-navigation/core because it's still a peer dependency of react-navigation-redux-helpers [3]. So we keep it, and align its version range with the one in react-navigation's dependencies. There's a long list of identifiers whose usage has changed. Searching our code for each one, these are the interesting ones: - `create*Navigator` being imported from the respective react-navigation-{tabs,stack,drawer} library; quite easy to handle - `cardStyle` (in stack nav config) being moved to `navigationOptions` (or `defaultNavigationOptions`). We deleted our only use of `cardStyle` in the recent react-navigation v3 upgrade because we only used it to make the background color white and, white became the default background color in that upgrade. Finally, attempt to upgrade libdefs for the dependencies we touched, if we import from them directly. We get new version-appropriate libdefs for react-navigation, react-navigation-drawer, and react-navigation-tabs. Unfortunately react-navigation-stack only has a v1 libdef. [1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/ [2] See https://reactnavigation.org/docs/testing/. We don't include the following line also recommended there: ``` // Silence the warning: Animated: `useNativeDriver` is not // supported because the native animated module is missing jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper'); ``` We don't get that warning in the first place, and it's not clear what mock is being applied (apart from React Native's own default one). If we need to mock `NativeAnimatedHelper` in the future, we shouldn't point to the internal path in react-native; it should go in our existing react-native mock (alongside NativeModules, etc.). [3] zulip#3804 (comment) Fixes: zulip#4248
The only use of this was removed in a recent commit. It's also great to be able to reduce our consumption of the nav state from Redux as we work toward zulip#3804 (decoupling nav state from Redux).
The only use of this was removed in a recent commit. It's also great to be able to reduce our consumption of the nav state from Redux as we work toward zulip#3804 (decoupling nav state from Redux).
…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`.
…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`.
…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`.
…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`.
I'm not aware that `params` will ever be null. If we thought it might be null, we would use the more transparent `| null`. Also, add a TODO to stop using a hack; we plan to do this later in the process for zulip#3804.
…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`.
I'm not aware that `params` will ever be null. If we thought it might be null, we would use the more transparent `| null`. Also, add a TODO to stop using a hack; we plan to do this later in the process for zulip#3804.
…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`.
Working toward zulip#3804. We're about to reimplement most of the functions in navSelectors.js so that they grab navigation state from the recently added `NavigationService` instead of from `state.nav` in Redux. For now, it's convenient to keep sourcing data from `state.nav` in just a few places. In this case, the one call site of `getCanGoBack` (the `connect` wrapper for `BackNavigationHandler`) is placed such that it's not convenient for it to get navigation state from `NavigationService`. In particular, `NavigationService` relies on a ref being set -- to a component that's deeper down in the component hierarchy. If we ask `NavigationService` for the navigation state, that component won't have been constructed, the ref will come up empty, and we'll have an error. `BackNavigationHandler` can disappear as zulip#3804 is completed; you can see how and why this is done in an upcoming commit.
As in the previous few commits, this is one of a small handful of places where we need to write code to explicitly get the navigation state from Redux, before we reimplement `getNav` to get the state from the recently added `NavigationService`. Like in those previous few commits, this is a place where we'll no longer need the navigation state, when zulip#3804 is completed.
Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). The various handlers will be transplanted into instructions to dispatch navigation actions at the appropriate times. These new functions will create those actions. It makes sense for them to be "reset" actions because the effect of those handlers in `navReducer` was to construct a state based on no previous state; this was ensured by passing no second argument (`lastState`) to `AppNavigator.router.getStateForAction` in our `getStateForRoute`. It also makes sense intuitively; if we're doing something like logging out, we don't want to allow back navigation. [1] https://reactnavigation.org/docs/4.x/stack-actions/#reset
Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). So, convert the `navReducer`'s `ACCOUNT_SWITCH` case into instructions to navigate to the loading screen. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition.
Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). So, convert the `navReducer`'s `LOGIN_SUCCESS` case into instructions to navigate to the loading screen. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition.
Also add a comment, with Greg's description [1] of what's going on here. We have this handy function for grabbing what we want from the navigation state, so we might as well use it. When we use this function, the navigation state is gotten from `NavigationService`, not from Redux, which moves us closer to zulip#3804. To be more rigorous, though, we wouldn't want this to stay this way in `navReduce` forever: reducers are supposed to be pure functions of state and actions. But the incorrectness isn't obviously harmful here, and we're about to transplant this code somewhere else, and it's helpful to separate a small tweak like this from the commit where we move the code. [1] zulip#4274 (comment)
Soon, we'll dismantle `navReducer`, as its purpose is incompatible with a design where we manage navigation state separately from our Redux-stored data (zulip#3804). So, convert the `navReducer`'s `INITIAL_FETCH_COMPLETE` case into instructions to navigate to the main-tabs screen, if not already there. Make the action creator into a thunk action creator and dispatch the navigation action from there, to reduce repetition.
We've been getting this value from the navigation state in Redux, via react-redux, but - We want to stop using the nav state in Redux, for zulip#3804. - `NavigationService` can give it to us, and it should be ready (the ref it uses should have been set) by the time we use it. - The value has only been used in an event handler; we haven't been taking advantage of the fact (thanks to react-redux) that `BackNavigationHandler`'s `render` and `componentDidUpdate` methods have been getting called whenever it changes.
We've long used Redux to manage our navigation state, with a helper package called react-navigation-redux-helpers. React Navigation has recommended against this for quite some time. In this commit: - Stop using r-n-r-h's `createReduxContainer`, and use React Navigation's own `createAppContainer` instead. - Rename `AppWithNavigation` (arguably better named `ReduxContainer` before this commit) to `AppContainer` - Rename `NavigationService.reduxContainerRef` to `NavigationService.appContainerRef` and change some of `NavigationService`'s implementation details to handle the new shape. - Remove use of `createReactNavigationReduxMiddleware`. - Remove `state.nav`. - Remove `navReducer`. - Adjust types as necessary. - Remove 'nav' from `discardKeys` in src/boot/store.js Fixes: zulip#3804
We stopped using this in a recent commit. Also, remove `@react-navigation/core`, as we were only including it to satisfy a peer dependency of react-navigation-redux-helpers; see f3b6c1f and zulip#3804 (comment).
We've long used Redux to manage our navigation state, with a helper package called react-navigation-redux-helpers. React Navigation has recommended against this for quite some time. In this commit: - Stop using r-n-r-h's `createReduxContainer`, and use React Navigation's own `createAppContainer` instead. - Rename `AppWithNavigation` (arguably better named `ReduxContainer` before this commit) to `AppContainer` - Rename `NavigationService.reduxContainerRef` to `NavigationService.appContainerRef` and change some of `NavigationService`'s implementation details to handle the new shape. - Remove use of `createReactNavigationReduxMiddleware`. - Remove `state.nav`. - Remove `navReducer`. - Adjust types as necessary. - Remove 'nav' from `discardKeys` in src/boot/store.js Fixes: zulip#3804
We stopped using this in a recent commit. Also, remove `@react-navigation/core`, as we were only including it to satisfy a peer dependency of react-navigation-redux-helpers; see f3b6c1f and zulip#3804 (comment).
We've been getting this value from the navigation state in Redux, via react-redux, but - We want to stop using the nav state in Redux, for zulip#3804. - `NavigationService` can give it to us, and it should be ready (the ref it uses should have been set) by the time we use it. - The value has only been used in an event handler; we haven't been taking advantage of the fact (thanks to react-redux) that `BackNavigationHandler`'s `render` and `componentDidUpdate` methods have been getting called whenever it changes.
We've long used Redux to manage our navigation state, with a helper package called react-navigation-redux-helpers. React Navigation has recommended against this for quite some time. In this commit: - Stop using r-n-r-h's `createReduxContainer`, and use React Navigation's own `createAppContainer` instead. - Rename `AppWithNavigation` (arguably better named `ReduxContainer` before this commit) to `AppContainer` - Rename `NavigationService.reduxContainerRef` to `NavigationService.appContainerRef` and change some of `NavigationService`'s implementation details to handle the new shape. - Remove use of `createReactNavigationReduxMiddleware`. - Remove `state.nav`. - Remove `navReducer`. - Adjust types as necessary. - Remove 'nav' from `discardKeys` in src/boot/store.js Fixes: zulip#3804
We stopped using this in a recent commit. Also, remove `@react-navigation/core`, as we were only including it to satisfy a peer dependency of react-navigation-redux-helpers; see f3b6c1f and zulip#3804 (comment).
We stopped using this in a recent commit. Also, remove `@react-navigation/core`, as we were only including it to satisfy a peer dependency of react-navigation-redux-helpers; see f3b6c1f and zulip#3804 (comment).
We've been getting this value from the navigation state in Redux, via react-redux, but - We want to stop using the nav state in Redux, for zulip#3804. - `NavigationService` can give it to us, and it should be ready (the ref it uses should have been set) by the time we use it. - The value has only been used in an event handler; we haven't been taking advantage of the fact (thanks to react-redux) that `BackNavigationHandler`'s `render` and `componentDidUpdate` methods have been getting called whenever it changes.
…pers. navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Help along the removal of navSelectors by removing these things that we stopped using in a recent commit.
As mentioned in a previous commit, navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Might as well remove navSelectors.
…pers. navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Help along the removal of navSelectors by removing these things that we stopped using in a recent commit.
As mentioned in a previous commit, navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Might as well remove navSelectors.
…pers. navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Help along the removal of navSelectors by removing these things that we stopped using in a recent commit.
As mentioned in a previous commit, navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Might as well remove navSelectors.
…pers. navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Help along the removal of navSelectors by removing these things that we stopped using in a recent commit.
As mentioned in a previous commit, navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Might as well remove navSelectors.
…pers. navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Help along the removal of navSelectors by removing these things that we stopped using in a recent commit.
As mentioned in a previous commit, navSelectors and navActions are kind of relics from before zulip#3804, and we already have zulip#4417 for removing most or all of navActions. Might as well remove navSelectors.
This won't be a priority until we're closer to upgrading to React Navigation v5 (which currently hasn't been released yet, and we're on v2 now), but React Navigation will eventually stop supporting the pattern of storing navigation state in Redux. Here are the indications:
See the discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/redux.20.2F.20navigation/near/808659.
In the meantime, this issue is a place to document what this coupling currently provides for us, so we don't forget about it when we eventually have to make the change. Since this pattern works for us, currently, incremental changes will be favored over large, dramatic ones.
The text was updated successfully, but these errors were encountered: