translation context: Eliminate key hack (the last one!).

In putting together a recent commit, I realized that this would be
possible if our `TranslationContextTranslator` more carefully spoke
the language of the old Context API. By making it a `Component`
instead of a `PureComponent`, and making sure there aren't any
disruptive ancestors between it and IntlProvider (in particular,
there are *no* ancestors at all) it'll be updated when `intl`
changes.

It already speaks the new Context API as well as it needs to (i.e.,
nothing special has to happen to get its consumers to update), and
we don't have any consumers that directly consume the old one
straight from `react-intl`.

One necessary tweak was to stop using `this._`, which is only set in
the constructor. (Reminiscent of b530f6c, when we were removing
the `key` hack for styles.)

Fixes: zulip#1946

Author: chrisbobbe

docs/architecture/react.md

@@ -102,76 +102,92 @@ long as the code adheres to core Redux principles:
 
 ### Context
 
-As long as you use the [current Context
+We're on board with the current API where possible; there's a
+third-party library we use that isn't there yet.
+
+#### Current Context API
+
+We should use the [current Context
 API](https://reactjs.org/docs/context.html) instead of the [legacy
-one](https://reactjs.org/docs/legacy-context.html), there's not much
-to worry about here. From the doc for the current one:
+one](https://reactjs.org/docs/legacy-context.html) wherever possible.
+The new API aggressively ensures consumers will be updated
+(re-`render`ed) on context changes, and the old one doesn't (see
+below). From the [new API's
+doc](https://reactjs.org/docs/context.html):
 
 > All consumers that are descendants of a Provider will re-render
 > whenever the Provider’s `value` prop changes.
 
-Then, anticipating the question "What about `shouldComponentUpdate`?"
-(and therefore, "What about `PureComponent`s?", because
-`PureComponents`
-[implement](https://reactjs.org/docs/react-api.html#reactpurecomponent)
-`shouldComponentUpdate`):
+It's so aggressive that there's a potential "gotcha" with the new API:
+context consumers are the first occurrence of the following that we're
+aware of (from the [doc on
+`shouldComponentUpdate`](https://reactjs.org/docs/react-component.html#shouldcomponentupdate)):
+
+> In the future React may treat `shouldComponentUpdate()` as a hint
+> rather than a strict directive, and returning `false` may still
+> result in a re-rendering of the component.
+
+We gather this from the following (in the [new API's
+doc](https://reactjs.org/docs/context.html)):
 
 > The propagation from Provider to its descendant consumers (including
 > [`.contextType`](https://reactjs.org/docs/context.html#classcontexttype)
-> and
-> [`useContext`](https://reactjs.org/docs/hooks-reference.html#usecontext))
-> is not subject to the shouldComponentUpdate method, so the consumer is
-> updated even when an ancestor component skips an update.
-
-(I think word "even" means to emphasize "ancestor" -- i.e., it's not
-only the case that a consumer's *own* `shouldComponentUpdate` is
-ignored, but that, remarkably, so is the `shouldComponentUpdate` of
-ancestor components. I think it's just as remarkable that a
-component's own `shouldComponentUpdate` is ignored: this sounds like a
-clear instance of the
-[following](https://reactjs.org/docs/react-component.html#shouldcomponentupdate):
-"In the future React may treat `shouldComponentUpdate()` as a hint
-rather than a strict directive, and returning `false` may still result
-in a re-rendering of the component.")
+> [...])
+> 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`*. So far, this hasn't been a 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
+effects that `shouldComponentUpdate` returning `false` is meant to
+prevent: losing the scroll position, mainly (but also, we expect,
+discarding the image cache, etc.).
+
+#### Legacy Context API
 
 The legacy Context API is
 [declared](https://reactjs.org/docs/legacy-context.html#updating-context)
-fundamentally broken because `shouldComponentUpdate` and
-`PureComponent`s can block updates.
-
-We're forced to reckon with the legacy Context API in one place: the
-`react-intl` library uses it for the `intl` context. We "translate"
-the old API to the new one ASAP, in `TranslationContextTranslator`, so
-we can get used to using Context in the new way. But we've made
-`TranslationContextTranslator` a `PureComponent`, exposing ourselves
-to the legacy API's flaw. So we have to use a "`key` hack", which is
-the following:
-
-If `locale` changes, we make the entire React component tree at and
-below `IntlProvider` remount. (Not merely re-`render`: completely
-vanish and start over with `componentDidMount`; see the note at [this
-doc](https://5d4b5feba32acd0008d0df98--reactjs.netlify.app/docs/reconciliation.html)
-starting with "Keys should be stable, predictable, and unique".) We do
-this with the [`key`
-attribute](https://reactjs.org/docs/lists-and-keys.html), which isn't
-recommended for use except in lists.
-
-Previously, we used the same `key` hack for our own styles data, to
-get changes in the theme to propagate. No longer! :)
+fundamentally broken because consumers could be blocked from receiving
+updates to the context, and not just by the consumer's own
+`shouldComponentUpdate`:
+
+> The problem is, if a context value provided by component changes,
+> descendants that use that value won’t update if an intermediate
+> parent returns `false` from `shouldComponentUpdate`. This is totally
+> out of control of the components using context, so there’s basically
+> no way to reliably update the context.
+
+We have to think about the legacy Context API in just one place. The
+`react-intl` library's `IntlProvider` uses it to provide the `intl`
+context. The only consumer of `intl` is
+`TranslationContextTranslator`, which "speaks" the old API by being
+the direct child of `IntlProvider` and by being a `Component`, not a
+`PureComponent` (whose under-the-hood `shouldComponentUpdate` would
+suppress updates on context changes)—all to make sure that it
+re-`render`s whenever `intl` changes. Then,
+`TranslationContextTranslator` is itself a provider, and it provides
+the same value, but it does so in the new Context API way. All its
+consumers are updated appropriately, which is what we want.
 
 ### 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
-the only component that implements `shouldComponentUpdate`.
+We have one React component that we wrote (beyond `connect` calls)
+that deviates from the above design: `MessageList`.  It 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.
+displayed) we don't have React re-render the component. (See the note
+on the current Context API, above, for a known case where our
+`shouldComponentUpdate` is 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

src/boot/TranslationProvider.js

@@ -1,5 +1,5 @@
 /* @flow strict-local */
-import React, { PureComponent } from 'react';
+import React, { PureComponent, Component } from 'react';
 import type { ComponentType, ElementConfig, Node as React$Node } from 'react';
 import { Text } from 'react-native';
 import { IntlProvider } from 'react-intl';
@@ -51,9 +51,37 @@ const makeGetText = (intl: IntlShape): GetText => {
  * new API.
  *
  * See https://reactjs.org/docs/context.html
- * vs. https://reactjs.org/docs/legacy-context.html .
+ * vs. https://reactjs.org/docs/legacy-context.html.
+ *
+ * Why do we need this? `IntlProvider` uses React's "legacy context
+ * API", deprecated since React 16.3, of which the docs say:
+ *
+ *   ## Updating Context
+ *
+ *   Don't do it.
+ *
+ *   React has an API to update context, but it is fundamentally
+ *   broken and you should not use it.
+ *
+ * It's broken because a consumer in the old way would never
+ * re-`render` on changes to the context if they, or any of their
+ * ancestors below the provider, implemented `shouldComponentUpdate`
+ * in a way that blocked updates from the context. This meant that
+ * neither the provider nor the consumer had the power to fix many
+ * non-re-`render`ing bugs. A very common context-update-blocking
+ * implementation of `shouldComponentUpdate` is the one
+ * `PureComponent` uses, so the effect is widespread.
+ *
+ * In the new way, `shouldComponentUpdate`s (as implemented by hand or
+ * by using `PureComponent`) in the hierarchy all the way down to the
+ * consumer (inclusive) are ignored when the context updates.
+ *
+ * Consumers should consume `TranslationContext` as it's provided
+ * here, so they don't have to worry about not updating when it
+ * changes.
  */
-class TranslationContextTranslator extends PureComponent<{|
+// This component MUST NOT be a `PureComponent`; see above.
+class TranslationContextTranslator extends Component<{|
   +children: React$Node,
 |}> {
   context: { intl: IntlShape };
@@ -62,11 +90,9 @@ class TranslationContextTranslator extends PureComponent<{|
     intl: () => null,
   };
 
-  _ = makeGetText(this.context.intl);
-
   render() {
     return (
-      <TranslationContext.Provider value={this._}>
+      <TranslationContext.Provider value={makeGetText(this.context.intl)}>
         {this.props.children}
       </TranslationContext.Provider>
     );
@@ -84,21 +110,7 @@ class TranslationProvider extends PureComponent<Props> {
     const { locale, children } = this.props;
 
     return (
-      /* `IntlProvider` uses React's "legacy context API", deprecated since
-       * React 16.3, of which the docs say:
-       *
-       *   ## Updating Context
-       *
-       *   Don't do it.
-       *
-       *   React has an API to update context, but it is fundamentally
-       *   broken and you should not use it.
-       *
-       * To work around that, we set `key={locale}` to force the whole tree
-       * to rerender if the locale changes.  Not cheap, but the locale
-       * changing is rare.
-       */
-      <IntlProvider key={locale} locale={locale} textComponent={Text} messages={messages[locale]}>
+      <IntlProvider locale={locale} textComponent={Text} messages={messages[locale]}>
         <TranslationContextTranslator>{children}</TranslationContextTranslator>
       </IntlProvider>
     );