Decouple Navigation from Redux (Part 2) #4278
Conversation
chrisbobbe
added
a-redux
blocked on other work
chrisbobbe
force-pushed
the
pr-decouple-nav-2
branch
4 times, most recently
from
b752775 to
ce47ee9
Compare
chrisbobbe
force-pushed
the
pr-decouple-nav-2
branch
2 times, most recently
from
3650acc to
b53278b
Compare
chrisbobbe
removed
the
blocked on other work
src/ZulipMobile.js
Outdated
| // `static navigationOptions` and `static router` not | ||
| // being handled properly | ||
| // $FlowFixMe | ||
| ref={NavigationService.appContainerRef} |
There was a problem hiding this comment.
Hmm interesting. Here's the error message I get when I remove the fixme:
Cannot create AppContainer element because:
• Either mixed [1] is incompatible with React.ComponentType [2] in property
current of property ref.
• Or a call signature declaring the expected parameter / return type is missing
in object type [3] but exists in function type [4] in property ref.
• Or withRouter [5] is not a React component.
• Or withOptionalNavigationOptions [6] is not a React component.
src/ZulipMobile.js
29│ <TranslationProvider>
30│ <ThemeProvider>
31│ <InitialNavigationDispatcher>
32│ <AppContainer
33│ // `static navigationOptions` and `static router` not
34│ // being handled properly
35│ ref={NavigationService.appContainerRef}
36│ />
37│ </InitialNavigationDispatcher>
38│ </ThemeProvider>
39│ </TranslationProvider>
flow-typed/npm/react-navigation_v4.x.x.js
[2] 413│ > = React$ComponentType<Props> &
[5] 414│ withRouter<State, Options> &
[6] 415│ withOptionalNavigationOptions<Options>;
/tmp/flow/flowlib_1bc53cbd/react.js
[1] 159│ declare type React$ComponentType<-Config> = React$AbstractComponent<Config, mixed>;
:
[4] 197│ | ((React$ElementRef<ElementType> | null) => mixed)
:
[3] 246│ ): {|current: null | T|};
I tried removing those statics from the NavigationContainer definition in the libdef:
--- a/flow-typed/npm/react-navigation_v4.x.x.js
+++ b/flow-typed/npm/react-navigation_v4.x.x.js
@@ -410,9 +410,9 @@ declare module 'react-navigation' {
State: NavigationState,
Options: {...},
Props: NavigationContainerProps<Options, State>,
- > = React$ComponentType<Props> &
+ > = React$ComponentType<Props> /* &
withRouter<State, Options> &
- withOptionalNavigationOptions<Options>;
+ withOptionalNavigationOptions<Options> */;That eliminated the last two of those four errors, but the first two remain. So I think the issue is more basic.
... OK, on some experimentation, I think I see the cause! It looks like the type parameter to React.createRef should be a React$ElementRef type:
const appContainerRef = React.createRef<
- NavigationContainer<NavigationState, { ... }, NavigationContainerProps<{ ... }, NavigationState>>,
+ React$ElementRef<
+ NavigationContainer<NavigationState, { ... }, NavigationContainerProps<{ ... }, NavigationState>>,
+ >,
>();That resolves this fixme, and also the one on appContainerRef.current.dispatch.
I got there by looking at the type definitions mentioned in that Flow error message, and in particular looking closely at the createRef declaration:
declare export function createRef<T>(
): {|current: null | T|};and comparing with this one in the same file (and also mentioned in the error message):
/**
* The type of the ref prop available on all React components.
*/
declare type React$Ref<ElementType: React$ElementType> =
| { -current: React$ElementRef<ElementType> | null, ... }
| ((React$ElementRef<ElementType> | null) => mixed)
| number | string;Note the types of the current property.
I'll push a commit with that change to the tip of this PR branch.
In the course of finding that, I also discovered some small bugs in the libdef and other small ways it can be improved. I pushed those fixes to another branch, though they turn out not to be necessary here.
| const appContainerRef = React.createRef< | ||
| NavigationContainer<NavigationState, { ... }, NavigationContainerProps<{ ... }, NavigationState>>, | ||
| React$ElementRef< |
There was a problem hiding this comment.
Amazing!! It sounds like we've finally found what React$ElementRef is for—thanks! See, e.g., 333c312 and its parent c1f9d15; the latter links to #4101 (comment) (in which I used React$ElementRef in very much the wrong way).
There was a problem hiding this comment.
Hmmmm, I'm actually getting an error when I remove the line
// $FlowFixMe - how to tell Flow about `.state`?
(below this diff chunk; it seems GitHub won't let me add a comment to a line this far away from a diff chunk)
There was a problem hiding this comment.
Looks like it's on the thing we're trying to pass as a type argument to React.createRef, and it looks familiar:
Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/nav/NavigationService.js:12:3
Cannot instantiate React.ElementRef because:
• Either withRouter [1] is not a React component.
• Or withOptionalNavigationOptions [2] is not a React component.
src/nav/NavigationService.js
9│
10│ /* prettier-ignore */
11│ const appContainerRef = React.createRef<
12│ React$ElementRef<
13│ NavigationContainer<
14│ NavigationState,
15│ { ... },
16│ NavigationContainerProps<{ ... }, NavigationState>>>
17│ >();
18│
19│ const getState = (): NavigationState => {
flow-typed/npm/react-navigation_v4.x.x.js
[1] 414│ withRouter<State, Options> &
[2] 415│ withOptionalNavigationOptions<Options>;
There was a problem hiding this comment.
@gnprice, could you please check and see if you get that error too? I was expecting to be able to remove this FlowFixMe just like this WIP commit removes this other fixme:
// $FlowFixMe - how to tell Flow about this method?
There was a problem hiding this comment.
(We discussed this in chat.) It seems like the React$ElementRef part is correct, and there's something else going on, probably in the react-navigation libdef. For now the right strategy is probably to type the ref the way we now think is right, and just include whatever fixmes we need to, like this one.
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.
|
I just pushed a new revision; please take a look, @gnprice. 🙂 It's not mergeable yet—please see my comments at #4278 (comment). |
BackNavigationHandler is no longer necessary to support `react-navigation-redux-helpers`, which we recently removed, but it does this other important thing that's easy to forget about. So, make a note. As Greg says [1], we may want to change the behavior as part of a UI redesign; it's not particularly intuitive. [1] zulip#4278 (comment)
chrisbobbe
force-pushed
the
pr-decouple-nav-2
branch
from
d292bef to
36dac0e
Compare
|
LGTM! One commit-message nit: I'd s/navActions:/nav:/ in those three commits toward the beginning of the branch. They're about how we navigate; and they're not particularly specific to the Please merge at will after adjusting that fixme discussed above. |
Instead, dispatch it via our recently added `NavigationService`.
Instead, dispatch it via our recently added `NavigationService`.
Instead, dispatch them via our recently added `NavigationService`. Add a few notes in `InitialNavigationDispatcher` about why we expect `NavigationService` to be ready in time. We chose to do the initial navigation here, rather than in a descendant of the `NavigationService`'s `ref`fed component for those reasons [1]. [1] zulip#4274 (comment)
…atch`. In the previous commit, we stopped dispatching navigation actions through Redux.
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. Leave a $FlowFixMe that's been a bit mystifying, but is
probably caused by an oddness in the libdef.
- 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).
BackNavigationHandler is no longer necessary to support `react-navigation-redux-helpers`, which we recently removed, but it does this other important thing that's easy to forget about. So, make a note. As Greg says [1], we may want to change the behavior as part of a UI redesign; it's not particularly intuitive. [1] zulip#4278 (comment)
chrisbobbe
force-pushed
the
pr-decouple-nav-2
branch
from
36dac0e to
d8ecd9b
Compare
Done, thanks for the review! |
BackNavigationHandler is no longer necessary to support `react-navigation-redux-helpers`, which we recently removed, but it does this other important thing that's easy to forget about. So, make a note. As Greg says [1], we may want to change the behavior as part of a UI redesign; it's not particularly intuitive. [1] zulip#4278 (comment)
…eter. As Greg pointed out it should be, a few months ago, after some careful examination [1]. Since the `$FlowFixMe`s start getting flagged as "unused suppression comments", remove them. Strangely, `this.textInputRef.current` and friends are `any(implicit)` after this. Hmm. Still, at least we can now say we're doing the `React$ElementRef` part correctly. [1] zulip#4278 (comment)
We should be passing a `React$ElementRef` type as a type argument to `React.createRef`; see zulip#4278 (comment).
As noted in the code comments, one mistake we were making (not seen until now) was passing the wrong type parameter to `React$Ref`; we should be passing a `React$ElementRef` type [1]. When we do that, the existing `$FlowFixMe`s (the ones with comments about RN v0.63) are marked as unused, as we'd expect. But unfortunately, at that point, `.current` is typed as `any(implicit)`, which we definitely don't want. And that doesn't seem to get fixed by upgrading to RN v0.63, so, remove the comments about that new version. Greg suggests this may be caused by bugs in Flow's special support for React [2]. So, mention our new knowledge that we need to be using `React$ElementRef`, but retain some fixmes and comments alerting us to the fact that `.current` is untyped and that we need to be careful about using it. [1] zulip#4278 (comment) [2] zulip#4420 (comment)
We should be passing a `React$ElementRef` type as a type argument to `React.createRef`; see zulip#4278 (comment).
As noted in the code comments, one mistake we were making (not seen until now) was passing the wrong type parameter to `React$Ref`; we should be passing a `React$ElementRef` type [1]. When we do that, the existing `$FlowFixMe`s (the ones with comments about RN v0.63) are marked as unused, as we'd expect. But unfortunately, at that point, `.current` is typed as `any(implicit)`, which we definitely don't want. And that doesn't seem to get fixed by upgrading to RN v0.63, so, remove the comments about that new version. Greg suggests this may be caused by bugs in Flow's special support for React [2]. So, mention our new knowledge that we need to be using `React$ElementRef`, but retain some fixmes and comments alerting us to the fact that `.current` is untyped and that we need to be careful about using it. [1] zulip#4278 (comment) [2] zulip#4420 (comment)
We should be passing a `React$ElementRef` type as a type argument to `React.createRef`; see zulip#4278 (comment).
As noted in the code comments, one mistake we were making (not seen until now) was passing the wrong type parameter to `React$Ref`; we should be passing a `React$ElementRef` type [1]. When we do that, the existing `$FlowFixMe`s (the ones with comments about RN v0.63) are marked as unused, as we'd expect. But unfortunately, at that point, `.current` is typed as `any(implicit)`, which we definitely don't want. And that doesn't seem to get fixed by upgrading to RN v0.63, so, remove the comments about that new version. Greg suggests this may be caused by bugs in Flow's special support for React [2]. So, mention our new knowledge that we need to be using `React$ElementRef`, but retain some fixmes and comments alerting us to the fact that `.current` is untyped and that we need to be careful about using it. [1] zulip#4278 (comment) [2] zulip#4420 (comment)
Fixes: #3804
To follow #4274, which is still in review.