From 17748d86edc04cd4ac8332ad56bf8b5cfcf9da4d Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 16 Jun 2021 12:42:07 -0400 Subject: [PATCH 01/16] MessageList: Stop accepting `typingUsers` prop from parent. 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. --- src/search/SearchMessagesCard.js | 2 -- src/webview/MessageList.js | 7 +++---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/search/SearchMessagesCard.js b/src/search/SearchMessagesCard.js index d4a8912c886..026709b143d 100644 --- a/src/search/SearchMessagesCard.js +++ b/src/search/SearchMessagesCard.js @@ -9,7 +9,6 @@ import { LoadingIndicator, SearchEmptyState } from '../common'; import { HOME_NARROW } from '../utils/narrow'; import MessageList from '../webview/MessageList'; import getHtmlPieceDescriptors from '../message/getHtmlPieceDescriptors'; -import { NULL_ARRAY } from '../nullObjects'; const styles = createStyleSheet({ results: { @@ -55,7 +54,6 @@ export default class SearchMessagesCard extends PureComponent { htmlPieceDescriptorsForShownMessages={htmlPieceDescriptors} fetching={SearchMessagesCard.NOT_FETCHING} showMessagePlaceholders={false} - typingUsers={NULL_ARRAY} /> ); diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index e38e0e485fc..b4ab29b89e2 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -309,10 +309,9 @@ type OuterProps = {| htmlPieceDescriptorsForShownMessages?: HtmlPieceDescriptor[], initialScrollMessageId?: number | null, - /* Passing these two from the parent is kind of a hack; search uses it - to hard-code some behavior. */ + /* Passing this from the parent is kind of a hack; search uses it to + hard-code some behavior. */ fetching?: Fetching, - typingUsers?: UserOrBot[], |}; export default connect((state, props: OuterProps) => { @@ -345,6 +344,6 @@ export default connect((state, props: OuterProps) => { htmlPieceDescriptorsForShownMessages: props.htmlPieceDescriptorsForShownMessages || getHtmlPieceDescriptorsForShownMessages(state, props.narrow), - typingUsers: props.typingUsers || getCurrentTypingUsers(state, props.narrow), + typingUsers: getCurrentTypingUsers(state, props.narrow), }; })(connectActionSheet(withGetText(MessageList))); From a74cb23c2c7ed67b7602a0f10abd04ec04abc52e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 16 Jun 2021 12:58:59 -0400 Subject: [PATCH 02/16] MessageList: Stop accepting `fetching` prop from parent. 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. --- src/search/SearchMessagesCard.js | 3 --- src/webview/MessageList.js | 6 +----- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/src/search/SearchMessagesCard.js b/src/search/SearchMessagesCard.js index 026709b143d..0eb70f9c9ee 100644 --- a/src/search/SearchMessagesCard.js +++ b/src/search/SearchMessagesCard.js @@ -22,8 +22,6 @@ type Props = $ReadOnly<{| |}>; export default class SearchMessagesCard extends PureComponent { - static NOT_FETCHING = { older: false, newer: false }; - render() { const { isFetching, messages } = this.props; @@ -52,7 +50,6 @@ export default class SearchMessagesCard extends PureComponent { messages={messages} narrow={HOME_NARROW} htmlPieceDescriptorsForShownMessages={htmlPieceDescriptors} - fetching={SearchMessagesCard.NOT_FETCHING} showMessagePlaceholders={false} /> diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index b4ab29b89e2..9eee8e666aa 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -308,10 +308,6 @@ type OuterProps = {| messages?: Message[], htmlPieceDescriptorsForShownMessages?: HtmlPieceDescriptor[], initialScrollMessageId?: number | null, - - /* Passing this from the parent is kind of a hack; search uses it to - hard-code some behavior. */ - fetching?: Fetching, |}; export default connect((state, props: OuterProps) => { @@ -339,7 +335,7 @@ export default connect((state, props: OuterProps) => { props.initialScrollMessageId !== undefined ? props.initialScrollMessageId : getFirstUnreadIdInNarrow(state, props.narrow), - fetching: props.fetching || getFetchingForNarrow(state, props.narrow), + fetching: getFetchingForNarrow(state, props.narrow), messages: props.messages || getShownMessagesForNarrow(state, props.narrow), htmlPieceDescriptorsForShownMessages: props.htmlPieceDescriptorsForShownMessages From 134dc953d61ec50005e2635c1e00069a23a66021 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 3 Feb 2021 13:35:10 -0500 Subject: [PATCH 03/16] ChatScreen [nfc]: Pass messages and initialScrollMessageId to MessageList. 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 --- src/chat/ChatScreen.js | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js index 23c37ec9076..8ec2ef5bf9a 100644 --- a/src/chat/ChatScreen.js +++ b/src/chat/ChatScreen.js @@ -20,6 +20,7 @@ import { canSendToNarrow } from '../utils/narrow'; import { getLoading, getSession } from '../directSelectors'; import { getFetchingForNarrow } from './fetchingSelectors'; import { getShownMessagesForNarrow, isNarrowValid as getIsNarrowValid } from './narrowsSelectors'; +import { getFirstUnreadIdInNarrow } from '../message/messageSelectors'; type Props = $ReadOnly<{| navigation: AppNavigationProp<'chat'>, @@ -42,7 +43,7 @@ const componentStyles = createStyleSheet({ * more details, including how Redux is kept up-to-date during the * whole process. */ -const useFetchMessages = args => { +const useMessagesWithFetch = args => { const { narrow } = args; const dispatch = useDispatch(); @@ -52,9 +53,9 @@ const useFetchMessages = args => { const loading = useSelector(getLoading); const fetching = useSelector(state => getFetchingForNarrow(state, narrow)); const isFetching = fetching.older || fetching.newer || loading; - const haveNoMessages = useSelector( - state => getShownMessagesForNarrow(state, narrow).length === 0, - ); + const messages = useSelector(state => getShownMessagesForNarrow(state, narrow)); + const haveNoMessages = messages.length === 0; + const firstUnreadIdInNarrow = useSelector(state => getFirstUnreadIdInNarrow(state, narrow)); // This could live in state, but then we'd risk pointless rerenders; // we only use it in our `useEffect` callbacks. Using `useRef` is @@ -93,7 +94,7 @@ const useFetchMessages = args => { // `eventQueueId` needed here because it affects `shouldFetchWhenNextFocused`. }, [isFocused, eventQueueId, fetch]); - return { fetchError, isFetching, haveNoMessages }; + return { fetchError, isFetching, messages, haveNoMessages, firstUnreadIdInNarrow }; }; export default function ChatScreen(props: Props) { @@ -106,7 +107,13 @@ export default function ChatScreen(props: Props) { const isNarrowValid = useSelector(state => getIsNarrowValid(state, narrow)); - const { fetchError, isFetching, haveNoMessages } = useFetchMessages({ narrow }); + const { + fetchError, + isFetching, + messages, + haveNoMessages, + firstUnreadIdInNarrow, + } = useMessagesWithFetch({ narrow }); const showMessagePlaceholders = haveNoMessages && isFetching; const sayNoMessages = haveNoMessages && !isFetching; @@ -128,6 +135,8 @@ export default function ChatScreen(props: Props) { return ( From 9f9dc2d84163ad57553e4e1162ff7c574a04073f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 3 Feb 2021 13:40:15 -0500 Subject: [PATCH 04/16] MessageList [nfc]: Require `messages` and `initialScrollMessageId`. 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. --- src/webview/MessageList.js | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 9eee8e666aa..c02e880b84d 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -37,13 +37,11 @@ import { getHtmlPieceDescriptorsForShownMessages, getFlags, getFetchingForNarrow, - getFirstUnreadIdInNarrow, getMute, getMutedUsers, getOwnUser, getSettings, getSubscriptions, - getShownMessagesForNarrow, getRealm, } from '../selectors'; import { withGetText } from '../boot/TranslationProvider'; @@ -93,9 +91,7 @@ type SelectorProps = {| // The remaining props contain data specific to the particular narrow or // particular messages we're displaying. Data that's independent of those // should go in `BackgroundData`, above. - initialScrollMessageId: number | null, fetching: Fetching, - messages: $ReadOnlyArray, htmlPieceDescriptorsForShownMessages: HtmlPieceDescriptor[], typingUsers: $ReadOnlyArray, |}; @@ -103,6 +99,8 @@ type SelectorProps = {| // TODO get a type for `connectActionSheet` so this gets fully type-checked. export type Props = $ReadOnly<{| narrow: Narrow, + messages: $ReadOnlyArray, + initialScrollMessageId: number | null, showMessagePlaceholders: boolean, startEditMessage: (editMessage: EditMessage) => void, @@ -301,13 +299,10 @@ class MessageList extends Component { type OuterProps = {| narrow: Narrow, + messages: $ReadOnlyArray, + initialScrollMessageId: number | null, showMessagePlaceholders: boolean, - - /* Remaining props are derived from `narrow` by default. */ - - messages?: Message[], htmlPieceDescriptorsForShownMessages?: HtmlPieceDescriptor[], - initialScrollMessageId?: number | null, |}; export default connect((state, props: OuterProps) => { @@ -331,12 +326,7 @@ export default connect((state, props: OuterProps) => { return { backgroundData, - initialScrollMessageId: - props.initialScrollMessageId !== undefined - ? props.initialScrollMessageId - : getFirstUnreadIdInNarrow(state, props.narrow), fetching: getFetchingForNarrow(state, props.narrow), - messages: props.messages || getShownMessagesForNarrow(state, props.narrow), htmlPieceDescriptorsForShownMessages: props.htmlPieceDescriptorsForShownMessages || getHtmlPieceDescriptorsForShownMessages(state, props.narrow), From 6ed1918dc00c854c039de2cb4b7ceef0be7ae4d8 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 8 Feb 2021 11:45:57 -0500 Subject: [PATCH 05/16] messagesSelectors: Add `getHtmlPieceDescriptorsForMessages`. 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. --- src/message/messageSelectors.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/message/messageSelectors.js b/src/message/messageSelectors.js index 5f4a7def85e..e05eff26103 100644 --- a/src/message/messageSelectors.js +++ b/src/message/messageSelectors.js @@ -1,7 +1,7 @@ /* @flow strict-local */ -import { createSelector } from 'reselect'; +import { createSelector, defaultMemoize } from 'reselect'; -import type { Message, Narrow, HtmlPieceDescriptor, Selector } from '../types'; +import type { Message, Outbox, Narrow, HtmlPieceDescriptor, Selector } from '../types'; import { getAllNarrows, getFlags, @@ -70,6 +70,11 @@ export const getHtmlPieceDescriptorsForShownMessages: Selector< (narrow, messages) => getHtmlPieceDescriptors(messages, narrow), ); +export const getHtmlPieceDescriptorsForMessages = defaultMemoize( + (messages: $ReadOnlyArray, narrow: Narrow) => + getHtmlPieceDescriptors(messages, narrow), +); + export const getFirstUnreadIdInNarrow: Selector = createSelector( (state, narrow) => getShownMessagesForNarrow(state, narrow), getFlags, From d7d6d3500cf61396190051add711056c46a4889f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 8 Feb 2021 12:07:39 -0500 Subject: [PATCH 06/16] MessageList [nfc]: Stop using `getHtmlPieceDescriptorsForShownMessages`. 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 --- src/message/messageSelectors.js | 11 +---------- src/webview/MessageList.js | 4 ++-- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/message/messageSelectors.js b/src/message/messageSelectors.js index e05eff26103..9ecda9d3996 100644 --- a/src/message/messageSelectors.js +++ b/src/message/messageSelectors.js @@ -1,7 +1,7 @@ /* @flow strict-local */ import { createSelector, defaultMemoize } from 'reselect'; -import type { Message, Outbox, Narrow, HtmlPieceDescriptor, Selector } from '../types'; +import type { Message, Outbox, Narrow, Selector } from '../types'; import { getAllNarrows, getFlags, @@ -61,15 +61,6 @@ export const getPrivateMessages: Selector = createSelector( }, ); -export const getHtmlPieceDescriptorsForShownMessages: Selector< - HtmlPieceDescriptor[], - Narrow, -> = createSelector( - (state, narrow) => narrow, - getShownMessagesForNarrow, - (narrow, messages) => getHtmlPieceDescriptors(messages, narrow), -); - export const getHtmlPieceDescriptorsForMessages = defaultMemoize( (messages: $ReadOnlyArray, narrow: Narrow) => getHtmlPieceDescriptors(messages, narrow), diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index c02e880b84d..3dc12982fda 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -34,7 +34,6 @@ import { getAllImageEmojiById, getCurrentTypingUsers, getDebug, - getHtmlPieceDescriptorsForShownMessages, getFlags, getFetchingForNarrow, getMute, @@ -46,6 +45,7 @@ import { } from '../selectors'; import { withGetText } from '../boot/TranslationProvider'; import type { ShowActionSheetWithOptions } from '../message/messageActionSheet'; +import { getHtmlPieceDescriptorsForMessages } from '../message/messageSelectors'; import type { WebViewInboundEvent } from './generateInboundEvents'; import type { WebViewOutboundEvent } from './handleOutboundEvents'; import getHtml from './html/html'; @@ -329,7 +329,7 @@ export default connect((state, props: OuterProps) => { fetching: getFetchingForNarrow(state, props.narrow), htmlPieceDescriptorsForShownMessages: props.htmlPieceDescriptorsForShownMessages - || getHtmlPieceDescriptorsForShownMessages(state, props.narrow), + || getHtmlPieceDescriptorsForMessages(props.messages, props.narrow), typingUsers: getCurrentTypingUsers(state, props.narrow), }; })(connectActionSheet(withGetText(MessageList))); From 66d16ab146959bcd4396d7050d1c09b3b5317257 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 8 Feb 2021 11:53:32 -0500 Subject: [PATCH 07/16] SearchMessagesCard: Use memoized function to get HTML piece descriptors. --- src/search/SearchMessagesCard.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/search/SearchMessagesCard.js b/src/search/SearchMessagesCard.js index 0eb70f9c9ee..27bded7a439 100644 --- a/src/search/SearchMessagesCard.js +++ b/src/search/SearchMessagesCard.js @@ -8,7 +8,7 @@ import { createStyleSheet } from '../styles'; import { LoadingIndicator, SearchEmptyState } from '../common'; import { HOME_NARROW } from '../utils/narrow'; import MessageList from '../webview/MessageList'; -import getHtmlPieceDescriptors from '../message/getHtmlPieceDescriptors'; +import { getHtmlPieceDescriptorsForMessages } from '../message/messageSelectors'; const styles = createStyleSheet({ results: { @@ -41,7 +41,7 @@ export default class SearchMessagesCard extends PureComponent { return ; } - const htmlPieceDescriptors = getHtmlPieceDescriptors(messages, HOME_NARROW); + const htmlPieceDescriptors = getHtmlPieceDescriptorsForMessages(messages, HOME_NARROW); return ( From f803542910de5e4bb0f1294bb35a7ed4aed8f31c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 8 Feb 2021 11:56:09 -0500 Subject: [PATCH 08/16] SearchMessagesCard [nfc]: Inline `htmlPieceDescriptors`. --- src/search/SearchMessagesCard.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/search/SearchMessagesCard.js b/src/search/SearchMessagesCard.js index 27bded7a439..acd157ebad9 100644 --- a/src/search/SearchMessagesCard.js +++ b/src/search/SearchMessagesCard.js @@ -41,15 +41,19 @@ export default class SearchMessagesCard extends PureComponent { return ; } - const htmlPieceDescriptors = getHtmlPieceDescriptorsForMessages(messages, HOME_NARROW); + // TODO: This is kind of a hack. + const narrow = HOME_NARROW; return ( From 13c91b6ac263fb93084eeada7ff667cb6e48cc50 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 8 Feb 2021 12:57:29 -0500 Subject: [PATCH 09/16] MessageList [nfc]: Express a conditional a different way. --- src/webview/MessageList.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 3dc12982fda..0f733caf5ef 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -327,9 +327,9 @@ export default connect((state, props: OuterProps) => { return { backgroundData, fetching: getFetchingForNarrow(state, props.narrow), - htmlPieceDescriptorsForShownMessages: - props.htmlPieceDescriptorsForShownMessages - || getHtmlPieceDescriptorsForMessages(props.messages, props.narrow), + htmlPieceDescriptorsForShownMessages: props.htmlPieceDescriptorsForShownMessages + ? props.htmlPieceDescriptorsForShownMessages + : getHtmlPieceDescriptorsForMessages(props.messages, props.narrow), typingUsers: getCurrentTypingUsers(state, props.narrow), }; })(connectActionSheet(withGetText(MessageList))); From f7519f2494d560c15a0614c7e19f88c8c35f8086 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 16 Jun 2021 13:24:36 -0400 Subject: [PATCH 10/16] SearchMessagesCard [nfc]: Don't pass htmlPieceDescriptorsForShownMessages. 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. --- src/search/SearchMessagesCard.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/search/SearchMessagesCard.js b/src/search/SearchMessagesCard.js index acd157ebad9..d88c2256310 100644 --- a/src/search/SearchMessagesCard.js +++ b/src/search/SearchMessagesCard.js @@ -8,7 +8,6 @@ import { createStyleSheet } from '../styles'; import { LoadingIndicator, SearchEmptyState } from '../common'; import { HOME_NARROW } from '../utils/narrow'; import MessageList from '../webview/MessageList'; -import { getHtmlPieceDescriptorsForMessages } from '../message/messageSelectors'; const styles = createStyleSheet({ results: { @@ -50,10 +49,6 @@ export default class SearchMessagesCard extends PureComponent { initialScrollMessageId={messages[0].id} messages={messages} narrow={narrow} - htmlPieceDescriptorsForShownMessages={getHtmlPieceDescriptorsForMessages( - messages, - narrow, - )} showMessagePlaceholders={false} /> From c655202fac45c90bd0f155c548021e3ce0d945ae Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 16 Jun 2021 13:27:29 -0400 Subject: [PATCH 11/16] MessageList [nfc]: Remove outer prop htmlPieceDescriptorsForShownMessages. We stopped passing this at the only callsite that had been passing it, in the previous commit. --- src/webview/MessageList.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 0f733caf5ef..4f4d196f52d 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -302,7 +302,6 @@ type OuterProps = {| messages: $ReadOnlyArray, initialScrollMessageId: number | null, showMessagePlaceholders: boolean, - htmlPieceDescriptorsForShownMessages?: HtmlPieceDescriptor[], |}; export default connect((state, props: OuterProps) => { @@ -327,9 +326,10 @@ export default connect((state, props: OuterProps) => { return { backgroundData, fetching: getFetchingForNarrow(state, props.narrow), - htmlPieceDescriptorsForShownMessages: props.htmlPieceDescriptorsForShownMessages - ? props.htmlPieceDescriptorsForShownMessages - : getHtmlPieceDescriptorsForMessages(props.messages, props.narrow), + htmlPieceDescriptorsForShownMessages: getHtmlPieceDescriptorsForMessages( + props.messages, + props.narrow, + ), typingUsers: getCurrentTypingUsers(state, props.narrow), }; })(connectActionSheet(withGetText(MessageList))); From 923f6336f1570dc2cd34f229ca93fe77792cb0fe Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 3 Feb 2021 14:14:12 -0500 Subject: [PATCH 12/16] MessageList types: Copy `startEditMessage` to `OuterProps`, where it 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. --- src/webview/MessageList.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 4f4d196f52d..ae617b760fa 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -302,6 +302,7 @@ type OuterProps = {| messages: $ReadOnlyArray, initialScrollMessageId: number | null, showMessagePlaceholders: boolean, + startEditMessage: (editMessage: EditMessage) => void, |}; export default connect((state, props: OuterProps) => { From 932173a895f7a62306b095f4d92b83b771515226 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 3 Feb 2021 11:27:32 -0500 Subject: [PATCH 13/16] action sheet: Add type wrapper for `connectActionSheet`. `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 8f569641f, use a type-wrapper instead of a libdef so that we can use our handy `BoundedDiff` type. --- src/react-native-action-sheet.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 src/react-native-action-sheet.js diff --git a/src/react-native-action-sheet.js b/src/react-native-action-sheet.js new file mode 100644 index 00000000000..16ec503fc19 --- /dev/null +++ b/src/react-native-action-sheet.js @@ -0,0 +1,29 @@ +/* @flow strict-local */ +import type { ComponentType, ElementConfig } from 'react'; +import { connectActionSheet as connectActionSheetInner } from '@expo/react-native-action-sheet'; + +import type { BoundedDiff } from './generics'; + +/* eslint-disable flowtype/generic-spacing */ + +export type ShowActionSheetWithOptions = ( + { options: string[], cancelButtonIndex: number }, + (number) => void, +) => void; + +/** + * Exactly like the `connectActionSheet` in + * `react-native-action-sheet` upstream, but more typed. + */ +export function connectActionSheet>( + WrappedComponent: C, +): ComponentType< + $ReadOnly< + BoundedDiff< + $Exact>, + {| showActionSheetWithOptions: ShowActionSheetWithOptions |}, + >, + >, +> { + return connectActionSheetInner(WrappedComponent); +} From 767fbf934464a1b9f8969aa730fd05d0cd37e551 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 3 Feb 2021 14:38:23 -0500 Subject: [PATCH 14/16] MessageList: Use our type-wrapper for `connectActionSheet`. 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. --- src/webview/MessageList.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index ae617b760fa..3e1fe13c32d 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -3,8 +3,8 @@ import React, { Component } from 'react'; import { Platform, NativeModules } from 'react-native'; import { WebView } from 'react-native-webview'; import type { WebViewNavigation } from 'react-native-webview'; -import { connectActionSheet } from '@expo/react-native-action-sheet'; +import { connectActionSheet } from '../react-native-action-sheet'; import type { AlertWordsState, Auth, @@ -96,7 +96,6 @@ type SelectorProps = {| typingUsers: $ReadOnlyArray, |}; -// TODO get a type for `connectActionSheet` so this gets fully type-checked. export type Props = $ReadOnly<{| narrow: Narrow, messages: $ReadOnlyArray, From ac97c91cb8de468112ae01077786965d572b1866 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 3 Feb 2021 14:42:53 -0500 Subject: [PATCH 15/16] MessageList: Fix type-checking of props. This is the problem we've run into a few times [1] [2] where adding `() => ;` 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`, which Greg noticed also worked [3], since we happen to have `OuterProps` lying around anyway. [1] fcde7005e [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 --- src/search/SearchMessagesCard.js | 3 ++ src/webview/MessageList.js | 64 +++++++++++++++++--------------- 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/src/search/SearchMessagesCard.js b/src/search/SearchMessagesCard.js index d88c2256310..8380f7aa36f 100644 --- a/src/search/SearchMessagesCard.js +++ b/src/search/SearchMessagesCard.js @@ -50,6 +50,9 @@ export default class SearchMessagesCard extends PureComponent { messages={messages} narrow={narrow} showMessagePlaceholders={false} + // TODO: handle editing a message from the search results, + // or make this prop optional + startEditMessage={() => undefined} /> ); diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 3e1fe13c32d..97784564ea0 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -1,5 +1,5 @@ /* @flow strict-local */ -import React, { Component } from 'react'; +import React, { Component, type ComponentType } from 'react'; import { Platform, NativeModules } from 'react-native'; import { WebView } from 'react-native-webview'; import type { WebViewNavigation } from 'react-native-webview'; @@ -142,7 +142,7 @@ const assetsUrl = */ const webviewAssetsUrl = new URL('webview/', assetsUrl); -class MessageList extends Component { +class MessageListInner extends Component { static contextType = ThemeContext; context: ThemeData; @@ -304,32 +304,36 @@ type OuterProps = {| startEditMessage: (editMessage: EditMessage) => void, |}; -export default connect((state, props: OuterProps) => { - // TODO Ideally this ought to be a caching selector that doesn't change - // when the inputs don't. Doesn't matter in a practical way here, because - // we have a `shouldComponentUpdate` that doesn't look at this prop... but - // it'd be better to set an example of the right general pattern. - const backgroundData: BackgroundData = { - alertWords: state.alertWords, - allImageEmojiById: getAllImageEmojiById(state), - auth: getAuth(state), - debug: getDebug(state), - flags: getFlags(state), - mute: getMute(state), - mutedUsers: getMutedUsers(state), - ownUser: getOwnUser(state), - subscriptions: getSubscriptions(state), - theme: getSettings(state).theme, - twentyFourHourTime: getRealm(state).twentyFourHourTime, - }; +const MessageList: ComponentType = connect( + (state, props: OuterProps) => { + // TODO Ideally this ought to be a caching selector that doesn't change + // when the inputs don't. Doesn't matter in a practical way here, because + // we have a `shouldComponentUpdate` that doesn't look at this prop... but + // it'd be better to set an example of the right general pattern. + const backgroundData: BackgroundData = { + alertWords: state.alertWords, + allImageEmojiById: getAllImageEmojiById(state), + auth: getAuth(state), + debug: getDebug(state), + flags: getFlags(state), + mute: getMute(state), + mutedUsers: getMutedUsers(state), + ownUser: getOwnUser(state), + subscriptions: getSubscriptions(state), + theme: getSettings(state).theme, + twentyFourHourTime: getRealm(state).twentyFourHourTime, + }; - return { - backgroundData, - fetching: getFetchingForNarrow(state, props.narrow), - htmlPieceDescriptorsForShownMessages: getHtmlPieceDescriptorsForMessages( - props.messages, - props.narrow, - ), - typingUsers: getCurrentTypingUsers(state, props.narrow), - }; -})(connectActionSheet(withGetText(MessageList))); + return { + backgroundData, + fetching: getFetchingForNarrow(state, props.narrow), + htmlPieceDescriptorsForShownMessages: getHtmlPieceDescriptorsForMessages( + props.messages, + props.narrow, + ), + typingUsers: getCurrentTypingUsers(state, props.narrow), + }; + }, +)(connectActionSheet(withGetText(MessageListInner))); + +export default MessageList; From 21fd83a53ed8b3e4809834413535436acf901c3c Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 16 Jun 2021 13:37:20 -0400 Subject: [PATCH 16/16] MessageList [nfc]: Spread `OuterProps` in `Props`, to remove repeated 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 https://github.com/zulip/zulip-mobile/pull/4465/commits/3bdb5fbc430a10a78849a33a4bedfad7259c1bb4#r652236395. --- src/webview/MessageList.js | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 97784564ea0..8bd402080c0 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -84,6 +84,14 @@ export type BackgroundData = $ReadOnly<{| twentyFourHourTime: boolean, |}>; +type OuterProps = {| + narrow: Narrow, + messages: $ReadOnlyArray, + initialScrollMessageId: number | null, + showMessagePlaceholders: boolean, + startEditMessage: (editMessage: EditMessage) => void, +|}; + type SelectorProps = {| // Data independent of the particular narrow or messages we're displaying. backgroundData: BackgroundData, @@ -97,11 +105,7 @@ type SelectorProps = {| |}; export type Props = $ReadOnly<{| - narrow: Narrow, - messages: $ReadOnlyArray, - initialScrollMessageId: number | null, - showMessagePlaceholders: boolean, - startEditMessage: (editMessage: EditMessage) => void, + ...OuterProps, dispatch: Dispatch, ...SelectorProps, @@ -296,14 +300,6 @@ class MessageListInner extends Component { } } -type OuterProps = {| - narrow: Narrow, - messages: $ReadOnlyArray, - initialScrollMessageId: number | null, - showMessagePlaceholders: boolean, - startEditMessage: (editMessage: EditMessage) => void, -|}; - const MessageList: ComponentType = connect( (state, props: OuterProps) => { // TODO Ideally this ought to be a caching selector that doesn't change