Skip to content

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe
Copy link
Contributor Author

One important difference from the previous draft is that I'm grabbing the haveServerData value as it's supplied by the connect call, instead of having the connect call just sit there without supplying anything (but dispatch) and using useSelector.

Manual testing on the earlier approach showed that the no-server-data state was not preventing the component's subtree from rerendering.

@chrisbobbe
Copy link
Contributor Author

This PR has MainTabsScreen and ChatScreen as the only two screen components that use the HOC.

When I said

Ideally we should do these things on all screens that depend on server data.

Greg said

Do we need to do it for all those screens (which is most of our screens), or just the ones it's possible to navigate to without using the UI of another such screen? I think the latter.

And I agree.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One important difference from the previous draft is that I'm grabbing the haveServerData value as it's supplied by the connect call, instead of having the connect call just sit there without supplying anything (but dispatch) and using useSelector.

Manual testing on the earlier approach showed that the no-server-data state was not preventing the component's subtree from rerendering.

Ah, that set of observations makes sense now that I think about it. I don't quite have in my head all the mechanics of how this scenario would work, but: if we have a connect sitting there but then its inner component uses useSelector to control the conditional, then the connect will go ahead and cause the inner component to rerender before deciding whether to notify descendant components to rerender... but that inner component will be getting its data from a source that doesn't necessarily get the updated information at the same time as connect would have provided it.

Hmm, in fact: the inner component calling useSelector(getHaveServerData) means it's getting that information in the same way as all those other components below in the tree, via the new ReactReduxData.Provider that's interposed by connect. The point for us of making sure to get that new provider interposed here is so that those components below in the tree continue to get the old version of the data until we've decided that the new version is OK to render from. But that means that to make that decision correctly, we need to be looking at the new version at a time when those descendant components are still seeing the old version.

I still don't feel I have that entirely pinned down, but I think that may be an accurate intuition for why useSelector(getHaveServerData) wasn't working here, and actually getting the information from the connect is necessary.


This PR has MainTabsScreen and ChatScreen as the only two screen components that use the HOC.

When I said
[…]

Hmm. But didn't we then discuss how in the case of a forced logout, this issue could arise on any screen that needs server data? Regardless of how one navigates to it, that is, because the forced logout can come when we're already on it.

Fortunately I think the tip commit of this branch (adding it to ChatScreen) demonstrates that it takes very little code per screen to do that, after the infrastructure built in the rest of the branch.

Comment on lines 127 to 128
* Passing a `dispatch` prop or a `haveServerData` prop to the
* returned component will lead to undefined behavior; don't.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it? I believe what will happen is connect will just clobber those props.

Also I think if Flow sees you're doing that, you'll get an error because the returned component-type doesn't accept those props -- it accepts what the argument Comp accepts, minus those two. That Flow error will probably be pretty obscure, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it? I believe what will happen is connect will just clobber those props.

Ah, I meant "undefined" in the sense of "we don't care to define", not "we can't define". 🙂 To make the component's interface more stable across implementation changes.

Does that sound like a good strategy? If so, I think this bit of jsdoc might be good to keep, especially with the likelihood that the Flow error would be obscure. There may be a better way to write it, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, rereading, I think what I said above is correct about the runtime behavior -- but if the inner component wants to take those props, and you pass them to the outer component, Flow won't give an error.

That's seen in the fact that the signature takes a component and returns a component which takes the same props:

export default function withHaveServerDataGate<P: { ... }, C: ComponentType<$Exact<P>>>(
  Comp: C,
): ComponentType<$Exact<ElementConfig<C>>> {

so if Flow were going to complain about how the implementation doesn't totally agree with that, it should do so already when type-checking the implementation.

And indeed empirically:

function example(c: ComponentType<{| dispatch: Dispatch |}>) {
  const MyComponent = withHaveServerDataGate(c);
  <MyComponent />; // error
  <MyComponent dispatch={(null: $FlowFixMe)} />; // no error
}

I think that's basically OK, because trying to pass these two props through doesn't seem like a likely mistake to make, and I don't think one would get very far before seeing it wasn't working.

But yeah, a jsdoc mention of it seems good. I'll try tweaking the wording.

}

export default withHaveServerDataGate(MainTabsScreen);
export default withHaveServerDataGate<Props, ComponentType<Props>>(MainTabsScreen);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can simplify this like so:

export default withHaveServerDataGate<Props, _>(MainTabsScreen);

and let it infer the second type argument as ComponentType<Props>.

We'll basically always want it to be that, which is also the totally boring thing for it to be. So I think a convention of saying just _ is the most helpful one -- like we do with connect.

Oh, in fact, we can simplify further: drop C: ComponentType<$Exact<P>> from the definition, and just substitute ComponentType<$Exact<P>> in the one place the definition mentions C.

In the type of connect I do something like this C argument, because it appears several times and there's a readability benefit to not repeating it. But it has this trade-off that it awkwardly adds a type parameter to the interface. Here we can just skip that.

