React Navigation v5 upgrade (part 1)

[chrisbobbe]

Fixes [edit: now the first portion of a fix for] #4296

Here, you can see how gigantic the main upgrade commit is—do you think it would be good to split it up, and how might be a good way to do that, I wonder?

[gnprice]

Thanks!

Discussed this in chat.

We've discussed some ideas [...] and I think the picture is quite encouraging! It seems like we'll be able to make this upgrade pretty clean.

[chrisbobbe]

Thanks for the review and many suggestions, @gnprice! In this revision, the main upgrade commit is considerably smaller.

[gnprice]

Thanks for the revisions! I like how much smaller that main upgrade commit is now 😄

Comments below on the first N commits of the branch. I read this far in today's session:
11d1dc8 types: Remove a dubious type.
872cc5b MessageReactionList [nfc]: Remove an already-done TODO.
ae9529c SharingScreen: Rename tab routes.
8e82bb2 ChatScreen: Use React Hooks.
6207f3f AppDataFetcher: Check needsInitialFetch on componentDidMount, too.
d712b28 AppDataFetcher: Make componentDidUpdate an instance method.
2f673b1 AppDataFetcher: Use React Hooks.
c07bb6e ZulipMobile: Don't show most of the component tree until done hydrating.
431cb5e InitialNavigationDispatcher [nfc]: Factor out getInitialRouteInfo.
10b7361 ZulipMobile: Set initial route declaratively; remove "initial navigation".
4704df7 accountsSelectors types: Change a $ReadOnlyArray to a regular array.
ab40b24 actionTypes [nfc]: Stop re-exporting a type from React Navigation.
11dd3dc NavigationService [nfc]: Take readiness assertion out into a helper function.
a3bf821 nav types [nfc]: Add src/nav/globalTypes, to be used soon.
11e5774 RealmScreen types: Mark that navigation.state.params will be present.

(plus looked ahead a bit, particularly to see the shrinkage of that main commit 😉 .) I think most of these will be good to merge eagerly, even as we're still working on the later portion of the branch.

Still to review:

e8e7f9f screen types [nfc]: Rewrite navigation type for screens on AppNavigator.
f2d9044 screen types [nfc]: Rewrite navigation type for screens on MainTabs.
c61b4ce screen types [nfc]: Rewrite navigation type for screens on StreamTabs.
ea703f4 screen types [nfc]: Rewrite navigation type for screens on SharingScreen.
b704740 MessageReactionList: Pass initialRouteName, even if undefined.
c339e91 MessageReactionList [nfc]: Replace getReactionsTabs with two functions.
74775f3 ZulipAppContainer: Pass theme prop to AppContainer.
c263c1e nav [nfc]: Remove some soon-to-be-unnecessary comments.
377a1a3 deps: Add libdefs for new, differently named React Navigation packages.
9a7a128 deps: Upgrade to React Navigation v5.
a759f8c deps: Clear out old libdefs for react-navigation.
f81a45d MessageReactionList [nfc]: Inline some helper functions.
5f203d3 navActions: Stop using @react-navigation/compat.
19167f6 ChatScreen: Stop using @react-navigation/compat.
43f1bd9 MainTabs: Stop using @react-navigation/compat.
96e11bf AppNavigator: Stop using @react-navigation/compat.
1252aa8 SharingScreen: Stop using @react-navigation/compat.
9037717 StreamTabs: Stop using @react-navigation/compat.
6e4b4d0 deps: Remove @react-navigation/compat.

[gnprice]

It looks like the old code will unconditionally fetch on first mount; and if I'm reading right, the new code won't -- it'll only fetch when first focused. Does that seem right to you?

I think ideally we'd have the existing behavior: fetch immediately on first mount. If this component got mounted at all, then the user will surely want to see it soon and we might as well get started on fetching. Can we maintain that?

[gnprice]

Aha -- I think we can do this by translating the componentDidMount like so:

React.useEffect(
  () => {
    fetch();
  },
  [],
);

Got that from the note here in the useEffect reference doc.

Well, except I think that will cause two fetches on first mount, if it happens that isFocused is already true.

Ah, but we can prevent that: put this useEffect in between the other two, and have it set shouldFetchWhenNextFocused.current = false. Or perhaps better yet, push that latter line inside fetch, since both its callsites will be doing it.

[chrisbobbe]

Hmm, good points! I've responded in chat.

[gnprice]

I think these two useEffects, and the associated state, would be a great candidate to pull out as a separate function. The call site might look like:

const fetchError = useFetchMessages({ eventQueueId, isFocused, narrow, dispatch });

