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/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/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, 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; diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js index 70a9b8eedd5..78a1aab2c5d 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 { @@ -38,7 +37,8 @@ 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'; +import { usePrevious } from '../reactUtils'; type OuterProps = $ReadOnly<{| narrow: Narrow, @@ -124,52 +124,125 @@ const webviewAssetsUrl = new URL('webview/', assetsUrl); */ const baseUrl = new URL('index.html', webviewAssetsUrl); -class MessageListInner extends React.Component { - static contextType = ThemeContext; - context: ThemeData; +function MessageListInner(props: Props) { + // NOTE: This component has an unusual lifecycle for a React component! + // + // 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. + // + // 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 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 . - webviewRef = React.createRef>(); - sendInboundEventsIsReady: boolean; - unsentInboundEvents: WebViewInboundEvent[] = []; + const theme = React.useContext(ThemeContext); - handleError = (event: mixed) => { - console.error(event); // eslint-disable-line - }; + const webviewRef = React.useRef | null>(null); + const sendInboundEventsIsReady = React.useRef(false); + const unsentInboundEvents = React.useRef([]); - sendInboundEvents = (uevents: $ReadOnlyArray): void => { - if (this.webviewRef.current !== null && uevents.length > 0) { - /* $FlowFixMe[incompatible-type]: This `postMessage` is undocumented; + /** + * Send the given inbound-events to the inside-webview code. + * + * See `handleMessageEvent` in the inside-webview code for where these are + * received and processed. + */ + 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 { - const { _ } = this.props; - handleWebViewOutboundEvent(this.props, _, eventData); + 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; } - }; - - shouldComponentUpdate(nextProps) { - const uevents = generateInboundEvents(this.props, nextProps); + propsRef.current = props; - if (this.sendInboundEventsIsReady) { - this.sendInboundEvents(uevents); + // Account for the new props by sending any needed inbound-events to the + // inside-webview code. + const uevents = generateInboundEvents(lastProps, props); + if (sendInboundEventsIsReady.current) { + sendInboundEvents(uevents); } else { - this.unsentInboundEvents.push(...uevents); + unsentInboundEvents.current.push(...uevents); } + }, [props, sendInboundEvents]); - return false; - } + 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], + ); - render() { + // 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, @@ -177,14 +250,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, @@ -193,15 +266,21 @@ class MessageListInner extends React.Component { }, backgroundData.serverEmojiData, ); - - return renderSinglePageWebView(html, baseUrl, { - decelerationRate: 'normal', - style: { backgroundColor: 'transparent' }, - ref: this.webviewRef, - onMessage: this.handleMessage, - onError: this.handleError, - }); } + + return ( + ({ backgroundColor: 'transparent' }), [])} + ref={webviewRef} + onMessage={handleMessage} + onError={React.useCallback(event => { + console.error(event); // eslint-disable-line no-console + }, [])} + /> + ); } /** @@ -235,6 +314,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/SinglePageWebView.js b/src/webview/SinglePageWebView.js index fd522715e8f..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,32 +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 = ( - html: string, - baseUrl: URL, - moreProps: $Rest< - ElementConfigFull, - {| source: mixed, originWhitelist: mixed, onShouldStartLoadWithRequest: mixed |}, - >, -): React.Node => ( - // 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 - -); +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 ( + + ); + }, + ), +): React.AbstractComponent>); 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, diff --git a/src/webview/handleOutboundEvents.js b/src/webview/handleOutboundEvents.js index c74a1592b8d..cf0f53967e9 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 { 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; @@ -206,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, @@ -214,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; } @@ -222,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({ @@ -251,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 @@ -280,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': @@ -316,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, }); 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`.