@chrisbobbe chrisbobbe force-pushed the pr-server-data-gate branch from ad2b0cb to 5210b31 Compare April 7, 2021 00:57
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Apr 7, 2021

Thanks for the review!

Hmm. But didn't we then discuss how in the case of a forced logout, this issue could arise on any screen that needs server data? Regardless of how one navigates to it, that is, because the forced logout can come when we're already on it.

Ah, yeah, makes sense.

My new revision doesn't yet use the HOC on all screen components that expect server data; I'm questioning an approach for where to call the HOC in a "no merge" commit at the tip.

Also, please see my comment at #4603 (comment). :)

@chrisbobbe chrisbobbe force-pushed the pr-server-data-gate branch from 5210b31 to 9c05c9e Compare April 7, 2021 01:01
@gnprice
Copy link
Member

gnprice commented Apr 8, 2021

the same file as the components we're calling them on. Also,
interacting with the type-param interface gets awkward. Forgetting
to pass the type param is (as far as I've seen) allowed, when the
calls are here, but it means we lose type-checking of the resulting
component (it didn't complain when I tried to put a
`withHaveServerDataGate(MainTabsScreen)` for e.g. the `emoji-picker`
screen.

Hmmm.

Well, here's a version that works!

-export default function withHaveServerDataGate<P: { ... }>(
-  Comp: ComponentType<$Exact<P>>,
-): ComponentType<$Exact<P>> {
+export default function withHaveServerDataGate<P: { ... }, C: ComponentType<$Exact<P>>>(
+  Comp: C,
+): ComponentType<$Exact<ElementConfig<C>>> {

That allows leaving off the type parameter at the call site:

-      <Stack.Screen
-        name="main-tabs"
-        component={withHaveServerDataGate<ElementConfig<typeof MainTabsScreen>>(MainTabsScreen)}
-      />
+      <Stack.Screen name="main-tabs" component={withHaveServerDataGate(MainTabsScreen)} />

plus now if you use a component that doesn't match up with the route name (like MainTabsScreen with emoji-picker), you get an error. Actually a series of errors, but some of them include the key identifiers to let you know what's wrong.

I don't totally have an explanation of why Flow doesn't give an error in the other case. It seems like in principle it should. I think with type-level operators like $Diff and ComponentType (the latter of which is built into Flow along with some other React-specific logic), sometimes the type information doesn't quite flow bidirectionally in all the ways that it should, and it's possible that one of those issues is the cause here.

What this version does is basically copy the way connect works:

export function connect<SP, P, C: ComponentType<P>>(
  mapStateToProps?: (GlobalState, OwnProps<C,
    // Error "property `foo` is missing"?  Add to inner component's props.
    SP>) => SP,
): C => ComponentType<$ReadOnly<OwnProps<C, SP>>> {

but with the return type simplified a bit because it should be passing through the same props.

@gnprice
Copy link
Member

gnprice commented Apr 8, 2021

And then I agree it's probably best to have the calls to withHaveServerDataGate at the navigator, rather than at the components' definitions. It'll certainly make it easier to scan through and spot any that should have it and don't.

@chrisbobbe chrisbobbe force-pushed the pr-server-data-gate branch from 9c05c9e to f25f135 Compare April 8, 2021 01:29
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Apr 8, 2021

Thanks for the review! Revision pushed.

I did a bit of tweaking and testing with TimingScreen, a random one of those screens that gets wrapped. Part of that was to make it a function component, so I've left that conversion commit in.

Also a bump on #4603 (comment). 🙂

chrisbobbe and others added 10 commits April 14, 2021 15:23
Combining the `connect` call and the show-if-server-data conditional
into one reusable HOC.
We'd like to reuse this for other components, as its jsdoc suggests.
…ator`.

Soon, we'll give all the components that need server data this
treatment. Putting the calls in `AppNavigator` will make it easier
to scan through and spot any that should have it and don't.
The user could have any of these screens mounted when they receive a
force-logout. If that happens, we want them all to handle it
gracefully.

Putting the calls here, instead of alongside each component's
definition, will make it easier to scan through and spot any that
should use the HOC and don't.
@gnprice gnprice force-pushed the pr-server-data-gate branch from f25f135 to a26c99e Compare April 14, 2021 22:24
@gnprice gnprice merged commit a26c99e into zulip:master Apr 14, 2021
@gnprice
Copy link
Member

gnprice commented Apr 14, 2021

Looks good, thanks! Merged.

I added a comment-commit on top reflecting that remaining thread above.

@chrisbobbe
Copy link
Contributor Author

Thanks for the review and that tweak!

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 7, 2021
…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

Here, we stop letting that value change, and we see that it
fixes zulip#4603.

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.

This approach lets us keep the calls in AppNavigator, but it adds
some ugly boilerplate. Ah, well.

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#useref

Fixes: zulip#4723
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request May 7, 2021
…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

Here, we stop letting that value change, and we see that it
fixes zulip#4603.

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.

This approach lets us keep the calls in AppNavigator, but it adds
some ugly boilerplate. Ah, well.

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

Fixes: zulip#4723
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants