msglist: Convert MessageList to a function component with Hooks

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.).

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<Props, State> {
+export default class ErrorBoundary extends React.PureComponent<Props, State> {
   static getDerivedStateFromError(error: Error): State {
     return { error };
   }

src/boot/TranslationProvider.js

@@ -16,9 +16,10 @@ export const TranslationContext: Context<GetText> = 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<P: { +_: GetText, ... }, C: ComponentType<P>>(
   WrappedComponent: C,

src/sharing/ShareToPm.js

@@ -37,7 +37,7 @@ type State = $ReadOnly<{|
   choosingRecipients: boolean,
 |}>;
 
-export default class ShareToPm extends React.Component<Props, State> {
+export default class ShareToPm extends React.PureComponent<Props, State> {
   state: State = {
     selectedRecipients: [],
     choosingRecipients: false,

src/sharing/ShareToStream.js

@@ -47,7 +47,7 @@ type State = $ReadOnly<{|
   isTopicFocused: boolean,
 |}>;
 
-class ShareToStreamInner extends React.Component<Props, State> {
+class ShareToStreamInner extends React.PureComponent<Props, State> {
   state = {
     streamName: '',
     streamId: null,

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<Props, State> {
+class ShareWrapperInner extends React.PureComponent<Props, State> {
   static contextType = TranslationContext;
   context: GetText;
 

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,67 +124,140 @@ const webviewAssetsUrl = new URL('webview/', assetsUrl);
  */
 const baseUrl = new URL('index.html', webviewAssetsUrl);
 
-class MessageListInner extends React.Component<Props> {
-  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<React$ElementRef<typeof WebView>>();
-  sendInboundEventsIsReady: boolean;
-  unsentInboundEvents: WebViewInboundEvent[] = [];
+  const theme = React.useContext(ThemeContext);
 
-  handleError = (event: mixed) => {
-    console.error(event); // eslint-disable-line
-  };
+  const webviewRef = React.useRef<React$ElementRef<typeof WebView> | null>(null);
+  const sendInboundEventsIsReady = React.useRef<boolean>(false);
+  const unsentInboundEvents = React.useRef<WebViewInboundEvent[]>([]);
 
-  sendInboundEvents = (uevents: $ReadOnlyArray<WebViewInboundEvent>): 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<WebViewInboundEvent>): 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,
       initialScrollMessageId,
       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<Props> {
       },
       backgroundData.serverEmojiData,
     );
-
-    return renderSinglePageWebView(html, baseUrl, {
-      decelerationRate: 'normal',
-      style: { backgroundColor: 'transparent' },
-      ref: this.webviewRef,
-      onMessage: this.handleMessage,
-      onError: this.handleError,
-    });
   }
+
+  return (
+    <SinglePageWebView
+      html={htmlRef.current}
+      baseUrl={baseUrl}
+      decelerationRate="normal"
+      style={React.useMemo(() => ({ 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<OuterProps> = connect<SelectorProps, _, _>(
   (state, props: OuterProps) => {
     // If this were a function component with Hooks, these would be