React Navigation v5 upgrade (part 2) #4393
Conversation
|
I'm not sure this PR is the place to make any changes to address it, but one thing that'll be good to keep in mind is that, while the
|
There was a problem hiding this comment.
Thanks @chrisbobbe ! There was a lot of complexity that had to be handled in this project -- glad to see it nearing completion.
Generally this looks great. Some comments below.
src/nav/NavigationService.js
Outdated
| // https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/decouple.20nav.20from.20redux.20%28.23M3804%29/near/1056167 | ||
| // $FlowFixMe | ||
| getContainer().state.nav; | ||
| export const getState = (): PossiblyStaleNavigationState => getContainer().getRootState(); |
There was a problem hiding this comment.
What's this "possibly stale" naming about? It seems like it's trying to warn us about some kind of gotcha.
There was a problem hiding this comment.
Yikes, yeah—responding in chat.
src/common/SmartUrlInput.js
Outdated
| // TODO: Currently this type is acceptable because the only | ||
| // `navigation` prop we pass to a `SmartUrlInput` instance is the | ||
| // one from the AppNavigator that's given to the 'realm'-route | ||
| // component. | ||
| navigation: AppNavigationProp<'realm'>, |
There was a problem hiding this comment.
Hmm, awkward. Can we just drop the mention of the specific route here? That type parameter has a default of "it could be any of them"...
ah, aha, it needs a few improvements to the libdef too. Here's a diff that does that and passes Flow:
diff --git a/flow-typed/npm/@react-navigation/stack_v5.x.x.js b/flow-typed/npm/@react-navigation/stack_v5.x.x.js
index 1679a2da0..15ac6055b 100644
--- a/flow-typed/npm/@react-navigation/stack_v5.x.x.js
+++ b/flow-typed/npm/@react-navigation/stack_v5.x.x.js
@@ -831,5 +831,5 @@ declare module '@react-navigation/stack' {
declare export type NavigationProp<
ParamList: ParamListBase,
- RouteName: $Keys<ParamList> = $Keys<ParamList>,
+ +RouteName: $Keys<ParamList> = $Keys<ParamList>,
State: PossiblyStaleNavigationState = PossiblyStaleNavigationState,
ScreenOptions: {...} = {...},
@@ -1262,5 +1262,5 @@ declare module '@react-navigation/stack' {
declare type InexactStackNavigationProp<
ParamList: ParamListBase = ParamListBase,
- RouteName: $Keys<ParamList> = $Keys<ParamList>,
+ +RouteName: $Keys<ParamList> = $Keys<ParamList>,
Options: {...} = StackOptions,
EventMap: EventMapBase = StackNavigationEventMap,
@@ -1282,5 +1282,5 @@ declare module '@react-navigation/stack' {
declare export type StackNavigationProp<
ParamList: ParamListBase = ParamListBase,
- RouteName: $Keys<ParamList> = $Keys<ParamList>,
+ +RouteName: $Keys<ParamList> = $Keys<ParamList>,
Options: {...} = StackOptions,
EventMap: EventMapBase = StackNavigationEventMap,
diff --git a/src/common/SmartUrlInput.js b/src/common/SmartUrlInput.js
index f74f7d285..9ea146ff9 100644
--- a/src/common/SmartUrlInput.js
+++ b/src/common/SmartUrlInput.js
@@ -50,5 +50,5 @@ type Props = $ReadOnly<{|
// one from the AppNavigator that's given to the 'realm'-route
// component.
- navigation: AppNavigationProp<'realm'>,
+ navigation: AppNavigationProp<>,
style?: ViewStyleProp,
onChangeText: (value: string) => void,
diff --git a/src/nav/AppNavigator.js b/src/nav/AppNavigator.js
index 67e336a1b..459414ab7 100644
--- a/src/nav/AppNavigator.js
+++ b/src/nav/AppNavigator.js
@@ -84,5 +84,5 @@ export type AppNavigatorParamList = {|
export type AppNavigationProp<
- RouteName: $Keys<AppNavigatorParamList> = $Keys<AppNavigatorParamList>,
+ +RouteName: $Keys<AppNavigatorParamList> = $Keys<AppNavigatorParamList>,
> = StackNavigationProp<GlobalParamList, RouteName>;
Those + markings at the definitions of the type parameters are "variance sigils" -- https://flow.org/en/docs/types/generics/#toc-variance-sigils -- indicating that e.g. AppNavigationProp is covariant in its parameter RouteName. That means that AppNavigationProp of a subtype of T is a subtype of AppNavigationProp of T... in particular, that AppNavigationProp<'realm'>, which is what the caller has there, is a subtype of AppNavigationProp<'realm' | 'account' | …>, which is what the diff makes the callee expect.
src/styles/tabs.js
Outdated
| // TODO: `upperCaseLabel` vanished in react-navigation v5; we | ||
| // used to use `false` for this, but it appears that the | ||
| // effective default value is `true`: | ||
| // https://github.com/react-navigation/react-navigation/issues/7952 |
There was a problem hiding this comment.
Hmm! Does this have a visible effect?
I'd expect it to cause the tab labels in e.g. StreamTabs to get shown in uppercase.
There was a problem hiding this comment.
Ah OK, I've figured out why this doesn't have a visible effect.
It would have had a visible effect if we had used a string instead of a React element for tabBarLabel, or if we hadn't specified tabBarLabel and defined title.
By passing a React element, we get to say exactly how the tab bar label should look, and that includes case.
There was a problem hiding this comment.
That might mean I can delete this TODO, but I have a feeling we'll run up against it again if we ever stop using our strategy here, or we make a material top tab nav somewhere else that doesn't use that strategy.
There was a problem hiding this comment.
Hmm interesting!
Yeah, I think that means the TODO still belongs here. It'd be good to also add a comment with that information you just learned about when it would start affecting things.
There was a problem hiding this comment.
Oh, hmm. With these findings in mind, I've just found a place where I'd expect to see a change: The bottom tab nav in MainTabs shows tab-bar labels, which we define with strings like "Home", "Streams", etc., if !!Platform.isPad. On an iPad simulator, I do indeed see those labels—but I don't see them in all caps.
I suspect the all-caps quirk applies to the "material top tab" navigator, but doesn't apply to the "bottom" tab navigator. I wouldn't be totally surprised to see differences spring up between them; the React Nav v5 upgrade took us from depending on react-navigation-tabs to @react-navigation/material-top-tabs and @react-navigation/bottom-tabs.
There was a problem hiding this comment.
Yeah, I think that's an intentional design choice.
Or more specifically, it's a faithful implementation (at least on that point) of Material Design, which those top tabs are advertised as being for. Material calls those top tabs simply "tabs", and they are in caps:
https://material.io/components/tabs
For our bottom tabs, Material's term is "bottom navigation":
https://material.io/components/bottom-navigation
and the titles are not in caps.
Meanwhile, Apple says "tab bar" for something very much like our bottom tabs, or Material's bottom navigation:
https://developer.apple.com/design/human-interface-guidelines/ios/bars/tab-bars/
and those also aren't in caps. I'm not sure there's any equivalent to top tabs, or Material tabs, in Apple's iOS guidelines.
Anyway, I think that means that we shouldn't expect the upper-case thing to just randomly change on us in the future due to something intended as a bugfix.
|
|
||
| export const navigateToUsersScreen = (): NavigationAction => | ||
| StackActions.push({ routeName: 'users' }); | ||
| export const navigateToUsersScreen = (): GenericNavigationAction => StackActions.push('users'); |
There was a problem hiding this comment.
They do note (as they do on their "navigating without the navigation
prop" doc [3]) that it's best to use the `navigation` object when
one can:
"""
It's highly recommended to use the methods on the navigation object
instead of using action creators and `dispatch`. It should only be
used for advanced use cases.
"""
Interesting. Seems like migrating to that would be a good followup commit, then (not necessarily within this PR.)
I think probably the bulk of these navActions actions are only ever invoked from within a React component. Perhaps most of them even from a React component that already has a navigation prop, and there's useNavigation for a convenient way to handle some of the others. So that might let us eliminate most of these.
And when we do, it looks like we'd get proper type-checking! When I try this.props.navigation.navigate() in a random component and start entering arguments, it looks like I get Flow telling me whether I have a proper route name and then whether I have the right arguments for that route. That's pretty nifty.
There was a problem hiding this comment.
And when we do, it looks like we'd get proper type-checking! When I try
this.props.navigation.navigate()in a random component and start entering arguments, it looks like I get Flow telling me whether I have a proper route name and then whether I have the right arguments for that route. That's pretty nifty.
Nice! Happy to do that as a followup.
|
|
||
| const dispatch = useDispatch(); | ||
| const isFocused = useIsFocused(); |
There was a problem hiding this comment.
src/chat/ChatScreen.js | 25 ++++++-------------------
1 file changed, 6 insertions(+), 19 deletions(-)
Very nice!
src/nav/AppNavigator.js
Outdated
| type SelectorProps = $ReadOnly<{| | ||
| hasAuth: boolean, | ||
| accounts: Account[], | ||
| haveServerData: boolean, |
There was a problem hiding this comment.
I think this means that the caller, ZulipNavigationContainer, can stop getting these three from Redux. It'd be good to pair the deletion with the addition, so that the diff is visibly just moving the logic.
I think this is also logically more or less independent of the change in the nav API, so it'd simplify things to make it a separate diff. (This commit in particular has a large quantity of boring mechanical stuff happening, so it'd be good to keep more-interesting changes out of it.) I think it could pretty well happen either before or after this one -- either createAppNavigator starts taking these instead of its old arguments and calling getInitialRouteInfo itself, or AppNavigator at first still takes the old arguments and later starts connecting to Redux for itself.
There was a problem hiding this comment.
or
AppNavigatorat first still takes the old arguments and later starts connecting to Redux for itself.
OK! I've taken this approach, with a new commit at the tip.
…param. When we make our own types based on `StackNavigationProp` -- e.g., when we define `AppNavigationProp`, in an upcoming commit -- we'll want those types to be covariant in their own `RouteName` param. That's because we want, e.g., `AppNavigationProp<'realm'>` to be recognized as a subtype of `AppNavigationProp<'realm' | 'account' | …>`, i.e., `AppNavigationProp<$Keys<AppNavigatorParamList>>`. So, start by making `NavigationProp`, `StackNavigationProp`, and `InexactStackNavigationProp` covariant in their `RouteName` params, following discussion at zulip#4393 (comment). Also make the change for types of navigators other than stack navigators, across all the libdef files. There's a lot of repetition because libdef files can't import types from other files [1]. [1] zulip#3458 (comment)
chrisbobbe
force-pushed
the
pr-react-navigation-5-part-2
branch
from
5ca95b0
to
9ba426f
Compare
|
I've just pushed a revision—but I just remembered that it doesn't address the results of the discussion about |
…param. When we make our own types based on `StackNavigationProp` -- e.g., when we define `AppNavigationProp`, in an upcoming commit -- we'll want those types to be covariant in their own `RouteName` param. That's because we want, e.g., `AppNavigationProp<'realm'>` to be recognized as a subtype of `AppNavigationProp<'realm' | 'account' | …>`, i.e., `AppNavigationProp<$Keys<AppNavigatorParamList>>`. So, start by making `NavigationProp`, `StackNavigationProp`, and `InexactStackNavigationProp` covariant in their `RouteName` params, following discussion at zulip#4393 (comment). Also make the change for types of navigators other than stack navigators, across all the libdef files. There's a lot of repetition because libdef files can't import types from other files [1]. [1] zulip#3458 (comment)
chrisbobbe
force-pushed
the
pr-react-navigation-5-part-2
branch
from
9ba426f
to
5a9ef21
Compare
OK, revision pushed! |
| declare type AccessibilityActionEvent = SyntheticEvent< | ||
| $ReadOnly<{actionName: string, ...}>, | ||
| >; | ||
| declare type AccessibilityActionEvent = SyntheticEvent<$ReadOnly<{ actionName: string, ... }>>; |
There was a problem hiding this comment.
react-navigation libdef [nfc]: Format files.
I think it'd be better to leave them in the format we found them in. The reason is that ideally we'd be contributing these fixes back upstream to the flow-typed repo, and preserving the format will help keep it smooth for us to do so.
So probably that means telling Prettier and ESLint to ignore these files.
src/styles/tabs.js
Outdated
| // TODO: `upperCaseLabel` vanished in react-navigation v5; we | ||
| // used to use `false` for this, but it appears that the | ||
| // effective default value is `true`: | ||
| // https://github.com/react-navigation/react-navigation/issues/7952 |
There was a problem hiding this comment.
Hmm interesting!
Yeah, I think that means the TODO still belongs here. It'd be good to also add a comment with that information you just learned about when it would start affecting things.
|
Thanks for the revision! All looks great except the two comments just above (one of them quite small.) |
…ator. This is NFC because there are no changes to our effective type annotations -- we just restructure our code to be more like the new way that's documented [1] for React Navigation v5: alongside each navigator, have a "param list" type (here, `AppNavigatorParamList`) that maps a route name to that route's params and a generic helper type (here, `AppNavigationProp`) that consults it, then have each screen use that helper to type its `navigation` prop. As noted in a recent commit where we added src/nav/globalTypes, we might switch to a different structure after the upgrade is complete, but we're sticking with the docs for now. [1] https://reactnavigation.org/docs/typescript
Like the previous commit, but for `MainTabs`.
…abs`. Like the previous commits, but for `StreamTabs`.
…creen. Like the previous commits, but for `SharingScreen`.
The issue is illustrated in a screenshot at react-navigation/react-navigation#7299 (comment). That's also where I got the idea to set `elevation` to 0.
We were scrupulous about not passing `initialRouteName` at all, in some cases, but it turns out that passing `undefined` in those cases is fine. See discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20MessageReactionList/near/1058099.
…ons. This will make the upcoming React Navigation v5 upgrade diff smaller. See discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20MessageReactionList/near/1058076.
Both React Navigation v4 and v5 have interfaces for setting the theme between light and dark, but they're quite different. One kind of unfortunate difference is that, in v5, using that interface is effectively required, even though we have a so-far quite adequate way of managing the theme. React Navigation v5 widely applies some styling overrides to its provided components (such as the background of a scene in a bottom-tab navigator) even when we don't pass a `theme` prop. I'm not sure if it applies them based on its own arbitrary default, or if it's listening to the system-wide theme setting. But we're left with a choice between papering over each of those overridden styles, or taking up their theme interface and doing something with it. We take the latter approach because it's not piecemeal. (It would be tricky to find all the forced styles we'd need to override, and it's not obvious that we'll always be able to paper over them. One such fix is to set a `backgroundColor` within `sceneContainerStyle` in `MainTabs` -- but this wouldn't have been possible even a few months ago, before @react-navigation/bottom-tabs@5.10.0.) To smoothen the change toward using the `theme` prop in v5, start doing so now, while we're still on v4. Theme support is much more rudimentary in v4 (i.e., the `theme` prop here just takes "dark" or "light"), but I don't see anything particularly disruptive by starting to use it now [1], so, might as well. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20theme.20colors/near/1061811
When we're done upgrading to React Navigation 5, including the removal of @react-navigation/compat (which is completely untyped and has no existing libdef) comments like these will be unnecessary. Currently, our navigators don't check that their attached screens are annotated with the correct shape of the `navigation` prop -- so we've just been saying they are, in these comments. We'll get those checks when the upgrade is complete. We remove them now to reduce the size of the diffs in the upcoming commits.
We can change out the libdefs for the React Navigation v5 upgrade outside the main upgrade commit because the packages changed names. In v5, they're all under the `@react-navigation` scope. So, add the new ones, and plan to clean out the old ones after the upcoming main upgrade commit.
As Greg points out [1], it's best to smoothen the path to making a PR to the FlowTyped repo with the changes we're about to make to this libdef. That means not converting it to our own style, which means telling Prettier and ESLint to ignore these files. ESLint already ignores them because of the `**/__flow-tests__/**` line in `.eslintignore`. [1] zulip#4393 (comment)
…param. When we make our own types based on `StackNavigationProp` -- e.g., when we define `AppNavigationProp`, in an upcoming commit -- we'll want those types to be covariant in their own `RouteName` param. That's because we want, e.g., `AppNavigationProp<'realm'>` to be recognized as a subtype of `AppNavigationProp<'realm' | 'account' | …>`, i.e., `AppNavigationProp<$Keys<AppNavigatorParamList>>`. So, start by making `NavigationProp`, `StackNavigationProp`, and `InexactStackNavigationProp` covariant in their `RouteName` params, following discussion at zulip#4393 (comment). Also make the change for types of navigators other than stack navigators, across all the libdef files. There's a lot of repetition because libdef files can't import types from other files [1]. [1] zulip#3458 (comment)
Done mostly with the upgrade guide at https://reactnavigation.org/docs/upgrading-from-4.x, but also by consulting the v4 and v5 documentation of specific areas. In this commit: - Take the packages' name changes (e.g., react-navigation -> @react-navigation/native) - Add react-native-tab-view to handle a peer-dep warning: warning " > @react-navigation/material-top-tabs@5.2.19" has unmet peer dependency "react-native-tab-view@>= 2.0.0". - Run the usual `yarn yarn-deduplicate && yarn`. - Use the newly available `NavigationContainer` instead of the removed `createAppContainer`. We leave it in its own file (src/nav/Zulip { App -> Navigation } Container.js), since it still has to handle some special logic: - setting up the NavigationService with a ref (the initialization gets slightly more complicated, with the `onReady` prop) - deciding what to pass for the `theme` prop, based on the current theme (for more discussion of our use of this prop, see the recent commit where we started passing it) - Update `NavigationService` to handle the new shape of the thing it tracks with a ref (`AppContainer` -> `NavigationContainer`), and handle some new initialization logic for `NavigationContainer` (its `onReady` prop). - Change `MessageReactionList` over to the new component-based API [1], without going through @react-navigation/compat first. (It's small and self-contained.) - Undo f4b8253, i.e., stop threading through `navigation` prop and assigning to `static router` in MainScreenWithTabs. The "common mistakes" doc that prescribed those things has been removed in v5, and, empirically, everything works fine without it. - Use @react-navigation/compat to reduce the size of this commit (we'll incrementally move away from it in the upcoming commits). (Note that that package is completely untyped [2] -- this causes Flow to misleadingly mark some `$FlowFixMe`s unnecessary; still, we go along with Flow and remove them -- but once we've completely adopted the v5 APIs, our types should be stronger than before.) There are a few things we use the compat package for [3]: - Postpone changes to the definitions of our four navigators: `AppNavigator`, `MainTabs`, `StreamTabs`, and `SharingScreen`, with `createCompatNavigatorFactory`. - In `navActions`, postpone taking the new interfaces of various action creators (`StackActions.push`, etc.). This isn't possible with `.reset` actions, but we only use three of those, so we change them in this commit. - Keep using the `withNavigationFocus` HOC, so we can switch to the new custom Hook later. And a few minor naming / simple API tweaks: - In `SmartUrlInput`, use the 'focus' event type instead of the now-removed one, 'didFocus', and adapt to the new API for removing a listener. - What used to be called `routeName` on a route in the navigation state is now called `name`. - Even though we're using `createCompatNavigatorFactory`, we have to access `tintColor` instead of `color` on the object passed to the function we pass to `tabBarLabel` and `tabBarIcon` in a route config on a tab navigator. - Various types are renamed, like `NavigationParams` -> `ScreenParams`. - Apparently `upperCaseLabel` in a tab navigator's config...vanished in this new version. Add a TODO with findings from a GitHub discussion about this [4]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20MessageReactionList/near/1061924 [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5/near/1046571 [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20compat/near/1058109 [4] zulip#4393 (comment)
chrisbobbe
force-pushed
the
pr-react-navigation-5-part-2
branch
from
5a9ef21
to
4bb0346
Compare
|
Thanks for the review! Revision pushed. I posted an additional finding, about the |
There was a problem hiding this comment.
Thanks for the revision! Looks great -- two nits below, and I replied about upper-case in tab labels above: #4393 (comment) Please merge at will.
| # FlowTyped, to make it easier to eventually make a PR back to | ||
| # FlowTyped. |
There was a problem hiding this comment.
ESLint already ignores them because of the `**/__flow-tests__/**`
line in `.eslintignore`.
nit: I think you mean the other line, **/flow-typed/**
.prettierignore
Outdated
| # FlowTyped, to make it easier to eventually make a PR back to | ||
| # FlowTyped. | ||
| flow-typed/@react-navigation |
There was a problem hiding this comment.
nit: last line should end with a newline, like other lines
(Git will display this like so:
+flow-typed/@react-navigation
\ No newline at end of file)
There was a problem hiding this comment.
Ah, yeah, thanks for the catch. This is one of those things that I know we're supposed to do, but that I don't usually think about—probably because our auto-formatting takes care of it in most files.
As Greg points out [1], it's best to smoothen the path to making a PR to the FlowTyped repo with the changes we're about to make to this libdef. That means not converting it to our own style, which means telling Prettier and ESLint to ignore these files. ESLint already ignores them because of the `**/flow-typed/**` line in `.eslintignore`. [1] zulip#4393 (comment)
…param. When we make our own types based on `StackNavigationProp` -- e.g., when we define `AppNavigationProp`, in an upcoming commit -- we'll want those types to be covariant in their own `RouteName` param. That's because we want, e.g., `AppNavigationProp<'realm'>` to be recognized as a subtype of `AppNavigationProp<'realm' | 'account' | …>`, i.e., `AppNavigationProp<$Keys<AppNavigatorParamList>>`. So, start by making `NavigationProp`, `StackNavigationProp`, and `InexactStackNavigationProp` covariant in their `RouteName` params, following discussion at zulip#4393 (comment). Also make the change for types of navigators other than stack navigators, across all the libdef files. There's a lot of repetition because libdef files can't import types from other files [1]. [1] zulip#3458 (comment)
It seems that `PossiblyStaleNavigtionState` [1] is the Flow libdef's version of what appears in the TypeScript as `PartialState<NavigationState>` [2]. In contrast with the Flow libdef, the TypeScript has `getRootState` returning the non-"partial", non-"possibly stale" thing, i.e., `NavigationState` [3]. That suggests we should consider making the Flow libdef say the same thing, which also has the name `NavigationState`. React Navigation does have a doc on "Partial state objects" [4]. Based on that, Greg points out [5] that a "stale" nav state is accepted as input to various React Navigation options, but a non-stale state is always the output -- suggesting that that the return value of something like `getRootState` should surely not be stale. Some logging confirms that there aren't prominent cases where `stale` is `true` or missing. (Any such cases would suggest that we should keep the `PossiblyStaleNavigationState` return type.) See discussion [6]. [1] https://github.com/flow-typed/flow-typed/blob/master/definitions/npm/%40react-navigation/native_v5.x.x/flow_v0.104.x-/native_v5.x.x.js#L526 [2] https://github.com/react-navigation/react-navigation/blob/main/packages/routers/src/types.tsx#L58 [3] https://github.com/react-navigation/react-navigation/blob/main/packages/core/src/types.tsx#L471 [4] https://reactnavigation.org/docs/navigation-state/#partial-state-objects [5] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20.60PossiblyStaleNavigationState.60/near/1097141 [6] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20.60PossiblyStaleNavigationState.60/near/1096924
Done mostly with the upgrade guide at https://reactnavigation.org/docs/upgrading-from-4.x, but also by consulting the v4 and v5 documentation of specific areas. In this commit: - Take the packages' name changes (e.g., react-navigation -> @react-navigation/native) - Add react-native-tab-view to handle a peer-dep warning: warning " > @react-navigation/material-top-tabs@5.2.19" has unmet peer dependency "react-native-tab-view@>= 2.0.0". - Run the usual `yarn yarn-deduplicate && yarn`. - Use the newly available `NavigationContainer` instead of the removed `createAppContainer`. We leave it in its own file (src/nav/Zulip { App -> Navigation } Container.js), since it still has to handle some special logic: - setting up the NavigationService with a ref (the initialization gets slightly more complicated, with the `onReady` prop) - deciding what to pass for the `theme` prop, based on the current theme (for more discussion of our use of this prop, see the recent commit where we started passing it) - Update `NavigationService` to handle the new shape of the thing it tracks with a ref (`AppContainer` -> `NavigationContainer`), and handle some new initialization logic for `NavigationContainer` (its `onReady` prop). - Change `MessageReactionList` over to the new component-based API [1], without going through @react-navigation/compat first. (It's small and self-contained.) - Undo f4b8253, i.e., stop threading through `navigation` prop and assigning to `static router` in MainScreenWithTabs. The "common mistakes" doc that prescribed those things has been removed in v5, and, empirically, everything works fine without it. - Use @react-navigation/compat to reduce the size of this commit (we'll incrementally move away from it in the upcoming commits). (Note that that package is completely untyped [2] -- this causes Flow to misleadingly mark some `$FlowFixMe`s unnecessary; still, we go along with Flow and remove them -- but once we've completely adopted the v5 APIs, our types should be stronger than before.) There are a few things we use the compat package for [3]: - Postpone changes to the definitions of our four navigators: `AppNavigator`, `MainTabs`, `StreamTabs`, and `SharingScreen`, with `createCompatNavigatorFactory`. - In `navActions`, postpone taking the new interfaces of various action creators (`StackActions.push`, etc.). This isn't possible with `.reset` actions, but we only use three of those, so we change them in this commit. - Keep using the `withNavigationFocus` HOC, so we can switch to the new custom Hook later. And a few minor naming / simple API tweaks: - In `SmartUrlInput`, use the 'focus' event type instead of the now-removed one, 'didFocus', and adapt to the new API for removing a listener. - What used to be called `routeName` on a route in the navigation state is now called `name`. - Even though we're using `createCompatNavigatorFactory`, we have to access `tintColor` instead of `color` on the object passed to the function we pass to `tabBarLabel` and `tabBarIcon` in a route config on a tab navigator. - Various types are renamed, like `NavigationParams` -> `ScreenParams`. - Apparently `upperCaseLabel` in a tab navigator's config...vanished in this new version. Add a TODO with findings from a GitHub discussion about this [4]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20MessageReactionList/near/1061924 [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5/near/1046571 [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20compat/near/1058109 [4] zulip#4393 (comment)
We're free to do this, since the upgrade to the differently named packages for React Navigation v5 has just been completed.
As discussed around https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20MessageReactionList/near/1058097. Also, as planned, delete a helper function we just don't use anymore.
The compatability layer [1] has eased our migration to React Navigation v5. Now, remove our use of it here by adapting to the new APIs. The ugprade guide [2] describes the changes. There are a lot of changes to the interface, but it does say the following: """ In addition, there have been some changes to the way the navigation actions work. These changes probably won't affect you if you didn't do any advanced tasks with these methods. """ I don't know where they put the threshold for "advanced", but I think we should be fine. They do note (as they do on their "navigating without the navigation prop" doc [3]) that it's best to use the `navigation` object when one can: """ It's highly recommended to use the methods on the navigation object instead of using action creators and `dispatch`. It should only be used for advanced use cases. """ But I haven't noticed any major disruptions so far. [1] https://reactnavigation.org/docs/compatibility [2] https://reactnavigation.org/docs/upgrading-from-4.x/#action-creators [3] https://reactnavigation.org/docs/navigating-without-navigation-prop
The `withNavigationFocus` HOC was dropped in v5. Also in v5, they introduced a `useIsFocused` Hook. But, as Greg points out [1], they've mitigated the disruption of this sudden change by supplying `withNavigationFocus` in the compat package, which we've been using. We made the switch to the new API easier, in a recent commit, by changing `ChatScreen` from a class component to a function component that uses React Hooks. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20compat/near/1058117
And switch over to the React Navigation v5 component-based API [1] for it. For all screens on `MainTabs`, that means the `navigation` prop gets a new shape, and the `route` prop is introduced. Also, where we invoke the screens to attach them to the navigator, the screens' `navigation` and `route` prop type annotations are checked for correctness. Since we newly define the `MainTabs` component ourselves, make it a Hooks-based component, as that seems to be the way of the future. [1] https://reactnavigation.org/docs/upgrading-from-4.x/#configuring-the-navigator
Like the previous commit, but for `AppNavigator`.
Like the previous commits, but for `SharingScreen`.
Like the previous commits, but for `StreamTabs`.
We're finished upgrading to React Navigation 5, so we can drop this. Fixes: zulip#4296
Instead, have AppNavigator grab them itself, with react-redux.
chrisbobbe
force-pushed
the
pr-react-navigation-5-part-2
branch
from
4bb0346
to
dd6ef39
Compare
Done, after fixing those nits and expanding a bit on the code comment about |
This is the next PR toward fixing #4296, the React Navigation v5 upgrade, and it includes making the upgrade itself! 🎉 It follows (from earliest to latest) the resolution of #3804 in #4273, #4274, #4278; #4315; and (merged last Friday) #4300.
It starts with some more prep commits for the main upgrade commit, to reduce the size of its diff and to address some quirky differences between v4 and v5. In the main upgrade commit, we drop in @react-navigation/compat to further reduce the size of the diff, but we promptly stop using @react-navigation/compat over several commits (rather than leaving it in semi-indefinitely) because it's completely untyped.
Tested on my physical iPhone 6s, an iPhone 12 Pro simulator, and the office Samsung Galaxy S9.
Fixes: #4296