The implementation would have shouldFetchWhenNextFocused as its own local variable (with the same React.useRef call), and make the React.useState call for fetchError and both calls to React.useEffect.

See discussion here: https://reactjs.org/docs/hooks-custom.html

[chrisbobbe]

Good idea! I've responded in chat, here.

[gnprice]

The old code at this spot is unnecessarily tricky, as I stare at it again: instead of this conditional, it could just unconditionally set this.shouldFetchWhenNextFocused = true.

Then the code just below here would see that, and if isFocused were indeed true, it'd go and do a fetch, and set this.shouldFetchWhenNextFocused back to false. That'd be simpler -- and also closer to what the hooks version ends up looking like. So if you do that as a prep commit, then it'll be a simplification in itself and also make it easier to reason about the refactor to hooks.

Hmm in fact: there's one case where that'd change the behavior -- and I think it fixes a subtle bug. If should-fetch is already true, and then this gets called with a(nother) change to eventQueueId at a time when we're focused... then the existing code will go and fetch, and leave should-fetch true. So then if we get unfocused and focused again, we'll fetch a second time, even if the queue hasn't changed. The hooks change fixes that, and so would the simplification that drops this conditional.

[chrisbobbe]

Thanks for catching this!

[gnprice]

The code for determining when `doInitialFetch` needs to run is now
more compact and easier to follow, as long as we're OK working with
React Hooks. I don't believe any functional changes are instrumental
to the upcoming upgrade to React Navigation 5.

I'm not sure I follow the last bit. Do you think this is NFC? It looks like it should be. If not, it'd be educational to discuss what does change.

[chrisbobbe]

One thing I'm not so sure about is how shouldComponentUpdate and PureComponents get translated to Hooks.

One thing we could use, in the direction of getting PureComponent's optimizations, is React.memo. While PureComponent does a shallow comparison of both props and state, React.memo only shallowly compares props. Not using React.memo, I believe, is comparable to using Component instead of PureComponent.

Can you think of any advantage we've been currently getting by using PureComponent for AppDataFetcher? I'm a little stuck on that question when I look at our pattern of using children as a render prop. My first thought is that we're probably not getting much of an advantage, if any. But you're right, it would be educational to find out.

[chrisbobbe]

I'm not sure how the following translates to React Native, but in React on the Web, useEffect is unlike componentDidMount in that it fires after the browser has painted. To avoid this change, the thing to use is useLayoutEffect, but the docs specifically recommend trying useEffect first:

If you’re migrating code from a class component, note useLayoutEffect fires in the same phase as componentDidMount and componentDidUpdate. However, we recommend starting with useEffect first and only trying useLayoutEffect if that causes a problem.

[chrisbobbe]

Looking through a TL;DR on a blog post by Dan Abramov on useEffect, it seems smart to generally assume there will be functional changes when you convert a class component to one that uses Hooks.

But I think we've avoided the most prominent among them: the "capturing" of props and state from the body of the component function. If, in the useEffect, we awaited some asynchronous thing, and then after that, we read some values from props or state, we would risk those values being stale. But we don't do such a thing.

[gnprice]

I replied in chat, to make more room and because this seems like a discussion with reusable interest.

[gnprice]

This is NFC, right? If so, helpful to say that in the commit message; if not, I'm curious about how 🙂

Also:

AppDataFetcher: Make `componentDidUpdate` an instance method.

I think "prototype method" would be clearer. The thing it was, in the old code, is a property on each instance, so it's confusing to have "instance method" mean the thing that contrasts with that. The thing it becomes in this new code is a property on the prototype.

I'm not aware of a canonical standard pair of terms for this distinction, but I got "prototype method" from the MDN "Classes" article, and it fits well.

[chrisbobbe]

Ah, right, makes sense! Thanks!

[gnprice]

- We attach a few event listeners in `AppEventHandlers` for some
  graphical concerns: handling orientation changes and setting the
  safe-area insets. These should be fine to be delayed as long as
  the component we show during the delay (`LoadingScreen`) is very
  simple, which it is.

Yeah. Hmm, let's see if we can pin down what it means to be simple that way: looking closely at AppEventHandlers, what it actually does with either of those is to dispatch some Redux actions, to store the orientation or the safe-area insets into Redux. So as long as LoadingScreen doesn't try to go get those from Redux, it can't be affected.

Ooh, but in fact it does. Specifically it invokes ZulipStatusBar, which does, like so:

