Finish removing connect calls for exported components, where it's easy.#4926
Finish removing connect calls for exported components, where it's easy.#4926gnprice merged 41 commits intozulip:masterfrom
connect calls for exported components, where it's easy.#4926Conversation
There was a problem hiding this comment.
Thanks @chrisbobbe! I've read through the bulk of these, and am about to go AFK for dinner so leaving a review now with what I have.
In these first two commits:
b592e32 flow: Annotate some exported React components, where very easy.
e0e629b userHelpers types [nfc]: Annotate return type of getUsersAndWildcards.
there are some spots where I'd like to avoid duplicating the annotations, similar to #4948.
Then in all these component conversions:
e9951c7 AccountDetails [nfc]: Convert to function component.
8129673 AccountDetails [nfc]: Use useSelector instead of connect.
b5db0d8 AccountDetailsScreen [nfc]: Convert to function component.
6c5c235 AccountDetailsScreen [nfc]: Use useSelector instead of connect.
8655442 ZulipStatusBar [nfc]: Convert to function component.
a77a051 ZulipStatusBar [nfc]: Use useSelector instead of connect.
236511f Emoji [nfc]: Pull out componentStyles.
ad7531a Emoji [nfc]: Convert to function component.
5c5c21b Emoji [nfc]: Use useSelector instead of connect.
43d1dc4 EmojiPickerScreen [nfc]: Convert to function component.
019cbf5 EmojiPickerScreen [nfc]: Use useSelector instead of connect.
2fa762f MentionedUserNotSubscribed [nfc]: Convert to function component.
8b2fa8e MentionedUserNotSubscribed [nfc]: Use useSelector instead of connect.
fd6942a NotSubscribed [nfc]: Convert to function component.
5be73b8 NotSubscribed [nfc]: Use useSelector instead of connect.
267bcf4 ZulipNavigationContainer [nfc]: Convert to function component.
eafb887 ZulipNavigationContainer [nfc]: Use useSelector instead of connect.
1f0850e InviteUsersScreen [nfc]: Convert to function component.
2d2faa0 InviteUsersScreen [nfc]: Use useSelector instead of connect.
a9dad13 InviteUsersScreen [nfc]: Inline handleFilterChange.
7eb83cf StreamSettingsScreen [nfc]: Convert to function component.
d0ab026 StreamSettingsScreen [nfc]: Use useSelector instead of connect.
ab953a4 SubscriptionsCard [nfc]: Convert to function component.
abde7f4 SubscriptionsCard [nfc]: Use useSelector instead of connect.
7f22d2b StreamListCard [nfc]: Convert to function component.
f392144 StreamListCard [nfc]: Use useSelector instead of connect.
5f40c2b ActivityText [nfc]: Convert to function component.
de67372 ActivityText [nfc]: Use useSelector instead of connect.
7a6fff1 TopicListScreen [nfc]: Add a TODO that we'll address soon.
02e9dcb TopicListScreen [nfc]: Convert to function component.
d309e15 TopicListScreen [nfc]: Use useSelector instead of connect.
2a80044 TopicListScreen [nfc]: Inline handleFilterChange.
just a couple of small comments below. Please feel free to go ahead and merge those changes (i.e. the branch up to this point, other than the first two commits), modulo those comments.
And the last few commits remain for me to look at later:
49bef92 UserStatusScreen [nfc]: Convert to function component.
6317a9c UserStatusScreen [nfc]: Use useSelector instead of connect.
b975b6c UserStatusScreen [nfc]: Inline setStatusTextState.
39ca278 UserStatusScreen [nfc]: Improve some variable names.
1f67982 EditStreamScreen [nfc]: Convert to function component.
11b5b2c EditStreamScreen [nfc]: Use useSelector instead of connect.
3400100 flow: Annotate some more exported React components.
| filter: '', | ||
| }; | ||
|
|
||
| componentDidMount() { |
There was a problem hiding this comment.
Naturally dissolving a TODO we added in the previous commit; simply
including `stream` in `useEffect`'s dependencies array will ensure
that a new fetch is started when the stream changes.
Cool. Probably clearest to leave the "nfc" out of this commit's summary line -- it's fixing a bug, albeit a latent one.
| const handlePress = useCallback( | ||
| (streamObj: string, topic: string) => { | ||
| // TODO: use Suspense, when available, in case of race conditions when | ||
| // fetching on changes to `stream`: | ||
| // https://github.com/zulip/zulip-mobile/pull/4906#discussion_r673557179 | ||
| // https://github.com/facebook/react/issues/14326#issuecomment-441680293 | ||
| // Currently `stream` doesn't change, so this ends up being OK. | ||
| dispatch(doNarrow(topicNarrow(stream.name, topic))); | ||
| }, |
There was a problem hiding this comment.
Hmm, I think this is actually fine.
The concern from those threads would apply here if we were taking stream, and then doing something with it that can take time, like fetching some data… and then we took the result and applied it to what we show in this component, but by the time we do so, stream may have changed so that the data we got may be an answer to an out-of-date question. We could be showing a new input value, and the output for an old input value, at the same time, so that what's on the screen is inconsistent and misleading.
But here we're not doing that. We take stream, and immediately use it to go initiate a navigation, and that's it.
When we do that, the important thing is just that the stream we use is the one the user intended -- which basically means the one that was used for rendering the UI that the user acted on.
(There is a gap in that logic, which is that the value the user actually intended to refer to is the one that was used to render the UI a few frames ago, like 30-100ms, because it takes the human eye and brain and finger that long to act. There isn't a perfect general solution to that issue, but it's an excellent reason to try to avoid jerking the UI around more than necessary, and especially right after first showing some UI, when the user may be most likely to try to interact with it.)
Anyway, so we want the stream that was used in rendering the UI the user acted on.
And that is exactly the stream that we had when we last rendered this component, because that's what gets passed down to TopicList. So using stream here is correct.
There was a problem hiding this comment.
One angle on the data-fetching race described at that upstream React thread is that we're basically completely avoiding it here by an indirection through Redux:
- The UI doesn't get the data from this component's state (which would be for a particular stream, opening up the possibility of a race where what stream that should be changes and the data doesn't get updated in sync). Instead it gets it from the global Redux state, where it looks up the data corresponding to the desired stream.
- When we fetch, we don't fetch the data into this component's state, or anything else that's about some particular stream that might be identified implicitly somewhere else. Instead we fetch it into Redux, storing it filed under whatever stream we fetched data for.
Because the data is stored in a place (our global Redux store) where it's always keyed by the stream the data corresponds to, and because the UI is always told just the stream and goes and looks up the corresponding data there, we avoid the possibility of this kind of race where the data shown in the UI isn't in sync with which stream it's supposed to be about.
There was a problem hiding this comment.
Thanks, that makes sense!
|
OK, read those remaining commits and they look good! So just the comments in my review above. |
chrisbobbe
force-pushed
the
pr-remaining-easy-useSelector
branch
from
3400100 to
a741086
Compare
|
Thanks for the review! But:
I'm actually not yet finding these spots. Would you mind looking again? |
An instance of zulip#4837.
…ct`. An instance of zulip#4837.
An instance of zulip#4837. Making sure to keep our chosen annotation for `stream`, which is looser than `getStreamInNarrow`'s return type. See b21bf43 and zulip#4520 (comment).
Like we did in 4aedc4c.
Several of the event handlers, like handlePressEditSubscribers, have `useCallback`s that are currently useless, because they're invoked in a fresh callback via `delay`. It was likewise useless to have these event handlers defined as properties on the instance, before this commit. I've left the `useCallback`s in, in this commit; as Greg says [1], this is "definitely the way to get the most direct, most clearly no-functional-change translation of the existing pattern we usually use on class components". [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.2EuseCallback/near/1226585
An instance of zulip#4837.
Naturally dissolving a TODO we added in the previous commit: simply including `stream` in `useEffect`'s dependencies array will ensure that a new fetch is started when the stream changes. Not marking as NFC because this fixes a bug, albeit a latent one.
Like we did in 4aedc4c.
Like we did with `handleFilterChange` in 4aedc4c.
This is easier to do now that we've converted these to function components in this series. This will help move us along toward Flow's new "Types-First" mode; switching entirely is zulip#4907. Prefer the `Node` type exported from React over `React$Node`. They're equivalent, but Greg prefers `Node` as it seems cleaner; see https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20types-first/near/1239060.
chrisbobbe
force-pushed
the
pr-remaining-easy-useSelector
branch
from
a741086 to
197521c
Compare
|
I've just resolved a rebase conflict. |
Hmm -- I'm not seeing them either! Not sure what I was misreading earlier. Thanks for the revision. All looks good -- merging. |
|
Thanks for the review! |
2 participants
Obviously the threshold for "easy" isn't set in stone, but for types-first (#4907) I think I'd like to try leaving the remaining exported
connected components (after this PR and #4796) intact for now, and just annotate them following Greg's suggestion here of a straightforward way to do that. Looks like there will be about a dozen of those.I guess, for #4837, we're not ready to mark it as fixed yet because the title says "all our component definitions". Ah, well, at least we'll have processed the low-hanging fruit. 🙂