Prep work toward decoupling navigation from Redux.

[chrisbobbe]

Some preparation for #3804.

[gnprice]

Thanks! This direction makes sense to me. Some comments below -- notably I think we should be able to simplify the "wait until the store is hydrated" part. Which IIUC runs through these three commits:
844646a messagesActions: Don't let doNarrow cancel an expected navigation (2/2).
dfac22a AppEventHandlers: Make handleInitialShare wait for rehydration.
3a90d90 AppEventHandlers: Use handleInitialCare correctly.

[gnprice]

So, remove the existing check for whether the Redux store is
hydrated.

But that's not *quite* enough: there's one case in the status quo in
which this check is a band-aid for a crash. In the status quo, if
the app is completely closed and you receive a notification, and tap
it, the app will open and we'll run some JavaScript
(`handleInitialNotification`) to send the user on their way to the
narrow corresponding to the notification. This code runs quite soon
after the app opens; in fact, it doesn't even wait to be sure the
data is rehydrated. It should wait; then, we can avoid the crash
without the band-aid.

Hmm dang -- did you observe this crash empirically?

The existing strategy we have that should prevent this kind of thing is, we use redux-action-buffer to cause all actions to be held up in a queue until a REHYDRATE action comes along. See listMiddleware in src/boot/store.js, and 42adfb3 where we started applying this to thunk actions.

More precisely, I believe all actions other than nav actions (because those are handled by a preceding middleware), and REHYDRATE actions themselves (because that's what we tell redux-action-buffer to look out for), will get enqueued that way. But in particular thunk actions will indeed get held in the queue, because that middleware comes after the createActionBuffer middleware.

I like that strategy fairly well, because it's relatively easy to stick to -- most of the interesting complex things that we do that interact with Redux, if they aren't on some component which isn't going to get shown before a rehydrate, are in a thunk action anyway, which means that for the most part when we don't think about it the right thing happens naturally. Occasionally we've had a bug where we get that wrong (I remember we've seen this at least once), but the fix is easy -- just push the relevant code inside a thunk action creator, and add a dispatch call at its call site -- and that's also nice.

Looking at handleInitialNotification:

export const handleInitialNotification = async (dispatch: Dispatch) => {
  const data = await getInitialNotification();
  dispatch(narrowToNotification(data));
};

it looks like the narrowing happens inside a dispatch, so it ought to be covered by that strategy. If you found empirically that something goes wrong, then let's debug.

[chrisbobbe]

Hmm dang -- did you observe this crash empirically?

I did observe a crash while investigating my suspicion of a crash, but I'm not reproducing it right now; I'll try more tomorrow?

The existing strategy we have that should prevent this kind of thing is, we use redux-action-buffer to cause all actions to be held up in a queue until a REHYDRATE action comes along.

True, but I was specifically thinking to prepare for the future when navigation actions aren't dispatched through Redux and redux-action-buffer will have no power over them. But you're right; now I think more closely about it, being in a thunk action creator should do the trick (and it seems to in my testing just now).

[chrisbobbe]

Hmm dang -- did you observe this crash empirically?

I did observe a crash while investigating my suspicion of a crash, but I'm not reproducing it right now; I'll try more tomorrow?

OK, I think I've solved this mystery—I was logged out when I observed the crash. If I set it up so that handleInitialNotification dispatches doNarrow unconditionally (I had it do so before await getInitialNotification()), it does crash if the user is logged out; I get the "no server data" message. No real surprise there.

But my test case case wasn't representative—narrowToNotification will handle checking to see that the account the notification is meant for is logged in, so there is no crash to worry about here.

[gnprice]

Great -- glad that's diagnosed.

[gnprice]

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].

Yeah, that's odd!

I wondered if this might be a difference between the TS definitions and the Flow libdef, in what they have NavigationStackProp take as a type parameter. But AFAICT it's equally wrong there -- the first type parameter is called State and is used for the state property, not state.params. (Though there is a second type parameter for the latter.)

[chrisbobbe]

Yeah, it's kind of all over the place.

[gnprice]

IIUC the precise logic is: because the only way we invoke this component is as a screen in such a route config, it will always get that prop.

Or I guess, equally correct and broader: because that's the only non-type-checked way we invoke it.

Typically I wouldn't expect us to use one of these components in any other way. But as a way of helping confirm that, I did this quick search: rg -w '[A-Z]\w+Screen' src/ to find all references to things that fit the naming pattern common to almost all these components. Other than their own definitions, including react-redux connect calls, the only ones I found referred to anywhere other than AppNavigator were:

• CompatibilityScreen
• ChooseRecipientsScreen

And indeed those don't appear in this commit, so that checks out.

I repeated the search on the components that appear in this commit and don't fit that naming pattern: rg -we SmartUrlInput -e MainScreenWithTabs -e MessageReactionList src/

That actually found one reference that doesn't line up! Did you mean to include SmartUrlInput?

[chrisbobbe]

Ah—SmartUrlInput should probably go in its own commit. It's a reusable component that expects...a navigation prop, to be passed down in the normal React way (i.e., it's not plugged into a navigator), and it shouldn't care whether that navigation prop comes from a stack navigator, drawer navigator, etc. I landed on NavigationScreenProp<NavigationState>, without much consideration, from reading react-navigation/react-navigation#3643 (comment), and it could be wrong.

[gnprice]

Hmm -- for some reason it seems like this type annotation is not working (it's resulting in any types), whereas in at least some spot-checking the ones from the previous commit with NavigationStackProp are working.

Specifically, if I go and do something silly like

   state = (() => {
-    const { sharedData } = this.props.navigation.state.params;
+    const { sharedData, asdfasdf } = this.props.navigation.state.params;

then I don't get any type error at the attempt to pull an asdfasdf property from params.

Also, hovering on the identifier navigation in that line I just quoted shows "any(implicit)" as the type, and ditto for the subsequent state or params. (That's the symptom I first noticed; I tried the asdfasdf thing just to confirm that it wasn't some IDE glitch.)

Perhaps relatedly, if in VS Code I hit F12 on NavigationTabProp to try to go to its definition, it doesn't find it -- it can only take me to the import type line at the top of this file. Whereas it does work fine for NavigationStackProp in a file like AccountDetailsScreen.js. There does seem to be a libdef in flow-typed/, so not sure what the issue is.

[chrisbobbe]

I have a hunch that I'll look into tomorrow—we've got two libdefs for react-navigation-tabs in flow-typed/npm, and one of them is a stub where everything is any.

[chrisbobbe]

Aha...that's it. Yeah, getting rid of that stub libdef (which we don't want anyway; I don't have reasons to suspect the real one is irredeemably bad) reveals lots of Flow errors that seem productive to address.

[gnprice]

Aha...that's it. Yeah, getting rid of that stub libdef (which we don't want anyway; I don't have reasons to suspect the real one is irredeemably bad) reveals lots of Flow errors that seem productive to address.

Aha! Good catch.

[chrisbobbe]

Thanks for the review! Just pushed some changes and responded to your comments above.

[chrisbobbe]

e192254 RealmScreen: Assume navigation.state.params will exist.

Oops! This commit is wrong; please don't merge until I've pushed a fix. I forgot about reaching RealmScreen through navReducer.

[chrisbobbe]

OK, done.

[gnprice]

One quibble remaining on this commit: this bit of comment doesn't really belong here. In a couple of ways, I guess:

• It doesn't say anything that's part of the interface, something that's actionable from the perspective of when you're using this function. So it doesn't belong in the jsdoc, though it could as an implementation comment.

• Even as an implementation comment, the state of affairs it's describing is really common to almost all our thunk actions creators, not distinctive to this one.

  In seeing it the reader naturally wonders, wait, why is this a thing we have to worry about here in particular? What's happening here that brings it up? The answer is really just that there used to be a getIsHydrated check here -- now that there no longer is, there's nothing in particular to say about it at this spot. But that means this remark is really naturally about the change that removed that check, not about the code after that change, so it belongs just in the commit message.

Or from another angle: this commit is basically completing a cleanup that ideally I'd have done right after 42adfb3 as part of the same series. In order to really clean up completely, this code should end up with no lasting complications from that wrinkle at all.

[gnprice]

Aha...that's it. Yeah, getting rid of that stub libdef (which we don't want anyway; I don't have reasons to suspect the real one is irredeemably bad) reveals lots of Flow errors that seem productive to address.

Aha! Good catch.

[gnprice]

Thanks for the revisions!

This all LGTM. A few comments below (and above, thanks GitHub) on small things; please merge at will after going through those.

[gnprice]

Why optional at all? I think the logic in your previous version (at 6f6a3ee) was sound for why it shouldn't simply go missing; and following that logic we make params non-optional on other screens too.

[gnprice]

Ah I see, you answered that here 🙂 : #4273 (comment)

I think this calls for a comment saying why it's optional, in any case -- since that is out of the ordinary compared to how all, I think, of our other screens work.

... Then I went reading this code some more to see how it handles the case where params is missing. OK, effectively realm gets defaulted to '', but initial? It seems to default to false. So when is it ever true -- is that just dead code? ... No, it's not. The answer is this hack in navReducer.js:

export const getStateForRoute = (route: string): NavigationState => {
  // TODO: this is kind of a hack!  Refactor to a better way.
  //  * Perhaps pass `initial: true` unconditionally in this initialization
  //    code, to all routes?  Then that'd just be part of the interface of
  //    making a screen work as an initial screen.
  //  * Alternatively, we could replace the whole system of `ModalNavBar`,
  //    `canGoBack`, etc., with our own "navigation view" that would be more
  //    directly integrated into the navigation framework:
  //      https://reactnavigation.org/docs/en/2.x/navigation-views.html
  const params = route === 'realm' ? { initial: true } : undefined;

So in fact in navReducer when we navigate to this screen, we also pass a params object (which I guess means we always do.) It has a property initial: true, and no params.

I guess now this extra definitely calls for a comment here 😛 Which really I should have added at the same time as adding that hack: 7a04a56

[chrisbobbe]

Yeah, I've already got plans to get rid of that hack soon, as part of the process :)

[gnprice]

Not a big deal, but I suspect you can make this change after the "Tell all screens on AppNavigator" commit (and include the RealmScreen change in that one.) If you do, then I think the story to tell in this one's commit message becomes simpler, because no foreshadowing is necessary.

[chrisbobbe]

I get an error if I leave this as mixed with the RealmScreen change applied:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/start/RealmScreen.js:121:10

Cannot create SmartUrlInput element because in property navigation:
 • Either cannot create exact type from mixed [1].
 • Or property getParam is missing in object type [2] but exists in NavigationScreenProp [3].

        src/start/RealmScreen.js
        118│         shouldShowLoadingBanner={false}
        119│       >
        120│         <Label text="Enter your Zulip server URL:" />
        121│         <SmartUrlInput
        122│           style={styles.input}
        123│           navigation={navigation}
        124│           defaultProtocol="https://"
        125│           defaultOrganization="your-org"
        126│           defaultDomain="zulipchat.com"
        127│           defaultValue={initialRealm}
        128│           onChangeText={this.handleRealmChange}
        129│           onSubmitEditing={this.tryRealm}
        130│           enablesReturnKeyAutomatically
        131│         />
        132│         {error !== null ? (
        133│           <ErrorMsg error={error} />
        134│         ) : (

        flow-typed/npm/react-navigation-stack_v1.x.x.js
    [2] 595│     & {
        596│         pop: (n?: number, params?: {| immediate?: boolean |}) => boolean,
        597│         popToTop: (params?: {| immediate?: boolean |}) => boolean,
        598│         push: (
        599│           routeName: string,
        600│           params?: NavigationParams,
        601│           action?: NavigationNavigateAction
        602│         ) => boolean,
        603│         replace: (
        604│           routeName: string,
        605│           params?: NavigationParams,
        606│           action?: NavigationNavigateAction
        607│         ) => boolean,
        608│         reset: (actions: NavigationAction[], index: number) => boolean,
        609│         ...
        610│       };

        src/common/SmartUrlInput.js
 [3][1]  52│   navigation: NavigationScreenProp<mixed>,

[chrisbobbe]

I could do a temporary $FlowFixMe, but I wonder if that would mean roughly the same amount of explanation that's already in this commit?

[gnprice]

Huh. That's a bit puzzling to me: it feels like it should be perfectly fine, and also when I look at the actual type definitions it seems like it should be accepted. In particular, the new type NavigationScreenProp<NavigationState> should be a subtype of the old type NavigationScreenProp<mixed> -- NavigationState is a subtype of mixed because everything is, and NavigationScreenProp is declared in the react-navigation libdef as covariant (with a +) in its type parameter:

  declare export type NavigationScreenProp<+S> = {

so S a subtype of T should make NavigationScreenProp<S> a subtype of NavigationScreenProp<T>.

But not worth trying to debug further, since you've already made a fine workaround.

[chrisbobbe]

Not a big deal, but I suspect you can make this change after the "Tell all screens on AppNavigator" commit (and include the RealmScreen change in that one.) If you do, then I think the story to tell in this one's commit message becomes simpler, because no foreshadowing is necessary.

Since you said it's not a big deal, I'll go ahead as-is, if that's OK; see the error in #4273 (comment) that I got when I tried the suggested adjustment.

I've addressed everything else, so I'll go ahead and merge.