Stop using legacy Context API for styles (continued)

docs/architecture/react.md

@@ -102,38 +102,93 @@ long as the code adheres to core Redux principles:
 
 ### Context
 
-Our "Pure Component Principle" says `render` is a pure function of props,
-state, *and context*.  But `PureComponent` only checks for changes to props
-and state, and skips re-render when just those two are unchanged.  Doesn't
-that open up bugs if just `this.context` changes?
-
-[Yes, it
-would](https://reactjs.org/docs/legacy-context.html#updating-context).  For
-this reason, when something provided in context is updated, we force the
-entire React component tree under that point (in our usage of `context`,
-this is nearly the entire tree) to re-render.  This means we use `context`
-sparingly -- only for things where its benefit for code readability is very
-large, and where updates are rare so we're OK with that global re-render.
-
-In `StylesProvider`, for example, this is done with a `key`.
-
-Relatedly, the `this.context` API we use is a legacy API.  Recent React
-versions offer a [new context API](https://reactjs.org/docs/context.html)
-that works much more like Redux and `connect`, above.
+We're on board with the current API where possible; there's a
+third-party library we use that isn't there yet, but we deal with that
+at the edge by "translating" the old API to the new one.
+
+#### 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) 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.
+
+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)
+> [...])
+> 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
+bad 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 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/ZulipMobile.js

@@ -5,7 +5,7 @@ import 'react-native-url-polyfill/auto';
 import '../vendor/intl/intl';
 import StoreProvider from './boot/StoreProvider';
 import TranslationProvider from './boot/TranslationProvider';
-import StylesProvider from './boot/StylesProvider';
+import ThemeProvider from './boot/ThemeProvider';
 import CompatibilityChecker from './boot/CompatibilityChecker';
 import AppEventHandlers from './boot/AppEventHandlers';
 import AppDataFetcher from './boot/AppDataFetcher';
@@ -26,11 +26,11 @@ export default (): React$Node => (
       <AppEventHandlers>
         <AppDataFetcher>
           <TranslationProvider>
-            <StylesProvider>
+            <ThemeProvider>
               <BackNavigationHandler>
                 <AppWithNavigation />
               </BackNavigationHandler>
-            </StylesProvider>
+            </ThemeProvider>
           </TranslationProvider>
         </AppDataFetcher>
       </AppEventHandlers>

src/boot/StylesProvider.js → src/boot/ThemeProvider.js

@@ -6,42 +6,25 @@ import type { Node as React$Node } from 'react';
 import type { ThemeName, Dispatch } from '../types';
 import { connect } from '../react-redux';
 import { getSettings } from '../directSelectors';
-import { stylesFromTheme, themeColors, ThemeContext } from '../styles/theme';
-
-const Dummy = props => props.children;
+import { themeData, ThemeContext } from '../styles/theme';
 
 type Props = $ReadOnly<{|
   dispatch: Dispatch,
   theme: ThemeName,
   children: React$Node,
 |}>;
 
-class StylesProvider extends PureComponent<Props> {
-  static childContextTypes = {
-    styles: () => {},
-  };
-
+class ThemeProvider extends PureComponent<Props> {
   static defaultProps = {
     theme: 'default',
   };
 
-  getChildContext() {
-    const { theme } = this.props;
-    const styles = stylesFromTheme(theme);
-    return { styles };
-  }
-
   render() {
     const { children, theme } = this.props;
-
-    return (
-      <ThemeContext.Provider value={themeColors[theme]}>
-        <Dummy key={theme}>{children}</Dummy>
-      </ThemeContext.Provider>
-    );
+    return <ThemeContext.Provider value={themeData[theme]}>{children}</ThemeContext.Provider>;
   }
 }
 
 export default connect(state => ({
   theme: getSettings(state).theme,
-}))(StylesProvider);
+}))(ThemeProvider);

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

src/chat/ChatScreen.js

@@ -5,7 +5,7 @@ import type { NavigationScreenProp } from 'react-navigation';
 import { ActionSheetProvider } from '@expo/react-native-action-sheet';
 
 import { connect } from '../react-redux';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import styles, { ThemeContext } from '../styles';
 import type { Dispatch, Fetching, Narrow, EditMessage } from '../types';
 import { KeyboardAvoider, OfflineNotice, ZulipStatusBar } from '../common';
@@ -39,7 +39,7 @@ type State = {|
 
 class ChatScreen extends PureComponent<Props, State> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   state = {
     editMessage: null,

src/common/Input.js

@@ -4,7 +4,7 @@ import { TextInput, Platform } from 'react-native';
 import { FormattedMessage } from 'react-intl';
 
 import type { LocalizableText } from '../types';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext, HALF_COLOR, BORDER_COLOR } from '../styles';
 
 export type Props = $ReadOnly<{|
@@ -34,7 +34,7 @@ type State = {|
  */
 export default class Input extends PureComponent<Props, State> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   styles = {
     input: {

src/common/Label.js

@@ -3,7 +3,7 @@ import React, { PureComponent } from 'react';
 import { Text } from 'react-native';
 import TranslatedText from './TranslatedText';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 import type { LocalizableText } from '../types';
 
@@ -26,7 +26,7 @@ type Props = $ReadOnly<{|
  */
 export default class Label extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   styles = {
     label: {

src/common/LineSeparator.js

@@ -2,12 +2,12 @@
 import React, { PureComponent } from 'react';
 import { View } from 'react-native';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 
 export default class LineSeparator extends PureComponent<{||}> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   styles = {
     lineSeparator: {

src/common/LoadingBanner.js

@@ -7,7 +7,7 @@ import type { Dispatch } from '../types';
 import { connect } from '../react-redux';
 import { getLoading } from '../selectors';
 import { Label, LoadingIndicator } from '.';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 
 const key = 'LoadingBanner';
@@ -40,7 +40,7 @@ type Props = $ReadOnly<{|
  */
 class LoadingBanner extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   render() {
     if (!this.props.loading) {

src/common/OptionButton.js

@@ -6,7 +6,7 @@ import Label from './Label';
 import Touchable from './Touchable';
 import { IconRight } from './Icons';
 import type { SpecificIconType } from './Icons';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import styles, { ThemeContext } from '../styles';
 
 type Props = $ReadOnly<{|
@@ -17,7 +17,7 @@ type Props = $ReadOnly<{|
 
 export default class OptionButton extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   styles = {
     icon: styles.settingsIcon,

src/common/OptionDivider.js

@@ -2,12 +2,12 @@
 import React, { PureComponent } from 'react';
 import { View } from 'react-native';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 
 export default class OptionDivider extends PureComponent<{||}> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   styles = {
     divider: {