From 35886cb63f1ee4b38a5fe5a023eaa57dfbc22a77 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 18 Oct 2022 22:46:14 -0700 Subject: [PATCH 01/12] react [nfc]: Use PureComponent where we say we do In docs/architecture/react.md we say we derive all our class components from React.PureComponent, with one exception, namely MessageList. From a grep, there are a handful of other components where we use simply React.Component instead. I'm not sure the difference has much practical effect on those components, but adjust them to follow our pattern for consistency. None of these components need special behavior in this respect: they don't implement shouldComponentUpdate, and just like for other components in our app the values we pass for their props are never mutated, only replaced with new values. --- src/RootErrorBoundary.js | 2 +- src/sharing/ShareToPm.js | 2 +- src/sharing/ShareToStream.js | 2 +- src/sharing/ShareWrapper.js | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/RootErrorBoundary.js b/src/RootErrorBoundary.js index 8e1f0f5cf55..8f541b46998 100644 --- a/src/RootErrorBoundary.js +++ b/src/RootErrorBoundary.js @@ -34,7 +34,7 @@ type State = $ReadOnly<{| * * [1] https://reactjs.org/docs/error-boundaries.html#how-about-event-handlers */ -export default class ErrorBoundary extends React.Component { +export default class ErrorBoundary extends React.PureComponent { static getDerivedStateFromError(error: Error): State { return { error }; } diff --git a/src/sharing/ShareToPm.js b/src/sharing/ShareToPm.js index caaacf38504..62a8fe2f42e 100644 --- a/src/sharing/ShareToPm.js +++ b/src/sharing/ShareToPm.js @@ -37,7 +37,7 @@ type State = $ReadOnly<{| choosingRecipients: boolean, |}>; -export default class ShareToPm extends React.Component { +export default class ShareToPm extends React.PureComponent { state: State = { selectedRecipients: [], choosingRecipients: false, diff --git a/src/sharing/ShareToStream.js b/src/sharing/ShareToStream.js index 3c0208c259b..22b881e9003 100644 --- a/src/sharing/ShareToStream.js +++ b/src/sharing/ShareToStream.js @@ -47,7 +47,7 @@ type State = $ReadOnly<{| isTopicFocused: boolean, |}>; -class ShareToStreamInner extends React.Component { +class ShareToStreamInner extends React.PureComponent { state = { streamName: '', streamId: null, diff --git a/src/sharing/ShareWrapper.js b/src/sharing/ShareWrapper.js index 71358603e2f..ab9b7bc74cc 100644 --- a/src/sharing/ShareWrapper.js +++ b/src/sharing/ShareWrapper.js @@ -98,7 +98,7 @@ type State = $ReadOnly<{| * Wraps Around different sharing screens, * for minimal duplication of code. */ -class ShareWrapperInner extends React.Component { +class ShareWrapperInner extends React.PureComponent { static contextType = TranslationContext; context: GetText; From 2fb9841bb566b1597bf2595ec3cced200c4b9561 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 18 Oct 2022 23:07:01 -0700 Subject: [PATCH 02/12] react [nfc]: Fix a reference to "legacy context API" The API this is referring to is actually the modern context API, just the class-component form of it. The "legacy context API" is something else, involving a static property `contextTypes`. We haven't used that one for anything since commit 30e4d19cc, PR #4199, back in 2020-07. Also mention what's now the most common alternative to withGetText: the useContext hook. --- src/boot/TranslationProvider.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/boot/TranslationProvider.js b/src/boot/TranslationProvider.js index 7454a74f8ab..4ab42f03b65 100644 --- a/src/boot/TranslationProvider.js +++ b/src/boot/TranslationProvider.js @@ -16,9 +16,10 @@ export const TranslationContext: Context = React.createContext(undefine /** * Provide `_` to the wrapped component, passing other props through. * - * This is useful when the component is already using its `context` property - * for the legacy context API. When that isn't the case, simply saying - * `context: TranslationContext` may be more convenient. + * This can be useful when the component is already using its `context` + * property for a different context provider. When that isn't the case, + * simply saying `context: TranslationContext` may be more convenient. + * Or in a function component, `const _ = useContext(TranslationContext);`. */ export function withGetText>( WrappedComponent: C, From 09d538130e75ac302a5093c01b82d71c5f905bff Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 17 Oct 2022 14:16:40 -0700 Subject: [PATCH 03/12] msglist [nfc]: Use actual MessageList props type in handleOutboundEvents We got this `Props` type properly type-checked with ac97c91cb and the series around that. --- src/webview/handleOutboundEvents.js | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index c74a1592b8d..027120eab10 100644 --- a/src/webview/handleOutboundEvents.js +++ b/src/webview/handleOutboundEvents.js @@ -4,9 +4,7 @@ import { Clipboard, Alert } from 'react-native'; import * as NavigationService from '../nav/NavigationService'; import * as api from '../api'; import config from '../config'; -import type { Dispatch, GetText, Message, Narrow, Outbox, EditMessage, UserId } from '../types'; -import type { BackgroundData } from './backgroundData'; -import type { ShowActionSheetWithOptions } from '../action-sheets'; +import type { GetText, UserId } from '../types'; import type { JSONableDict } from '../utils/jsonable'; import { showToast } from '../utils/info'; import { pmKeyRecipientsFromMessage } from '../utils/recipient'; @@ -30,6 +28,7 @@ import { } from '../action-sheets'; import { ensureUnreachable } from '../types'; import { base64Utf8Decode } from '../utils/encoding'; +import type { Props } from './MessageList'; type WebViewOutboundEventReady = {| type: 'ready', @@ -159,19 +158,6 @@ export type WebViewOutboundEvent = | WebViewOutboundEventTimeDetails | WebViewOutboundEventVote; -// TODO: Consider completing this and making it exact, once -// `MessageList`'s props are type-checked. -type Props = $ReadOnly<{ - backgroundData: BackgroundData, - dispatch: Dispatch, - messages: $ReadOnlyArray, - narrow: Narrow, - doNotMarkMessagesAsRead: boolean, - showActionSheetWithOptions: ShowActionSheetWithOptions, - startEditMessage: (editMessage: EditMessage) => void, - ... -}>; - const fetchMore = (props: Props, event: WebViewOutboundEventScroll) => { const { innerHeight, offsetHeight, scrollY } = event; const { dispatch, narrow } = props; From 4f4b62f2610455b065e7f78e291bccd4fde1f6ad Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 17 Oct 2022 14:25:39 -0700 Subject: [PATCH 04/12] msglist [nfc]: Simplify by getting `_` from props more No need to pass it around separately where we're already passing the whole props object. --- src/webview/MessageList.js | 3 +-- src/webview/handleOutboundEvents.js | 16 +++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 70a9b8eedd5..b3e48733df2 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -152,8 +152,7 @@ class MessageListInner extends React.Component { this.sendInboundEvents([{ type: 'ready' }, ...this.unsentInboundEvents]); this.unsentInboundEvents = []; } else { - const { _ } = this.props; - handleWebViewOutboundEvent(this.props, _, eventData); + handleWebViewOutboundEvent(this.props, eventData); } }; diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index 027120eab10..cf0f53967e9 100644 --- a/src/webview/handleOutboundEvents.js +++ b/src/webview/handleOutboundEvents.js @@ -4,7 +4,7 @@ import { Clipboard, Alert } from 'react-native'; import * as NavigationService from '../nav/NavigationService'; import * as api from '../api'; import config from '../config'; -import type { GetText, UserId } from '../types'; +import type { UserId } from '../types'; import type { JSONableDict } from '../utils/jsonable'; import { showToast } from '../utils/info'; import { pmKeyRecipientsFromMessage } from '../utils/recipient'; @@ -192,7 +192,6 @@ const handleImage = (props: Props, src: string, messageId: number) => { const handleLongPress = ( props: Props, - _: GetText, target: 'message' | 'header' | 'link', messageId: number, href: string | null, @@ -200,6 +199,7 @@ const handleLongPress = ( if (href !== null) { const url = new URL(href, props.backgroundData.auth.realm).toString(); Clipboard.setString(url); + const { _ } = props; showToast(_('Link copied')); return; } @@ -208,7 +208,8 @@ const handleLongPress = ( if (!message) { return; } - const { dispatch, showActionSheetWithOptions, backgroundData, narrow, startEditMessage } = props; + const { dispatch, showActionSheetWithOptions, backgroundData, narrow, startEditMessage, _ } = + props; if (target === 'header') { if (message.type === 'stream') { showTopicActionSheet({ @@ -237,11 +238,7 @@ const handleLongPress = ( } }; -export const handleWebViewOutboundEvent = ( - props: Props, - _: GetText, - event: WebViewOutboundEvent, -) => { +export const handleWebViewOutboundEvent = (props: Props, event: WebViewOutboundEvent) => { switch (event.type) { case 'ready': // handled by caller @@ -266,7 +263,7 @@ export const handleWebViewOutboundEvent = ( break; case 'longPress': - handleLongPress(props, _, event.target, event.messageId, event.href); + handleLongPress(props, event.target, event.messageId, event.href); break; case 'url': @@ -302,6 +299,7 @@ export const handleWebViewOutboundEvent = ( } case 'time': { + const { _ } = props; const alertText = _('This time is in your timezone. Original text was “{originalText}”.', { originalText: event.originalText, }); From 1148de845d778b9fdf8aecb24feee3e39b9a815f Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 17 Oct 2022 14:48:34 -0700 Subject: [PATCH 05/12] msglist [nfc]: Inline the onError callback Just to simplify a bit the top level of this class declaration. Also delete the redundant type annotation, and narrow the lint-ignore to the particular rule it's meant for. --- src/webview/MessageList.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index b3e48733df2..8d9505185b6 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -132,10 +132,6 @@ class MessageListInner extends React.Component { sendInboundEventsIsReady: boolean; unsentInboundEvents: WebViewInboundEvent[] = []; - handleError = (event: mixed) => { - console.error(event); // eslint-disable-line - }; - sendInboundEvents = (uevents: $ReadOnlyArray): void => { if (this.webviewRef.current !== null && uevents.length > 0) { /* $FlowFixMe[incompatible-type]: This `postMessage` is undocumented; @@ -198,7 +194,9 @@ class MessageListInner extends React.Component { style: { backgroundColor: 'transparent' }, ref: this.webviewRef, onMessage: this.handleMessage, - onError: this.handleError, + onError: event => { + console.error(event); // eslint-disable-line no-console + }, }); } } From e5283d34b8a2088dca7e0d16f9d1bbeecbfe1bc4 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 17 Oct 2022 15:00:54 -0700 Subject: [PATCH 06/12] msglist [nfc]: Make renderSinglePageWebView more render-like By having it take a single props object, rather than some props-like values as positional arguments. --- src/webview/MessageList.js | 4 +++- src/webview/SinglePageWebView.js | 24 ++++++++++++++---------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 8d9505185b6..1f1f81378e7 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -189,7 +189,9 @@ class MessageListInner extends React.Component { backgroundData.serverEmojiData, ); - return renderSinglePageWebView(html, baseUrl, { + return renderSinglePageWebView({ + html, + baseUrl, decelerationRate: 'normal', style: { backgroundColor: 'transparent' }, ref: this.webviewRef, diff --git a/src/webview/SinglePageWebView.js b/src/webview/SinglePageWebView.js index fd522715e8f..5ce16580ba9 100644 --- a/src/webview/SinglePageWebView.js +++ b/src/webview/SinglePageWebView.js @@ -71,14 +71,16 @@ function makeOnShouldStartLoadWithRequest( // TODO: This should ideally be a proper React component of its own. The // thing that may require care when doing that is our use of // `shouldComponentUpdate` in its caller, `MessageList`. -export const renderSinglePageWebView = ( +export const renderSinglePageWebView = (props: { html: string, baseUrl: URL, - moreProps: $Rest< + ...$Rest< ElementConfigFull, {| source: mixed, originWhitelist: mixed, onShouldStartLoadWithRequest: mixed |}, >, -): React.Node => ( +}): React.Node => { + const { html, baseUrl, ...moreProps } = props; + // The `originWhitelist` and `onShouldStartLoadWithRequest` props are // meant to mitigate possible XSS bugs, by interrupting an attempted // exploit if it tries to navigate to a new URL by e.g. setting @@ -90,10 +92,12 @@ export const renderSinglePageWebView = ( // // Worse, the `originWhitelist` parameter is completely broken. See: // https://github.com/react-native-community/react-native-webview/pull/697 - -); + return ( + + ); +}; From 129d83a19163475577112fa6661f687b06ca8b45 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 18 Oct 2022 12:42:34 -0700 Subject: [PATCH 07/12] msglist [nfc]: Make SinglePageWebView a real React component From some experimentation, it turns out that re-rendering a WebView element is perfectly fine, just as one would hope if WebView is to be a well-behaved React component type. What isn't fine is re-rendering it with a *different `html` value*. Doing that will cause the page to be reloaded, so that any UI state inside it gets reset -- most notably, the user gets scrolled right back to their starting point (i.e. the first unread, or else the very end). Which is pretty reasonable on the WebView's part. So that means that the way our current logic succeeds at avoiding that problem isn't that it avoids re-rendering the WebView (though it does do that); it's that it avoids re-rendering the MessageList, and thereby avoids ever computing a new `html` in the first place. As a result, it's perfectly fine to have another component interposed here. Even if this new component gets re-rendered for some arbitrary reason, it'll use the `html` value it got from its parent, namely MessageList -- which we continue to take care not to re-render, so the `html` value still never changes. --- src/webview/MessageList.js | 26 ++++++------ src/webview/SinglePageWebView.js | 71 +++++++++++++++++--------------- 2 files changed, 52 insertions(+), 45 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 1f1f81378e7..f59ad3980d2 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -38,7 +38,7 @@ import { base64Utf8Encode } from '../utils/encoding'; import { caseNarrow, isConversationNarrow } from '../utils/narrow'; import { type BackgroundData, getBackgroundData } from './backgroundData'; import { ensureUnreachable } from '../generics'; -import { renderSinglePageWebView } from './SinglePageWebView'; +import SinglePageWebView from './SinglePageWebView'; type OuterProps = $ReadOnly<{| narrow: Narrow, @@ -189,17 +189,19 @@ class MessageListInner extends React.Component { backgroundData.serverEmojiData, ); - return renderSinglePageWebView({ - html, - baseUrl, - decelerationRate: 'normal', - style: { backgroundColor: 'transparent' }, - ref: this.webviewRef, - onMessage: this.handleMessage, - onError: event => { - console.error(event); // eslint-disable-line no-console - }, - }); + return ( + { + console.error(event); // eslint-disable-line no-console + }} + /> + ); } } diff --git a/src/webview/SinglePageWebView.js b/src/webview/SinglePageWebView.js index 5ce16580ba9..f93bc2ba9f2 100644 --- a/src/webview/SinglePageWebView.js +++ b/src/webview/SinglePageWebView.js @@ -56,8 +56,17 @@ function makeOnShouldStartLoadWithRequest( } } +type Props = $ReadOnly<{| + html: string, + baseUrl: URL, + ...$Rest< + ElementConfigFull, + {| source: mixed, originWhitelist: mixed, onShouldStartLoadWithRequest: mixed |}, + >, +|}>; + /** - * Render a WebView that shows the given HTML at the given base URL, only. + * A WebView that shows the given HTML at the given base URL, only. * * The WebView will show the page described by the HTML string `html`. Any * attempts to navigate to a new page will be rejected. @@ -68,36 +77,32 @@ function makeOnShouldStartLoadWithRequest( * Assumes `baseUrl` has the scheme `file:`. No actual file need exist at * `baseUrl` itself, because the page is taken from the string `html`. */ -// TODO: This should ideally be a proper React component of its own. The -// thing that may require care when doing that is our use of -// `shouldComponentUpdate` in its caller, `MessageList`. -export const renderSinglePageWebView = (props: { - html: string, - baseUrl: URL, - ...$Rest< - ElementConfigFull, - {| source: mixed, originWhitelist: mixed, onShouldStartLoadWithRequest: mixed |}, - >, -}): React.Node => { - const { html, baseUrl, ...moreProps } = props; +export default (React.memo( + React.forwardRef>( + /* eslint-disable-next-line prefer-arrow-callback */ + function SinglePageWebView(props, ref) { + const { html, baseUrl, ...moreProps } = props; - // The `originWhitelist` and `onShouldStartLoadWithRequest` props are - // meant to mitigate possible XSS bugs, by interrupting an attempted - // exploit if it tries to navigate to a new URL by e.g. setting - // `window.location`. - // - // Note that neither of them is a hard security barrier; they're checked - // only against the URL of the document itself. They cannot be used to - // validate the URL of other resources the WebView loads. - // - // Worse, the `originWhitelist` parameter is completely broken. See: - // https://github.com/react-native-community/react-native-webview/pull/697 - return ( - - ); -}; + // The `originWhitelist` and `onShouldStartLoadWithRequest` props are + // meant to mitigate possible XSS bugs, by interrupting an attempted + // exploit if it tries to navigate to a new URL by e.g. setting + // `window.location`. + // + // Note that neither of them is a hard security barrier; they're checked + // only against the URL of the document itself. They cannot be used to + // validate the URL of other resources the WebView loads. + // + // Worse, the `originWhitelist` parameter is completely broken. See: + // https://github.com/react-native-community/react-native-webview/pull/697 + return ( + + ); + }, + ), +): React.AbstractComponent>); From 086bd7c65593045aeb1d8d6f7f6315a33ab86033 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 17 Oct 2022 14:41:09 -0700 Subject: [PATCH 08/12] msglist [nfc]: Add more comments about inbound-events vs re-render --- src/webview/MessageList.js | 45 +++++++++++++++++++++++++++++++++++++- src/webview/js/js.js | 1 + 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index f59ad3980d2..8dda76f49b7 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -125,6 +125,35 @@ const webviewAssetsUrl = new URL('webview/', assetsUrl); const baseUrl = new URL('index.html', webviewAssetsUrl); class MessageListInner extends React.Component { + // NOTE: This component has an unusual lifecycle for a React component! + // + // In the React element which the render method returns, the bulk of + // the interesting content is in one string, an HTML document, which will + // get passed to a WebView. + // + // That WebView is a leaf of the tree that React sees. But there's a lot + // of structure inside that HTML string, and there's UI state (in + // particular, the scroll position) in the resulting page in the browser. + // So when the content that would go in that HTML changes, we don't want + // to just replace the entire HTML document. We want to use the structure + // to make localized updates to the page in the browser, much as React + // does automatically for changes in its tree. + // + // This is important not only for performance (computing all the HTML for + // a long list of messages is expensive), but for correct behavior: if we + // did change the HTML string passed to the WebView, the user would find + // themself suddenly scrolled back to the bottom. + // + // So: + // * We let this component render just once, and define + // `shouldComponentUpdate` so as to mostly prevent re-renders. + // (We still re-render if the theme changes, though, and potentially at + // arbitrary other times; see docs/architecture/react.md .) + // * When the props change, we compute a set of events describing the + // changes, and send them to our code inside the webview to execute. + // + // See also docs/architecture/react.md . + static contextType = ThemeContext; context: ThemeData; @@ -132,6 +161,12 @@ class MessageListInner extends React.Component { sendInboundEventsIsReady: boolean; unsentInboundEvents: WebViewInboundEvent[] = []; + /** + * Send the given inbound-events to the inside-webview code. + * + * See `handleMessageEvent` in the inside-webview code for where these are + * received and processed. + */ sendInboundEvents = (uevents: $ReadOnlyArray): void => { if (this.webviewRef.current !== null && uevents.length > 0) { /* $FlowFixMe[incompatible-type]: This `postMessage` is undocumented; @@ -153,18 +188,26 @@ class MessageListInner extends React.Component { }; shouldComponentUpdate(nextProps) { + // Account for the new props by sending any needed inbound-events to the + // inside-webview code. const uevents = generateInboundEvents(this.props, nextProps); - if (this.sendInboundEventsIsReady) { this.sendInboundEvents(uevents); } else { this.unsentInboundEvents.push(...uevents); } + // Then, skip any React re-render. See discussion at top of component. return false; } render() { + // NB: This only runs once for a given component instance! + // See `shouldComponentUpdate`, and discussion at top of component. + // + // This means that all changes to props must be handled by + // inbound-events, or they simply won't be handled at all. + const { backgroundData, messageListElementsForShownMessages, diff --git a/src/webview/js/js.js b/src/webview/js/js.js index 8e2459d8b31..8367b48f0b3 100644 --- a/src/webview/js/js.js +++ b/src/webview/js/js.js @@ -739,6 +739,7 @@ const inboundEventHandlers = { }; // See `handleInitialLoad` for how this gets subscribed to events. +// See `sendInboundEvents` on `MessageList` for where they're sent from. const handleMessageEvent: MessageEventListener = e => { scrollEventsDisabled = true; // This decoding inverts `base64Utf8Encode`. From 190676ce249ef21258b7720844e55f1eda46bb9d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 18 Oct 2022 22:13:17 -0700 Subject: [PATCH 09/12] msglist [nfc]: Properly initialize a boolean The code already behaved correctly, because when we look at this property we only ever test it for truthiness, so void is just as good as false. But it wasn't really well-typed; apparently Flow looked the other way. --- src/webview/MessageList.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 8dda76f49b7..7bedccac6de 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -158,7 +158,7 @@ class MessageListInner extends React.Component { context: ThemeData; webviewRef = React.createRef>(); - sendInboundEventsIsReady: boolean; + sendInboundEventsIsReady: boolean = false; unsentInboundEvents: WebViewInboundEvent[] = []; /** From bc3532196380e3a6c0c8e853685b8c6b032eae76 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 17 Oct 2022 15:19:01 -0700 Subject: [PATCH 10/12] msglist: Convert to a Hooks-based function component Most of this is a routine class-to-Hooks conversion. The interesting part is the lifecycle method `shouldComponentUpdate`; the part where it tracks changes to props turns into an effect, and the part where it blocks (or tries to) re-renders turns into forcibly memoizing the bulk of the old render method using a ref. As a bonus, this fixes a longstanding latent bug: React could have at any time chosen to go ahead and re-render the component even though we have a `shouldComponentUpdate` that always returns false. (The React docs stress that there's no guarantee on that score, that the method exists only for performance optimization.) That would cause us to recompute `html`, potentially taking some time. And if the resulting value had changed since the initial render, it'd cause the state in the WebView, including scroll position, to reset. With React Hooks, we can use refs to ensure that we never compute `html` a second time, and never change its value, just as we intend. (With a bit of future-proofing to ensure we continue displaying something coherent if the theme changes, which #5533 will enable.) --- docs/architecture/react.md | 50 ++++-------- src/webview/MessageList.js | 143 +++++++++++++++++++--------------- src/webview/backgroundData.js | 7 +- 3 files changed, 99 insertions(+), 101 deletions(-) diff --git a/docs/architecture/react.md b/docs/architecture/react.md index c64fcfd7fb2..c74b4caa6cd 100644 --- a/docs/architecture/react.md +++ b/docs/architecture/react.md @@ -141,40 +141,16 @@ doc](https://reactjs.org/docs/context.html)): > [...]) > is not subject to the shouldComponentUpdate method -Concretely, this means that our `MessageList` component updates -(re-`render`s) when the theme changes, since it's a `ThemeContext` -consumer, *even though its `shouldComponentUpdate` always returns -`false`*. This generally isn't a problem because the UI for -changing our own theme setting can't appear while a `MessageList` is -in the navigation stack; so the theme can change only once we have -#4009, via the OS-level theme changing (either automatically on -schedule, or because the user changed it in system settings.) When -this does happen we see that the message list's color scheme changes -as we'd want it to, but we also see the bad effects that -`shouldComponentUpdate` returning `false` is meant to prevent: -losing the scroll position, mainly (but also, we expect, discarding -the image cache, etc.). - -### The exception: `MessageList` - -We have one React component that we wrote (beyond `connect` calls) that -deviates from the above design: `MessageList`. This is the only -component that extends plain `Component` rather than `PureComponent`, -and it's the only component in which we implement -`shouldComponentUpdate`. - -In fact, `MessageList` does adhere to the Pure Component Principle -- its -`render` method is a pure function of `this.props` and `this.context`. So -it could use `PureComponent`, but it doesn't -- instead we have a -`shouldComponentUpdate` that always returns `false`, so even when `props` -change quite materially (e.g., a new Zulip message arrives which should be -displayed) we don't have React re-render the component. (See the note -on the current Context API, above, for a known case where -`shouldComponentUpdate` can be ignored.) - -The specifics of why not, and what we do instead, deserve an architecture -doc of their own. In brief: `render` returns a single React element, a -`WebView`; on new Zulip messages or other updates to the props, we choose -not to have React make a new `WebView` and render it in the usual way; -instead, we use `WebView#postMessage` to send information to the JS code -running inside the `WebView`, and that code updates the DOM accordingly. +We also confirmed this behavior experimentally, in a 2020 version of +`MessageList` which used `ThemeContext` to get the theme colors. +(More recently it's not a class component at all, but a Hooks-based +function component.) That component re-`render`ed when the theme changed, +*even though its `shouldComponentUpdate` always returned `false`*. +This didn't cause a live problem because the UI doesn't +allow changing the theme while a `MessageList` is in the navigation +stack. If it were possible, it would be a concern: setting a short +interval to automatically toggle the theme, we see that the message +list's color scheme changes as we'd want it to, but we also see the +bad effects that `shouldComponentUpdate` returning `false` is meant to +prevent: losing the scroll position, mainly (but also, we expect, +discarding the image cache, etc.). diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 7bedccac6de..1f0ca1f2fe3 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -16,7 +16,6 @@ import type { EditMessage, } from '../types'; import { assumeSecretlyGlobalState } from '../reduxTypes'; -import type { ThemeData } from '../styles'; import { ThemeContext } from '../styles'; import { connect } from '../react-redux'; import { @@ -39,6 +38,7 @@ import { caseNarrow, isConversationNarrow } from '../utils/narrow'; import { type BackgroundData, getBackgroundData } from './backgroundData'; import { ensureUnreachable } from '../generics'; import SinglePageWebView from './SinglePageWebView'; +import { usePrevious } from '../reactUtils'; type OuterProps = $ReadOnly<{| narrow: Narrow, @@ -124,10 +124,10 @@ const webviewAssetsUrl = new URL('webview/', assetsUrl); */ const baseUrl = new URL('index.html', webviewAssetsUrl); -class MessageListInner extends React.Component { +function MessageListInner(props: Props) { // NOTE: This component has an unusual lifecycle for a React component! // - // In the React element which the render method returns, the bulk of + // In the React element which this render function returns, the bulk of // the interesting content is in one string, an HTML document, which will // get passed to a WebView. // @@ -145,21 +145,18 @@ class MessageListInner extends React.Component { // themself suddenly scrolled back to the bottom. // // So: - // * We let this component render just once, and define - // `shouldComponentUpdate` so as to mostly prevent re-renders. - // (We still re-render if the theme changes, though, and potentially at - // arbitrary other times; see docs/architecture/react.md .) + // * We compute the HTML document just once and then always re-use that + // initial value. // * When the props change, we compute a set of events describing the // changes, and send them to our code inside the webview to execute. // // See also docs/architecture/react.md . - static contextType = ThemeContext; - context: ThemeData; + const theme = React.useContext(ThemeContext); - webviewRef = React.createRef>(); - sendInboundEventsIsReady: boolean = false; - unsentInboundEvents: WebViewInboundEvent[] = []; + const webviewRef = React.useRef | null>(null); + const sendInboundEventsIsReady = React.useRef(false); + const unsentInboundEvents = React.useRef([]); /** * Send the given inbound-events to the inside-webview code. @@ -167,47 +164,70 @@ class MessageListInner extends React.Component { * See `handleMessageEvent` in the inside-webview code for where these are * received and processed. */ - sendInboundEvents = (uevents: $ReadOnlyArray): void => { - if (this.webviewRef.current !== null && uevents.length > 0) { - /* $FlowFixMe[incompatible-type]: This `postMessage` is undocumented; + const sendInboundEvents = React.useCallback( + (uevents: $ReadOnlyArray): void => { + if (webviewRef.current !== null && uevents.length > 0) { + /* $FlowFixMe[incompatible-type]: This `postMessage` is undocumented; tracking as #3572. */ - const secretWebView: { postMessage: (string, string) => void, ... } = this.webviewRef.current; - secretWebView.postMessage(base64Utf8Encode(JSON.stringify(uevents)), '*'); - } - }; + const secretWebView: { postMessage: (string, string) => void, ... } = webviewRef.current; + secretWebView.postMessage(base64Utf8Encode(JSON.stringify(uevents)), '*'); + } + }, + [], + ); - handleMessage = (event: { +nativeEvent: { +data: string, ... }, ... }) => { - const eventData: WebViewOutboundEvent = JSON.parse(event.nativeEvent.data); - if (eventData.type === 'ready') { - this.sendInboundEventsIsReady = true; - this.sendInboundEvents([{ type: 'ready' }, ...this.unsentInboundEvents]); - this.unsentInboundEvents = []; - } else { - handleWebViewOutboundEvent(this.props, eventData); + const handleMessage = React.useCallback( + (event: { +nativeEvent: { +data: string, ... }, ... }) => { + const eventData: WebViewOutboundEvent = JSON.parse(event.nativeEvent.data); + if (eventData.type === 'ready') { + sendInboundEventsIsReady.current = true; + sendInboundEvents([{ type: 'ready' }, ...unsentInboundEvents.current]); + unsentInboundEvents.current = []; + } else { + handleWebViewOutboundEvent(props, eventData); + } + }, + [props, sendInboundEvents], + ); + + const lastProps = usePrevious(props, props); + React.useEffect(() => { + if (props === lastProps) { + // Nothing to update. (This happens in particular on first render.) + return; } - }; - shouldComponentUpdate(nextProps) { // Account for the new props by sending any needed inbound-events to the // inside-webview code. - const uevents = generateInboundEvents(this.props, nextProps); - if (this.sendInboundEventsIsReady) { - this.sendInboundEvents(uevents); + const uevents = generateInboundEvents(lastProps, props); + if (sendInboundEventsIsReady.current) { + sendInboundEvents(uevents); } else { - this.unsentInboundEvents.push(...uevents); + unsentInboundEvents.current.push(...uevents); } + }, [lastProps, props, sendInboundEvents]); - // Then, skip any React re-render. See discussion at top of component. - return false; - } - - render() { - // NB: This only runs once for a given component instance! - // See `shouldComponentUpdate`, and discussion at top of component. - // - // This means that all changes to props must be handled by - // inbound-events, or they simply won't be handled at all. - + // We compute the page contents as an HTML string just once (*), on this + // MessageList's first render. See discussion at top of function. + // + // Note this means that all changes to props must be handled by + // inbound-events, or they simply won't be handled at all. + // + // (*) The logic below doesn't quite look that way -- what it says is that + // we compute the HTML on first render, and again any time the theme + // changes. Until we implement #5533, this comes to the same thing, + // because the only way to change the theme is for the user to + // navigate out to our settings UI. We write it this way so that it + // won't break with #5533. + // + // On the other hand, this means that if the theme changes we'll get + // the glitch described at top of function, scrolling the user to the + // bottom. Better than mismatched themes, but not ideal. A nice bonus + // followup on #5533 would be to add an inbound-event for changing the + // theme, and then truly compute the HTML just once. + const htmlRef = React.useRef(null); + const prevTheme = usePrevious(theme); + if (htmlRef.current == null || theme !== prevTheme) { const { backgroundData, messageListElementsForShownMessages, @@ -215,14 +235,14 @@ class MessageListInner extends React.Component { showMessagePlaceholders, doNotMarkMessagesAsRead, _, - } = this.props; + } = props; const contentHtml = messageListElementsForShownMessages .map(element => messageListElementHtml({ backgroundData, element, _ })) .join(''); const { auth } = backgroundData; - const html: string = getHtml( + htmlRef.current = getHtml( contentHtml, - this.context.themeName, + theme.themeName, { scrollMessageId: initialScrollMessageId, auth, @@ -231,21 +251,21 @@ class MessageListInner extends React.Component { }, backgroundData.serverEmojiData, ); - - return ( - { - console.error(event); // eslint-disable-line no-console - }} - /> - ); } + + return ( + ({ backgroundColor: 'transparent' }), [])} + ref={webviewRef} + onMessage={handleMessage} + onError={React.useCallback(event => { + console.error(event); // eslint-disable-line no-console + }, [])} + /> + ); } /** @@ -279,6 +299,7 @@ const marksMessagesAsRead = (narrow: Narrow): boolean => mentioned: () => false, }); +// TODO next steps: merge these wrappers into function, one at a time. const MessageList: React.ComponentType = connect( (state, props: OuterProps) => { // If this were a function component with Hooks, these would be diff --git a/src/webview/backgroundData.js b/src/webview/backgroundData.js index 02075c03f04..4722e56dd46 100644 --- a/src/webview/backgroundData.js +++ b/src/webview/backgroundData.js @@ -73,9 +73,10 @@ export type BackgroundData = $ReadOnly<{| // 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 as used in -// MessageList, 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. +// MessageList, because we memoize the expensive parts of rendering and in +// generateInboundEvents we check for changes to individual relevant +// properties of backgroundData, rather than backgroundData itself. But +// it'd be better to set an example of the right general pattern. export const getBackgroundData = ( state: PerAccountState, globalSettings: GlobalSettingsState, From 5785363bcc95f02eff0105dad7ccde70c9603f0d Mon Sep 17 00:00:00 2001 From: Greg Price Date: Mon, 17 Oct 2022 17:14:16 -0700 Subject: [PATCH 11/12] msglist [nfc]: Maintain props as a ref This just represents inlining the definition of `usePrevious`, and then simplifying. We'll make use of this ref in the next commit. --- src/webview/MessageList.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 1f0ca1f2fe3..965078e78fc 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -190,12 +190,14 @@ function MessageListInner(props: Props) { [props, sendInboundEvents], ); - const lastProps = usePrevious(props, props); + const propsRef = React.useRef(props); React.useEffect(() => { + const lastProps = propsRef.current; if (props === lastProps) { // Nothing to update. (This happens in particular on first render.) return; } + propsRef.current = props; // Account for the new props by sending any needed inbound-events to the // inside-webview code. @@ -205,7 +207,7 @@ function MessageListInner(props: Props) { } else { unsentInboundEvents.current.push(...uevents); } - }, [lastProps, props, sendInboundEvents]); + }, [props, sendInboundEvents]); // We compute the page contents as an HTML string just once (*), on this // MessageList's first render. See discussion at top of function. From a44dd531728dd69784fa61d91b868db3023f6e9c Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 18 Oct 2022 21:37:07 -0700 Subject: [PATCH 12/12] msglist: Tweak how handleMessage callback gets the props --- src/webview/MessageList.js | 41 +++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 965078e78fc..78a1aab2c5d 100644 --- a/src/webview/MessageList.js +++ b/src/webview/MessageList.js @@ -176,20 +176,6 @@ function MessageListInner(props: Props) { [], ); - const handleMessage = React.useCallback( - (event: { +nativeEvent: { +data: string, ... }, ... }) => { - const eventData: WebViewOutboundEvent = JSON.parse(event.nativeEvent.data); - if (eventData.type === 'ready') { - sendInboundEventsIsReady.current = true; - sendInboundEvents([{ type: 'ready' }, ...unsentInboundEvents.current]); - unsentInboundEvents.current = []; - } else { - handleWebViewOutboundEvent(props, eventData); - } - }, - [props, sendInboundEvents], - ); - const propsRef = React.useRef(props); React.useEffect(() => { const lastProps = propsRef.current; @@ -209,6 +195,33 @@ function MessageListInner(props: Props) { } }, [props, sendInboundEvents]); + const handleMessage = React.useCallback( + (event: { +nativeEvent: { +data: string, ... }, ... }) => { + const eventData: WebViewOutboundEvent = JSON.parse(event.nativeEvent.data); + if (eventData.type === 'ready') { + sendInboundEventsIsReady.current = true; + sendInboundEvents([{ type: 'ready' }, ...unsentInboundEvents.current]); + unsentInboundEvents.current = []; + } else { + // Instead of closing over `props` itself, we indirect through + // `propsRef`, which gets updated by the effect above. + // + // That's because unlike in a typical component, the UI this acts as + // a UI callback for isn't based on the current props, but on the + // data we've communicated through inbound-events. (See discussion + // at top of component.) So that's the data we want to refer to + // when interpreting the user's interaction; and `propsRef` is what + // the effect above updates in sync with sending those events. + // + // (The distinction may not matter much here in practice. But a + // nice bonus of this way is that we avoid re-renders of + // SinglePageWebView, potentially a helpful optimization.) + handleWebViewOutboundEvent(propsRef.current, eventData); + } + }, + [sendInboundEvents], + ); + // We compute the page contents as an HTML string just once (*), on this // MessageList's first render. See discussion at top of function. //