export default connect<SelectorProps, _, _>((state, props) => ({
  safeAreaInsets: getSession(state).safeAreaInsets,
  theme: getSettings(state).theme,
  narrowTitleBackgroundColor: getTitleBackgroundColor(state, props.narrow),
  orientation: getSession(state).orientation,
}))(ZulipStatusBar);

Now, I'm not sure that logic is right. See #3066. But it will potentially be affected.

[chrisbobbe]

Oh, right. Hmm. So, during rehydration, the default values will be used. A particular problem is that the safe-area insets default to zero. (That doesn't matter when the orientation is landscape, since the status bar isn't shown and the safe-area insets are ignored. But the default value for the orientation is portrait.)

We currently use react-native-safe-area to asynchronously get the safe-area insets on startup and put them in Redux.

I wonder about using the much-more-popular react-native-safe-area-context (already in our dependencies) instead. Once you've set up a SafeAreaProvider, it looks like you can subscribe any component to the safe-area insets using a Hook, a HOC, or a Context consumer. This seems a more natural approach than depending on a dispatch to Redux, maybe? And it looks like react-native-safe-area doesn't have something like this.

[chrisbobbe]

I wonder about using the much-more-popular react-native-safe-area-context (already in our dependencies) instead.

Just opened #4315 for this.

[gnprice]

Hmm! If this array really can't be read-only, then that's potentially a sign of something wrong someplace that consumes this data. So worth a quick investigation.

If at the tip of this branch, I edit this back to $ReadOnlyArray, I get this error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/account/AccountPickScreen.js:84:16

