Start type checking `MessageList`'s props. #4465

chrisbobbe · 2021-02-03T20:14:28Z

By adding a type-wrapper for connectActionSheet, and annotating the export of MessageList.js with ComponentType<OuterProps>, as @gnprice mentioned in a recent discussion of Flow and connect.

I've removed the kind of hacky cases where we use the same name for an "outer" prop as we use for a prop that connect provides. Let me know if you see a better way to do this, and in particular, if you don't like the changes to MessageList's interface. 🙂

Also, in this one—

MessageList [nfc]: Remove htmlPieceDescriptorsForShownMessages prop (3/x).

—there's a potential performance regression, where we stop using a caching selector. After I'd removed that, it occurred to me that some of the work it caches (calling getHtmlPieceDescriptors on each shown message) is enough that we'll want to keep the caching and find something else to do about the htmlPieceDescriptorsForShownMessages prop. What do you think?

gnprice · 2021-02-05T06:37:09Z

Also, in this one—

MessageList [nfc]: Remove htmlPieceDescriptorsForShownMessages prop (3/x).

—there's a potential performance regression, where we stop using a caching selector. After I'd removed that, it occurred to me that some of the work it caches (calling getHtmlPieceDescriptors on each shown message) is enough that we'll want to keep the caching and find something else to do about the htmlPieceDescriptorsForShownMessages prop. What do you think?

Yeah, I think we'll want to keep that work memoized.

There's likely a related perf regression at
c7487a2 MessageList [nfc]: Remove htmlPieceDescriptorsForShownMessages prop (6/x).
where instead of having that data as a prop, it becomes something that gets recomputed in several different places and potentially several times.

Oh, I guess two of those get removed a couple of commits later. 🙂 That mitigates it. Still, should be memoized so that we only compute it once each time the inputs change.

One thing I notice from this refactoring is that in the old code, we aren't getting memoization in the search case! Each time the SearchMessagesCard got rerendered, for any reason, it would go recompute htmlPieceDescriptors from scratch. So one benefit of centralizing this logic in MessageList, as this branch does, is that we can be sure to consistently memoize.

chrisbobbe · 2021-02-05T21:12:44Z

Yeah, I think we'll want to keep that work memoized.

Makes sense.

Looking at how best to memoize the work done in getHtmlPieceDescriptors, I notice that getHtmlPieceDescriptors asks for some messages and a narrow, as inputs. With MessageList newly requiring props.narrow and props.messages, I think it makes sense to pass those. Which means…we don't need to use state, as it's passed to mapStateToProps. And that means we're not tied to using a caching selector with help from reselect; in fact, I think it would look a bit odd to use one.

Does that sound right? Can we, e.g., use useMemo inside mapStateToProps?

chrisbobbe · 2021-02-08T18:02:49Z

Thanks, @gnprice! New revision pushed.

chrisbobbe · 2021-03-10T18:59:27Z

@gnprice, I've just rebased this past #4518. Thanks for reviewing #4518 so quickly!

Getting MessageList's prop type-checking to work would be really helpful for the next part in my Flow-related work toward the RN v0.64 upgrade (#4426). Quoting from #4465's description at #4518 (comment):

I have a draft on top of this that goes further toward #3452. The "implicit-inexact-object" lint Greg mentions at #3452 (comment) finds 192 warnings (142 in our own code, the rest in libdefs) at the current master (before this PR). At the tip of that draft in its current state, it finds just 54 warnings, with 19 in our own code, and those numbers are decreasing 😄.

I can be more confident in some parts of that draft after a PR like the current one is merged. 🙂

An instance of zulip#3452. PR zulip#4465 is currently open for type-checking `MessageList`'s props.

chrisbobbe · 2021-04-16T23:28:34Z

Just fixed some rebase conflicts. 🙂

chrisbobbe · 2021-05-10T18:09:04Z

Just resolved some rebase conflicts. 🙂

gnprice

Thanks @chrisbobbe ! This looks great -- sorry for the delayed review. Comments below.

The part about OuterProps is a potential followup, plus some backstory FYI; any refactoring around startEditMessage (i.e., beyond tweaking the TODO comment) is also a potential followup. Everything else should be pretty straightforward to take care of.

src/webview/MessageList.js

src/react-native-action-sheet.js

gnprice · 2021-06-15T23:55:39Z

src/search/SearchMessagesCard.js

+          // TODO: handle editing a message from the search results,
+          // or don't let callers pass this.
+          startEditMessage={() => undefined}


What do you have in mind for "don't let callers pass this"? The two natural options I see here are

handle editing a message from the search results, or

make this optional.

Probably we don't want to try to handle editing from here, so that'd point to the latter. We already have this where we construct the action sheet:

