Skip to content

Commit

Permalink
AppNavigator: Stop changing what we pass for Stack.Screens' `compon…
Browse files Browse the repository at this point in the history 
…ent`.

Before, whenever `AppNavigator` was called, a new value was passed
to these `Stack.Screen`s as `component`. That's because
`withHaveServerDataGate` returns a fresh component made by
react-redux's `connect`.

We found with a bisect that something in a80b4e8 was
causing #4723.

Here, we stop letting that value change, and we see that it fixes
that issue.

Alternatively, we might have put the `withHaveServerDataGate` call
in the same file as `MainTabsScreen`, etc. -- after all, that's our
usual practice for higher-order components. But the choice to put it
in AppNavigator (in a80b4e8 and f53d4f6) was intentional: we
wanted to make it easy to scan through and spot any that should be
treated with `withHaveServerDataGate` and aren't.

We considered `useMemo` instead of `useRef`, but the
documentation [1] cautions against that for cases like these:

> **You may rely on useMemo as a performance optimization, not as a
> semantic guarantee.** In the future, React may choose to “forget”
> some previously memoized values and recalculate them on next
> render, e.g. to free memory for offscreen components. Write your
> code so that it still works without `useMemo` — and then add it to
> optimize performance.

So, use `useRef`.

[1] https://reactjs.org/docs/hooks-reference.html#usememo

Co-authored-by: Greg Price <greg@zulip.com>
Fixes: #4723
  • Loading branch information
@chrisbobbe @gnprice
chrisbobbe and gnprice committed Jun 1, 2021
1 parent 61b136d commit 98d19b8
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 26 deletions.
50 changes: 25 additions & 25 deletions src/nav/AppNavigator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import EmojiPickerScreen from '../emoji/EmojiPickerScreen';
import LegalScreen from '../settings/LegalScreen';
import UserStatusScreen from '../user-status/UserStatusScreen';
import SharingScreen from '../sharing/SharingScreen';
import withHaveServerDataGate from '../withHaveServerDataGate';
import { useHaveServerDataGate } from '../withHaveServerDataGate';

export type AppNavigatorParamList = {|
'account-pick': RouteParamsOf<typeof AccountPickScreen>,
Expand Down Expand Up @@ -108,40 +108,40 @@ export default function AppNavigator(props: Props) {
{/* These screens expect server data in order to function normally. */}
<Stack.Screen
name="account-details"
component={withHaveServerDataGate(AccountDetailsScreen)}
component={useHaveServerDataGate(AccountDetailsScreen)}
/>
<Stack.Screen name="group-details" component={withHaveServerDataGate(GroupDetailsScreen)} />
<Stack.Screen name="chat" component={withHaveServerDataGate(ChatScreen)} />
<Stack.Screen name="emoji-picker" component={withHaveServerDataGate(EmojiPickerScreen)} />
<Stack.Screen name="main-tabs" component={withHaveServerDataGate(MainTabsScreen)} />
<Stack.Screen name="group-details" component={useHaveServerDataGate(GroupDetailsScreen)} />
<Stack.Screen name="chat" component={useHaveServerDataGate(ChatScreen)} />
<Stack.Screen name="emoji-picker" component={useHaveServerDataGate(EmojiPickerScreen)} />
<Stack.Screen name="main-tabs" component={useHaveServerDataGate(MainTabsScreen)} />
<Stack.Screen
name="message-reactions"
component={withHaveServerDataGate(MessageReactionsScreen)}
component={useHaveServerDataGate(MessageReactionsScreen)}
/>
<Stack.Screen
name="search-messages"
component={withHaveServerDataGate(SearchMessagesScreen)}
component={useHaveServerDataGate(SearchMessagesScreen)}
/>
<Stack.Screen name="users" component={withHaveServerDataGate(UsersScreen)} />
<Stack.Screen name="language" component={withHaveServerDataGate(LanguageScreen)} />
<Stack.Screen name="lightbox" component={withHaveServerDataGate(LightboxScreen)} />
<Stack.Screen name="create-group" component={withHaveServerDataGate(CreateGroupScreen)} />
<Stack.Screen name="invite-users" component={withHaveServerDataGate(InviteUsersScreen)} />
<Stack.Screen name="diagnostics" component={withHaveServerDataGate(DiagnosticsScreen)} />
<Stack.Screen name="variables" component={withHaveServerDataGate(VariablesScreen)} />
<Stack.Screen name="timing" component={withHaveServerDataGate(TimingScreen)} />
<Stack.Screen name="storage" component={withHaveServerDataGate(StorageScreen)} />
<Stack.Screen name="debug" component={withHaveServerDataGate(DebugScreen)} />
<Stack.Screen name="users" component={useHaveServerDataGate(UsersScreen)} />
<Stack.Screen name="language" component={useHaveServerDataGate(LanguageScreen)} />
<Stack.Screen name="lightbox" component={useHaveServerDataGate(LightboxScreen)} />
<Stack.Screen name="create-group" component={useHaveServerDataGate(CreateGroupScreen)} />
<Stack.Screen name="invite-users" component={useHaveServerDataGate(InviteUsersScreen)} />
<Stack.Screen name="diagnostics" component={useHaveServerDataGate(DiagnosticsScreen)} />
<Stack.Screen name="variables" component={useHaveServerDataGate(VariablesScreen)} />
<Stack.Screen name="timing" component={useHaveServerDataGate(TimingScreen)} />
<Stack.Screen name="storage" component={useHaveServerDataGate(StorageScreen)} />
<Stack.Screen name="debug" component={useHaveServerDataGate(DebugScreen)} />
<Stack.Screen
name="stream-settings"
component={withHaveServerDataGate(StreamSettingsScreen)}
component={useHaveServerDataGate(StreamSettingsScreen)}
/>
<Stack.Screen name="edit-stream" component={withHaveServerDataGate(EditStreamScreen)} />
<Stack.Screen name="create-stream" component={withHaveServerDataGate(CreateStreamScreen)} />
<Stack.Screen name="topic-list" component={withHaveServerDataGate(TopicListScreen)} />
<Stack.Screen name="notifications" component={withHaveServerDataGate(NotificationsScreen)} />
<Stack.Screen name="legal" component={withHaveServerDataGate(LegalScreen)} />
<Stack.Screen name="user-status" component={withHaveServerDataGate(UserStatusScreen)} />
<Stack.Screen name="edit-stream" component={useHaveServerDataGate(EditStreamScreen)} />
<Stack.Screen name="create-stream" component={useHaveServerDataGate(CreateStreamScreen)} />
<Stack.Screen name="topic-list" component={useHaveServerDataGate(TopicListScreen)} />
<Stack.Screen name="notifications" component={useHaveServerDataGate(NotificationsScreen)} />
<Stack.Screen name="legal" component={useHaveServerDataGate(LegalScreen)} />
<Stack.Screen name="user-status" component={useHaveServerDataGate(UserStatusScreen)} />

{/* These screens do not expect server data in order to function
normally. */}
Expand Down
20 changes: 19 additions & 1 deletion src/withHaveServerDataGate.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* @flow strict-local */
import React, { type ComponentType, type ElementConfig } from 'react';
import React, { type ComponentType, type ElementConfig, useRef } from 'react';

import { connect } from './react-redux';
import type { Dispatch } from './types';
Expand All @@ -16,6 +16,9 @@ import FullScreenLoading from './common/FullScreenLoading';
* The implementation uses props named `dispatch` and `haveServerData`; the
* inner component shouldn't try to accept props with those names, and the
* caller shouldn't try to pass them in.
*
* Inside a render method, don't call this directly: like most HOCs, it will
* return a new value each time. Instead, use `useHaveServerDataGate`.
*/
// It sure seems like Flow should catch the `dispatch` / `haveServerData`
// thing and reflect it in the types; it's not clear why it doesn't.
Expand Down Expand Up @@ -61,3 +64,18 @@ export default function withHaveServerDataGate<P: { ... }, C: ComponentType<$Exa
),
);
}

/**
* Like `withHaveServerDataGate`, but with a stable value on React re-render.
*
* This is a React hook. On initial render, it returns the same value as
* `withHaveServerDataGate` would. On re-render, it returns the same value
* as on the previous render.
*/
export function useHaveServerDataGate<P: { ... }, C: ComponentType<$Exact<P>>>(
Comp: C,
): ComponentType<$Exact<ElementConfig<C>>> {
// Not `useMemo`, because that function's memoization is only a
// performance optimization and not a semantic guarantee.
return useRef(withHaveServerDataGate(Comp)).current;
}

0 comments on commit 98d19b8

Please sign in to comment.