Cannot call connect because read-only array type [1] is incompatible with array
type [2] in property accounts of type argument T.

     src/account/AccountPickScreen.js
 [2] 19│   accounts: AccountStatus[],
       :
     81│   }
     82│ }
     83│
     84│ export default connect(state => ({
     85│   accounts: getAccountStatuses(state),
     86│   hasAuth: hasAuth(state),
     87│ }))(AccountPickScreen);
     88│

     src/account/accountsSelectors.js
 [1] 17│ export const getAccountStatuses: Selector<$ReadOnlyArray<AccountStatus>> = createSelector(

Changing that array type to read-only too, I get a similar error at the next hop; changing that one too, the chain terminates. So this diff fixes things:

diff --git a/src/account/AccountList.js b/src/account/AccountList.js
index 01ef1585a..4a6aa78d0 100644
--- a/src/account/AccountList.js
+++ b/src/account/AccountList.js
@@ -9,3 +9,3 @@ import AccountItem from './AccountItem';
 type Props = $ReadOnly<{|
-  accounts: AccountStatus[],
+  accounts: $ReadOnlyArray<AccountStatus>,
   onAccountSelect: number => void,
diff --git a/src/account/AccountPickScreen.js b/src/account/AccountPickScreen.js
index 0af0b3b8f..48e4e5eb2 100644
--- a/src/account/AccountPickScreen.js
+++ b/src/account/AccountPickScreen.js
@@ -18,3 +18,3 @@ type Props = $ReadOnly<{|
 
-  accounts: AccountStatus[],
+  accounts: $ReadOnlyArray<AccountStatus>,
   dispatch: Dispatch,

In general we should probably have a lot more array types marked as read-only than we do. So when there's a mismatch like this, my preference is to fix it by expanding what's marked that way rather than shrinking it.

In addition to catching if we accidentally pass a value somewhere that could mutate it, the core thing that marking as read-only does for us is it makes the type covariant.

• If you wanted an array of some base type A and what you have is an array of a subtype B, that won't do -- the code that wanted A[] could expect to be able to add an A value to it, which would break its being a B[]. So the type-operator [] aka Array is "invariant".
• But if you only wanted a $ReadOnlyArray<A>, then a $ReadOnlyArray<B> is perfectly fine: the code that wanted the $ReadOnlyArray<A> might read things from it and it'll get B values, and those are fine A values. So B a subtype of A means $ReadOnlyArray<B> is a subtype of $ReadOnlyArray<A>; in other words, $ReadOnlyArray is "covariant".

So for example a GravatarURL[] couldn't be passed to a function that wanted AvatarURL[]; but if the function's parameter type is marked read-only, then a $ReadOnlyArray<GravatarURL> could be passed to it.

[chrisbobbe]

Ah, right, thanks for all that detail! 🙂

[gnprice]

Hmm, a shame to have to have the fixme there.

One way to avoid that -- a way to make it transparent to Flow what's happening -- is to have the helper with the assertions also return the container value:

const getContainer = () => {
  if (navigationContainerRef.current === null) {
    throw new Error('Tried to use NavigationService before `navigationContainerRef` was set.');
  }
  if (isReadyRef.current !== true) {
    throw new Error('Tried to use NavigationService before `NavigationContainer` was ready.');
  }
  return navigationContainerRef.current;
};

Then the two callers nicely simplify, too:

const getState = (): PossiblyStaleNavigationState => getContainer().getRootState();

const dispatch = (navigationAction: GenericNavigationAction): void => getContainer().dispatch(navigationAction);

[gnprice]

Hmm -- can we turn this from export default { foo, bar } style to export const foo = …; export const bar = … style?

The main nice thing that'd give us is that it's manifest at the definition whether it's going to be exported or not -- which helps for understanding the intended interface.

A small bonus is that if you hit Shift+F12 in VS Code on one of these definitions now, to find where it's used, it'll only show you references within the module. Not too bad, because you can go and hit Shift+F12 again over at one of those references -- the export default -- and find all the other places it's used. But it'd be nicer to be able to see that in one hop; and it's also potentially misleading to see references only within the module, as it's not necessarily obvious that's what's happening.

To keep the call sites unchanged like NavigationService.dispatch, I think all that's needed is to say import * as NavigationService rather than import NavigationService. See e.g. 3a6abb3, where I made a similar change for exampleData.

[gnprice]

Before a770a348d, there was an easily overlooked case where `params`
would be missing: although we included `params` whenever we
dispatched an action to navigate to the 'realm' route, there was
still a case in `navReducer` in which the nav state was constructed
with the 'realm' route active but with no params.

I think this isn't actually true -- though the code was certainly tricky. We had return getStateForRoute('realm');... but then that function's definition had this:

 /**
  * Get the initial state for the given route.
  *
  * Private; exported only for tests.
  */
 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 when we went to the 'realm' route via the logic in navReducer, we'd still pass a params object.

(Hmm, and I have the feeling that we've had this conversation before, sometime in the last few weeks. Am I imagining that? The lesson is probably that upon discussing it then, it would have been smart to go promptly edit the code to make it clearer -- with a comment over on the RealmScreen side, if nothing else, but better by clarifying the actual code.)

[gnprice]

(Also the type of that params object isn't quite right: comparing navigateToRealmScreen and this navReducer hack, it should be {| realm?: URL | void, initial: boolean | void |}.)

[chrisbobbe]

(Hmm, and I have the feeling that we've had this conversation before, sometime in the last few weeks. Am I imagining that? The lesson is probably that upon discussing it then, it would have been smart to go promptly edit the code to make it clearer -- with a comment over on the RealmScreen side, if nothing else, but better by clarifying the actual code.)

Ah, right—that conversation was over at #4273 (comment). It looks like (even in a34d3ba) I skipped right over your observation in that discussion that params would always be present, oops. I have a vague memory that I had actually encountered an error while testing the branch, which prompted me to remove 6f6a3ee from it (and comment at #4273 (comment)), and that might have led me to skip over that part. I guess, if there was an error, it was about something else, or else the navReducer code just jumped out at me as a potential reason for params to be missing, even if that turned out to be wrong.

[chrisbobbe]

(Also the type of that params object isn't quite right: comparing navigateToRealmScreen and this navReducer hack, it should be {| realm?: URL | void, initial: boolean | void |}.)

Hmm, now that navReducer is gone and we're using navigateToRealmScreen and resetToRealmScreen, I think it should be {| realm: URL | void, initial: boolean | void |}, maybe? In both those functions, if the caller doesn't pass realm or initial, it ends up being present in the action as undefined.

[gnprice]

I've gone and merged these two commits from early in the branch, as they're completely ready and also aren't closely related to the upgrade:
33de8a4 MessageReactionList [nfc]: Remove an already-done TODO.
5fb6057 SharingScreen: Rename tab routes.

[chrisbobbe]

Thanks for the review, @gnprice! I've pushed a new revision that responds to several comments, and I've got a few replies for you:

• https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20.60ChatScreen.60.20and.20Hooks/near/1064349
• React Navigation v5 upgrade (part 1) #4300 (comment)
• React Navigation v5 upgrade (part 1) #4300 (comment)
• React Navigation v5 upgrade (part 1) #4300 (comment)

[chrisbobbe]

(Just fixed some conflicts.)

[gnprice]

I replied in chat, to make more room and because this seems like a discussion with reusable interest.

[gnprice]

Thanks for the revision! Looks great -- merged.

[chrisbobbe]

Thanks for the revision! Looks great -- merged.

Great, thanks! Just sent #4393 for the rest.