if ( message.sender_id === ownUser.user_id // Our "edit message" UI only works in certain kinds of narrows. && (isStreamOrTopicNarrow(narrow) || isPmNarrow(narrow)) ) { buttons.push(editMessage); }

Perhaps that filter on the narrow could move up to ChatScreen, and control whether we pass a non-void startEditMessage callback at all? (And here in search, just never pass it.) Then in the action sheet we could just check if the callback is truthy.

don't let callers pass this

I think I meant to say "don't require callers to pass this".

Perhaps that filter on the narrow could move up to ChatScreen, and control whether we pass a non-void startEditMessage callback at all? (And here in search, just never pass it.) Then in the action sheet we could just check if the callback is truthy.

Yeah; just this week, it's come up that it'd be nice to do this conditional in just one place: #4799 (comment).

To check if the callback is truthy, in the action sheet, are we OK with constructMessageActionButtons taking startEditMessage as another param? Currently it looks like this:

export const constructMessageActionButtons = ({ backgroundData: { ownUser, flags }, message, narrow, }: { backgroundData: $ReadOnly<{ ownUser: User, flags: FlagsState, ... }>, message: Message, narrow: Narrow, }): Button<MessageArgs>[] => { // ... }

Hmm, yeah, it is a little awkward to pass it down there (through constructNonHeaderActionButtons, too. But I think it's OK.

react-redux was injecting a `typingUsers` prop that shadowed this one, which can cause confusion. Rather than renaming the outer prop, just remove it. This gives the same behavior for current callers: `getCurrentTypingUsers` appropriately contains a sanity check that will return an empty array when the narrow can't have meaningful typing state, i.e., when it's not a "conversation" narrow (a PM narrow or topic narrow) [1]. Search narrows can't have meaningful typing state, and `MessageList`'s only callsite that passed this prop was for search narrows. [1] In fact, it doesn't include topic narrows; we may want to change that at some point.

react-redux was injecting a `fetching` prop that shadowed this one, which can cause confusion. Rather than renaming the outer prop, just remove it. This gives the same behavior for current callers: the one caller that passed this prop is for search narrows, and we don't store fetching state in Redux for search narrows. So we'd go look up the search narrow in the fetching state, not find it, and default to a not-fetching value. This seems better: if, in the future, we do want to start fetching when you scroll to the top or bottom of a search narrow, we'd want that to get indicated, just the same way as we do for other narrows.

…List. Now, both callers of `MessageList` pass these props, so we're set up to have `MessageList` require them. Put the getting of `messages` and `firstUnreadIdInNArrow` in our custom Hook, to help distill the lines of `ChatScreen` down to things that are about UI choices, like the getting of `sayNoMessages`, `showComposeBox`, and the JSX [1]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.20Navigation.20v5.3A.20.60ChatScreen.60.20and.20Hooks/near/1064349

This change in `MessageList`'s interface might be seen as stretching a bit far to accommodate the search screen's peculiar choices. This is partly fair, but also, the choice to keep reusing a widget in two important places means we should concede, to some extent, that one use isn't peculiar just because it's different from the other.

Like `getHtmlPieceDescriptorsForShownMessages`, but it takes an array of messages instead of picking one from the state. ...Which means, the state is no longer needed as an input. So, this one doesn't really end up being a selector. We do want memoization, though -- so, use `defaultMemoize`. That's the memoize function used by `createSelector` [1]. [1] https://www.npmjs.com/package/reselect#defaultmemoizefunc-equalitycheck--defaultequalitycheck.

The caller that doesn't pass the `htmlPieceDescriptorsForShownMessages` prop uses the same calculation to get the list of messages as `getHtmlPieceDescriptorsForShownMessages` was using. Namely it uses the result of `getShownMessagesForNarrow(state, narrow)`. Also, memoization is done the same way as it was being done with `createSelector` [1]. [1] https://www.npmjs.com/package/reselect#defaultmemoizefunc-equalitycheck--defaultequalitycheck

…ages. This is NFC because the falsy branch of the conditional on this prop in `MessageList`'s `connect`'s `mapStateToProps` computes the same thing we had been passing.

…ages. We stopped passing this at the only callsite that had been passing it, in the previous commit.

…belongs. Now, `OuterProps` matches the subset of `Props` that callers are meant to pass. If `OuterProps` must exist, this is what it should look like.

`connectActionSheet`'s poor typing is one thing stopping `MessageList`'s props from being type checked. Like we did for react-native-safe-area-context in 8f56964, use a type-wrapper instead of a libdef so that we can use our handy `BoundedDiff` type.

This will get us part of the way toward type-checking `MessageList`'s interface. It doesn't get us all of the way there; that'll be done in the next commit.

This is the problem we've run into a few times [1] [2] where adding `() => <C />;` to the file magically causes C's callers even *outside* the file to start getting their props type-checked, where they weren't before. Instead of doing that, annotate the export with `ComponentType<OuterProps>`, which Greg noticed also worked [3], since we happen to have `OuterProps` lying around anyway. [1] fcde700 [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20and.20.60connect.60/near/1098203 [3] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20and.20.60connect.60/near/1098230

… code. It turns out that it's handy to have `OuterProps`, for the work done in the previous commit. But at least we can remove some repetition by doing this. See also discussion at zulip@3bdb5fb#r652236395.

chrisbobbe · 2021-06-16T19:54:18Z

Thanks for the review! Revision pushed.

gnprice · 2021-06-16T20:30:13Z

Thanks, looks good! Merged.

There's one open thread about a possible followup: #4465 (comment)

chrisbobbe · 2021-06-16T20:52:11Z

Thanks for the review!

chrisbobbe requested a review from gnprice February 3, 2021 20:14

chrisbobbe force-pushed the pr-type-msglist-props branch from 5a31e7e to 739c0db Compare February 8, 2021 18:01

chrisbobbe mentioned this pull request Feb 26, 2021

messageActionSheet : using Modal instead of @expo/react-native-action-sheet #4490

Closed

chrisbobbe force-pushed the pr-type-msglist-props branch from 739c0db to 3013d77 Compare March 10, 2021 18:54

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 11, 2021

types: Make a few Props definitions explicitly inexact.

0f9d71f

An instance of zulip#3452. PR zulip#4465 is currently open for type-checking `MessageList`'s props.

chrisbobbe force-pushed the pr-type-msglist-props branch from 3013d77 to 31feae8 Compare April 16, 2021 23:28

chrisbobbe mentioned this pull request Apr 22, 2021

msglist [nfc]: Warn about passing non-corresponding messages and rend… #3923

Closed

chrisbobbe force-pushed the pr-type-msglist-props branch from 31feae8 to d939fd0 Compare May 10, 2021 18:08

chrisbobbe added the P1 high-priority label Jun 3, 2021

gnprice reviewed Jun 15, 2021

View reviewed changes

chrisbobbe added 14 commits June 16, 2021 13:40

SearchMessagesCard: Use memoized function to get HTML piece descriptors.

66d16ab

SearchMessagesCard [nfc]: Inline htmlPieceDescriptors.

f803542

MessageList [nfc]: Express a conditional a different way.

13c91b6

SearchMessagesCard [nfc]: Don't pass htmlPieceDescriptorsForShownMess…

f7519f2

…ages. This is NFC because the falsy branch of the conditional on this prop in `MessageList`'s `connect`'s `mapStateToProps` computes the same thing we had been passing.

MessageList [nfc]: Remove outer prop htmlPieceDescriptorsForShownMess…

c655202

…ages. We stopped passing this at the only callsite that had been passing it, in the previous commit.

MessageList types: Copy startEditMessage to OuterProps, where it …

923f633

…belongs. Now, `OuterProps` matches the subset of `Props` that callers are meant to pass. If `OuterProps` must exist, this is what it should look like.

MessageList: Use our type-wrapper for connectActionSheet.

767fbf9

This will get us part of the way toward type-checking `MessageList`'s interface. It doesn't get us all of the way there; that'll be done in the next commit.

chrisbobbe added 2 commits June 16, 2021 15:41

MessageList [nfc]: Spread OuterProps in Props, to remove repeated…

21fd83a

… code. It turns out that it's handy to have `OuterProps`, for the work done in the previous commit. But at least we can remove some repetition by doing this. See also discussion at zulip@3bdb5fb#r652236395.

chrisbobbe force-pushed the pr-type-msglist-props branch from d939fd0 to 21fd83a Compare June 16, 2021 19:54

gnprice merged commit 21fd83a into zulip:master Jun 16, 2021

chrisbobbe deleted the pr-type-msglist-props branch November 4, 2021 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start type checking `MessageList`'s props. #4465

Start type checking `MessageList`'s props. #4465

chrisbobbe commented Feb 3, 2021

gnprice commented Feb 5, 2021

chrisbobbe commented Feb 5, 2021 •

edited

Loading

chrisbobbe commented Feb 8, 2021

chrisbobbe commented Mar 10, 2021 •

edited

Loading

chrisbobbe commented Apr 16, 2021

chrisbobbe commented May 10, 2021

gnprice left a comment •

edited

Loading

gnprice Jun 15, 2021

chrisbobbe Jun 16, 2021

chrisbobbe Jun 16, 2021 •

edited

Loading

gnprice Jun 16, 2021

chrisbobbe commented Jun 16, 2021

gnprice commented Jun 16, 2021

chrisbobbe commented Jun 16, 2021

Start type checking MessageList's props. #4465

Start type checking MessageList's props. #4465

Conversation

chrisbobbe commented Feb 3, 2021

gnprice commented Feb 5, 2021

chrisbobbe commented Feb 5, 2021 • edited Loading

chrisbobbe commented Feb 8, 2021

chrisbobbe commented Mar 10, 2021 • edited Loading

chrisbobbe commented Apr 16, 2021

chrisbobbe commented May 10, 2021

gnprice left a comment • edited Loading

Choose a reason for hiding this comment

gnprice Jun 15, 2021

Choose a reason for hiding this comment

chrisbobbe Jun 16, 2021

Choose a reason for hiding this comment

chrisbobbe Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

gnprice Jun 16, 2021

Choose a reason for hiding this comment

chrisbobbe commented Jun 16, 2021

gnprice commented Jun 16, 2021

chrisbobbe commented Jun 16, 2021

Start type checking `MessageList`'s props. #4465

Start type checking `MessageList`'s props. #4465

chrisbobbe commented Feb 5, 2021 •

edited

Loading

chrisbobbe commented Mar 10, 2021 •

edited

Loading

gnprice left a comment •

edited

Loading

chrisbobbe Jun 16, 2021 •

edited

Loading