From 8b30007a9df4e020553ba5b501bfe9909a8aaaed Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Fri, 24 Jul 2020 13:20:39 -0700
Subject: [PATCH 1/6] subscriptions reducer tests: Remove unwanted expectation
 from a test.

Currently, this test confirms that the reducer isn't clobbering the
details of a subscription.

It does this by making the `color` property

- present in the previous state
- missing in the subscription in the action
- present (at the same value) in the result state.

As Greg says in discussion [1], this isn't really an expectation
that we want to have:

"""
If we get an `op: 'add'` subscription event that includes a stream
we already had a subscription for, and especially if any of the data
differed, then (a) I think that means something has already gone
wrong and (b) if it does happen, we'd probably be best off doing the
opposite of what this test tested for, and clobbering our existing
clearly-stale data in favor of the new data.
"""

That being said,

"""
It's probably fine that we keep the code simple and effectively
assume this case doesn't happen, but there's no need for the test to
confirm that if it did happen we'd do the *wrong* thing.
"""

So, remove the unwanted expectation, and do it here, rather than in
the same commit where we start type-checking this file, since it's a
substantive change.

[1] https://github.com/zulip/zulip-mobile/pull/4199#discussion_r459121987
---
 src/subscriptions/__tests__/subscriptionsReducer-test.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/subscriptions/__tests__/subscriptionsReducer-test.js b/src/subscriptions/__tests__/subscriptionsReducer-test.js
index d1acbadde71..41653077bf0 100644
--- a/src/subscriptions/__tests__/subscriptionsReducer-test.js
+++ b/src/subscriptions/__tests__/subscriptionsReducer-test.js
@@ -108,6 +108,7 @@ describe('subscriptionsReducer', () => {
         subscriptions: [
           {
             name: 'some stream',
+            color: 'red',
             stream_id: 1,
           },
           {

From 5835d0604a9dda1702eefb5ab3936a0975ec49d2 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulipchat.com>
Date: Tue, 14 Apr 2020 17:45:34 -0700
Subject: [PATCH 2/6] subscriptions reducer test: Enable type checking, update
 accordingly.

This includes the removal of the test 'on unrecognized action,
returns input state unchanged' that is redundant with proper type
checking: it's impossible to represent the input intended for the
test in a way that passes our type checking, and the same type
checking will guard against the input in live code.
---
 .../__tests__/subscriptionsReducer-test.js    | 276 ++++--------------
 1 file changed, 61 insertions(+), 215 deletions(-)

diff --git a/src/subscriptions/__tests__/subscriptionsReducer-test.js b/src/subscriptions/__tests__/subscriptionsReducer-test.js
index 41653077bf0..e179a81e1bf 100644
--- a/src/subscriptions/__tests__/subscriptionsReducer-test.js
+++ b/src/subscriptions/__tests__/subscriptionsReducer-test.js
@@ -1,46 +1,48 @@
+/* @flow strict-local */
+
 import deepFreeze from 'deep-freeze';
 
 import { EventTypes } from '../../api/eventTypes';
-import { REALM_INIT, EVENT_SUBSCRIPTION, ACCOUNT_SWITCH, EVENT } from '../../actionConstants';
+import { EVENT_SUBSCRIPTION, ACCOUNT_SWITCH, EVENT } from '../../actionConstants';
 import subscriptionsReducer from '../subscriptionsReducer';
+import * as eg from '../../__tests__/lib/exampleData';
 
 describe('subscriptionsReducer', () => {
+  const stream1 = eg.makeStream({ name: 'stream1', description: 'my first stream' });
+  const sub1 = eg.makeSubscription({ stream: stream1 });
+
+  const stream2 = eg.makeStream({ name: 'stream2', description: 'my second stream' });
+  const sub2 = eg.makeSubscription({ stream: stream2 });
+
+  const stream3 = eg.makeStream({ name: 'stream3', description: 'my third stream' });
+  const sub3 = eg.makeSubscription({ stream: stream3 });
+
   describe('REALM_INIT', () => {
     test('when `subscriptions` data is provided init state with it', () => {
       const initialState = deepFreeze([]);
+
       const action = deepFreeze({
-        type: REALM_INIT,
+        ...eg.action.realm_init,
         data: {
-          subscriptions: [
-            {
-              name: 'some stream',
-              stream_id: 1,
-            },
-          ],
+          ...eg.action.realm_init.data,
+          subscriptions: [sub1],
         },
       });
 
       const actualState = subscriptionsReducer(initialState, action);
 
-      expect(actualState).toEqual([
-        {
-          name: 'some stream',
-          stream_id: 1,
-        },
-      ]);
+      expect(actualState).toEqual([sub1]);
     });
 
     test('when `subscriptions` is an empty array, reset state', () => {
-      const initialState = deepFreeze([
-        {
-          name: 'some stream',
-          stream_id: 1,
-        },
-      ]);
+      const initialState = deepFreeze([sub1]);
 
       const action = deepFreeze({
-        type: REALM_INIT,
-        data: { subscriptions: [] },
+        ...eg.action.realm_init,
+        data: {
+          ...eg.action.realm_init.data,
+          subscriptions: [],
+        },
       });
 
       const expectedState = [];
@@ -51,13 +53,6 @@ describe('subscriptionsReducer', () => {
     });
   });
 
-  test('on unrecognized action, returns input state unchanged', () => {
-    const prevState = deepFreeze({ hello: 'world' });
-
-    const newState = subscriptionsReducer(prevState, {});
-    expect(newState).toEqual(prevState);
-  });
-
   describe('EVENT_SUBSCRIPTION -> add', () => {
     test('if new subscriptions do not exist in state, add them', () => {
       const prevState = deepFreeze([]);
@@ -65,28 +60,11 @@ describe('subscriptionsReducer', () => {
       const action = deepFreeze({
         type: EVENT_SUBSCRIPTION,
         op: 'add',
-        subscriptions: [
-          {
-            name: 'some stream',
-            stream_id: 1,
-          },
-          {
-            name: 'some other stream',
-            stream_id: 2,
-          },
-        ],
+        id: 1,
+        subscriptions: [sub1, sub2],
       });
 
-      const expectedState = [
-        {
-          name: 'some stream',
-          stream_id: 1,
-        },
-        {
-          name: 'some other stream',
-          stream_id: 2,
-        },
-      ];
+      const expectedState = [sub1, sub2];
 
       const newState = subscriptionsReducer(prevState, action);
 
@@ -94,41 +72,16 @@ describe('subscriptionsReducer', () => {
     });
 
     test('if some of subscriptions already exist, do not add them', () => {
-      const prevState = deepFreeze([
-        {
-          color: 'red',
-          stream_id: 1,
-          name: 'some stream',
-        },
-      ]);
+      const prevState = deepFreeze([sub1]);
 
       const action = deepFreeze({
         type: EVENT_SUBSCRIPTION,
         op: 'add',
-        subscriptions: [
-          {
-            name: 'some stream',
-            color: 'red',
-            stream_id: 1,
-          },
-          {
-            name: 'some other stream',
-            stream_id: 2,
-          },
-        ],
+        id: 1,
+        subscriptions: [sub1, sub2],
       });
 
-      const expectedState = [
-        {
-          color: 'red',
-          name: 'some stream',
-          stream_id: 1,
-        },
-        {
-          name: 'some other stream',
-          stream_id: 2,
-        },
-      ];
+      const expectedState = [sub1, sub2];
 
       const newState = subscriptionsReducer(prevState, action);
 
@@ -138,46 +91,16 @@ describe('subscriptionsReducer', () => {
 
   describe('EVENT_SUBSCRIPTION -> remove', () => {
     test('removes subscriptions from state', () => {
-      const prevState = deepFreeze([
-        {
-          color: 'red',
-          stream_id: 1,
-          name: 'some stream',
-        },
-        {
-          color: 'green',
-          stream_id: 2,
-          name: 'other stream',
-        },
-        {
-          color: 'blue',
-          stream_id: 3,
-          name: 'third stream',
-        },
-      ]);
+      const prevState = deepFreeze([sub1, sub2, sub3]);
 
       const action = deepFreeze({
         type: EVENT_SUBSCRIPTION,
         op: 'remove',
-        subscriptions: [
-          {
-            name: 'some stream',
-            stream_id: 1,
-          },
-          {
-            name: 'some other stream',
-            stream_id: 2,
-          },
-        ],
+        id: 1,
+        subscriptions: [sub1, sub2],
       });
 
-      const expectedState = [
-        {
-          color: 'blue',
-          stream_id: 3,
-          name: 'third stream',
-        },
-      ];
+      const expectedState = [sub3];
 
       const newState = subscriptionsReducer(prevState, action);
 
@@ -185,26 +108,13 @@ describe('subscriptionsReducer', () => {
     });
 
     test('removes subscriptions that exist, do nothing if not', () => {
-      const prevState = deepFreeze([
-        {
-          name: 'some stream',
-          stream_id: 1,
-        },
-      ]);
+      const prevState = deepFreeze([sub1]);
 
       const action = deepFreeze({
         type: EVENT_SUBSCRIPTION,
         op: 'remove',
-        subscriptions: [
-          {
-            name: 'some stream',
-            stream_id: 1,
-          },
-          {
-            name: 'some other stream',
-            stream_id: 2,
-          },
-        ],
+        id: 1,
+        subscriptions: [sub1, sub2],
       });
 
       const expectedState = [];
@@ -217,54 +127,28 @@ describe('subscriptionsReducer', () => {
 
   describe('EVENT_SUBSCRIPTION -> update', () => {
     test('Change the in_home_view property', () => {
-      const initialState = deepFreeze([
-        {
-          stream_id: 123,
-          name: 'competition',
-          in_home_view: false,
-        },
-        {
-          stream_id: 67,
-          name: 'design',
-          in_home_view: false,
-        },
-        {
-          stream_id: 53,
-          name: 'mobile',
-          in_home_view: true,
-        },
-      ]);
+      const subNotInHomeView = deepFreeze({
+        ...eg.makeSubscription({
+          stream: stream1,
+        }),
+        in_home_view: false,
+      });
+
+      const initialState = deepFreeze([subNotInHomeView, sub2, sub3]);
 
       const action = deepFreeze({
-        stream_id: 123,
         type: EVENT_SUBSCRIPTION,
         op: 'update',
-        eventId: 2,
         id: 2,
-        name: 'competition',
+        email: subNotInHomeView.email_address,
+        stream_id: subNotInHomeView.stream_id,
+        name: subNotInHomeView.name,
         property: 'in_home_view',
         value: true,
       });
 
-      const expectedState = [
-        {
-          stream_id: 123,
-          name: 'competition',
-          in_home_view: true,
-        },
-        {
-          stream_id: 67,
-          name: 'design',
-          in_home_view: false,
-        },
-        {
-          stream_id: 53,
-          name: 'mobile',
-          in_home_view: true,
-        },
-      ];
-
       const actualState = subscriptionsReducer(initialState, action);
+      const expectedState = [{ ...subNotInHomeView, in_home_view: true }, sub2, sub3];
 
       expect(actualState).toEqual(expectedState);
     });
@@ -272,23 +156,14 @@ describe('subscriptionsReducer', () => {
 
   describe('EVENT_STREAM -> delete', () => {
     test('when a stream is delrted but user is not subscribed to it, do not change state', () => {
-      const initialState = deepFreeze([
-        {
-          stream_id: 3,
-          name: 'not subscribed to stream',
-        },
-      ]);
+      const initialState = deepFreeze([sub3]);
       const action = deepFreeze({
         type: EVENT,
         event: {
           type: EventTypes.stream,
+          id: 1,
           op: 'delete',
-          streams: [
-            {
-              name: 'some stream',
-              stream_id: 1,
-            },
-          ],
+          streams: [stream1],
         },
       });
 
@@ -298,27 +173,14 @@ describe('subscriptionsReducer', () => {
     });
 
     test('when a stream is deleted the user is unsubscribed', () => {
-      const initialState = deepFreeze([
-        {
-          stream_id: 1,
-          name: 'some stream',
-        },
-      ]);
+      const initialState = deepFreeze([sub1]);
       const action = deepFreeze({
         type: EVENT,
         event: {
           type: EventTypes.stream,
+          id: 1,
           op: 'delete',
-          streams: [
-            {
-              name: 'some stream',
-              stream_id: 1,
-            },
-            {
-              name: 'some other stream',
-              stream_id: 2,
-            },
-          ],
+          streams: [stream1, stream2],
         },
       });
       const expectedState = [];
@@ -329,31 +191,14 @@ describe('subscriptionsReducer', () => {
     });
 
     test('when multiple streams are deleted the user is unsubscribed from all of them', () => {
-      const initialState = deepFreeze([
-        {
-          stream_id: 1,
-          name: 'some stream',
-        },
-        {
-          name: 'some other stream',
-          stream_id: 2,
-        },
-      ]);
+      const initialState = deepFreeze([sub1, sub2]);
       const action = deepFreeze({
         type: EVENT,
         event: {
           type: EventTypes.stream,
+          id: 1,
           op: 'delete',
-          streams: [
-            {
-              name: 'some stream',
-              stream_id: 1,
-            },
-            {
-              name: 'some other stream',
-              stream_id: 2,
-            },
-          ],
+          streams: [stream1, stream2],
         },
       });
       const expectedState = [];
@@ -366,10 +211,11 @@ describe('subscriptionsReducer', () => {
 
   describe('ACCOUNT_SWITCH', () => {
     test('resets state to initial state', () => {
-      const initialState = deepFreeze(['some_stream']);
+      const initialState = deepFreeze([sub1]);
 
       const action = deepFreeze({
         type: ACCOUNT_SWITCH,
+        index: 2,
       });
 
       const expectedState = [];

From 53b4c0a46db29002f5ca935dd08b4d446f864143 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulipchat.com>
Date: Thu, 16 Apr 2020 13:15:53 -0700
Subject: [PATCH 3/6] styles: Dismantle pipeline for `styles` using legacy
 Context API.

As foreshadowed in this series, we get to remove the `key={theme}`
hack (along with some boilerplate to get things all working with the
old Context API).

Fixes: #3994
---
 docs/architecture/react.md                    | 104 ++++++++++++++----
 src/ZulipMobile.js                            |   6 +-
 .../{StylesProvider.js => ThemeProvider.js}   |  25 +----
 src/styles/theme.js                           |   7 --
 src/types.js                                  |   5 -
 5 files changed, 90 insertions(+), 57 deletions(-)
 rename src/boot/{StylesProvider.js => ThemeProvider.js} (51%)

diff --git a/docs/architecture/react.md b/docs/architecture/react.md
index 37d1e285900..ca16283edff 100644
--- a/docs/architecture/react.md
+++ b/docs/architecture/react.md
@@ -102,38 +102,100 @@ 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.
+
+#### 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`, but we've made that a `PureComponent`,
+so it doesn't get updated when `intl` changes. Our workaround until
+now has been a "`key` hack":
+
+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.
+
+In the next commit, we stop using the `key` hack and instead make
+`TranslationContextTranslator` "speak" the old API better.
 
 ### 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`.
+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.
+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
diff --git a/src/ZulipMobile.js b/src/ZulipMobile.js
index a9a3ff3a8fb..3e305a32c5b 100644
--- a/src/ZulipMobile.js
+++ b/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>
diff --git a/src/boot/StylesProvider.js b/src/boot/ThemeProvider.js
similarity index 51%
rename from src/boot/StylesProvider.js
rename to src/boot/ThemeProvider.js
index 32402545f1b..bf9cbd0beb9 100644
--- a/src/boot/StylesProvider.js
+++ b/src/boot/ThemeProvider.js
@@ -6,9 +6,7 @@ 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 { themeColors, ThemeContext } from '../styles/theme';
 
 type Props = $ReadOnly<{|
   dispatch: Dispatch,
@@ -16,32 +14,17 @@ type Props = $ReadOnly<{|
   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={themeColors[theme]}>{children}</ThemeContext.Provider>;
   }
 }
 
 export default connect(state => ({
   theme: getSettings(state).theme,
-}))(StylesProvider);
+}))(ThemeProvider);
diff --git a/src/styles/theme.js b/src/styles/theme.js
index 0085ee00984..61547572dfb 100644
--- a/src/styles/theme.js
+++ b/src/styles/theme.js
@@ -1,9 +1,6 @@
 /* @flow strict-local */
 import React from 'react';
 import type { Context } from 'react';
-import { StyleSheet } from 'react-native';
-
-import type { ThemeName } from '../types';
 
 export type ThemeColors = {|
   color: string,
@@ -12,8 +9,6 @@ export type ThemeColors = {|
   dividerColor: string,
 |};
 
-export type AppStyles = $ReadOnly<{||}>;
-
 export const themeColors: { [string]: ThemeColors } = {
   night: {
     color: 'hsl(210, 11%, 85%)',
@@ -35,5 +30,3 @@ export const themeColors: { [string]: ThemeColors } = {
 themeColors.default = themeColors.light;
 
 export const ThemeContext: Context<ThemeColors> = React.createContext(themeColors.default);
-
-export const stylesFromTheme = (name: ThemeName) => StyleSheet.create({});
diff --git a/src/types.js b/src/types.js
index f173bafa93f..f910057291f 100644
--- a/src/types.js
+++ b/src/types.js
@@ -3,7 +3,6 @@ import type { IntlShape } from 'react-intl';
 import type { DangerouslyImpreciseStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';
 
 import type { Auth, Topic, Message, Reaction, ReactionType, Narrow } from './api/apiTypes';
-import type { AppStyles } from './styles/theme';
 import type { ZulipVersion } from './utils/zulipVersion';
 
 export type * from './reduxTypes';
@@ -147,10 +146,6 @@ export type TopicExtended = {|
   unreadCount: number,
 |};
 
-export type Context = {|
-  styles: AppStyles,
-|};
-
 /**
  * A message we're in the process of sending.
  *

From 1a122933b6a63752f41725bf9e6609436429b1c8 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulipchat.com>
Date: Thu, 16 Apr 2020 13:46:36 -0700
Subject: [PATCH 4/6] theme types: Make `themeColors` annotation more specific.

The keys won't be just any string. Nice to tighten our types where
we can.
---
 src/reduxTypes.js   | 3 +++
 src/styles/theme.js | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/reduxTypes.js b/src/reduxTypes.js
index 83f3236bc8c..306d67fb6c1 100644
--- a/src/reduxTypes.js
+++ b/src/reduxTypes.js
@@ -254,6 +254,9 @@ export type RealmState = {|
   isAdmin: boolean,
 |};
 
+// TODO: Stop using the 'default' name. Any 'default' semantics should
+// only apply the device level, not within the app. See
+// https://github.com/zulip/zulip-mobile/issues/4009#issuecomment-619280681.
 export type ThemeName = 'default' | 'night';
 
 export type SettingsState = {|
diff --git a/src/styles/theme.js b/src/styles/theme.js
index 61547572dfb..2964e4ddbbd 100644
--- a/src/styles/theme.js
+++ b/src/styles/theme.js
@@ -2,6 +2,8 @@
 import React from 'react';
 import type { Context } from 'react';
 
+import type { ThemeName } from '../reduxTypes';
+
 export type ThemeColors = {|
   color: string,
   backgroundColor: string,
@@ -9,7 +11,7 @@ export type ThemeColors = {|
   dividerColor: string,
 |};
 
-export const themeColors: { [string]: ThemeColors } = {
+export const themeColors: { [name: ThemeName | 'light']: ThemeColors } = {
   night: {
     color: 'hsl(210, 11%, 85%)',
     backgroundColor: 'hsl(212, 28%, 18%)',

From 855de671efd0f5656ff94c2da670e3e32b53ac5d Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulipchat.com>
Date: Fri, 24 Apr 2020 11:16:00 -0700
Subject: [PATCH 5/6] ThemeColors [nfc]: Rename to ThemeData.

Best seen in the ThemeContext declaration, below, it's inconsistent
for one name to refer to "colors" and one not to. Furthermore,
"colors" makes it sound like this is the only theme-related data
that exists (which isn't necessarily true), or, worse, that there's
some other theme-related data, besides the colors, that's found
elsewhere, under a different name.

This type encompasses all data about the theme, so, rename it to
ThemeData.

Just `Theme` was considered. `ThemeData` was chosen, not because of
any semantics in Data, but because it's friendlier for newer
contributors who might not be using their "whole-word" setting when
doing a text search through the codebase. With `Theme`, a search
without using that setting would newly contain a lot of noise from,
e.g., ThemeName, ThemeProvider, ThemeContext, etc.

Greg explains some useful searching strategies at
https://github.com/zulip/zulip-mobile/pull/4046#issuecomment-619254798.
---
 src/boot/ThemeProvider.js                   | 4 ++--
 src/chat/ChatScreen.js                      | 4 ++--
 src/common/Input.js                         | 4 ++--
 src/common/Label.js                         | 4 ++--
 src/common/LineSeparator.js                 | 4 ++--
 src/common/LoadingBanner.js                 | 4 ++--
 src/common/OptionButton.js                  | 4 ++--
 src/common/OptionDivider.js                 | 4 ++--
 src/common/OptionRow.js                     | 4 ++--
 src/common/Popup.js                         | 4 ++--
 src/common/RawLabel.js                      | 4 ++--
 src/common/Screen.js                        | 4 ++--
 src/common/SectionHeader.js                 | 4 ++--
 src/common/SmartUrlInput.js                 | 4 ++--
 src/compose/ComposeBox.js                   | 4 ++--
 src/main/MainScreenWithTabs.js              | 4 ++--
 src/nav/ModalNavBar.js                      | 4 ++--
 src/nav/ModalSearchNavBar.js                | 4 ++--
 src/pm-conversations/PmConversationsCard.js | 4 ++--
 src/streams/StreamItem.js                   | 4 ++--
 src/styles/index.js                         | 2 +-
 src/styles/theme.js                         | 8 ++++----
 src/user-groups/UserGroupItem.js            | 4 ++--
 src/webview/MessageList.js                  | 4 ++--
 24 files changed, 49 insertions(+), 49 deletions(-)

diff --git a/src/boot/ThemeProvider.js b/src/boot/ThemeProvider.js
index bf9cbd0beb9..dbfa14cb444 100644
--- a/src/boot/ThemeProvider.js
+++ b/src/boot/ThemeProvider.js
@@ -6,7 +6,7 @@ import type { Node as React$Node } from 'react';
 import type { ThemeName, Dispatch } from '../types';
 import { connect } from '../react-redux';
 import { getSettings } from '../directSelectors';
-import { themeColors, ThemeContext } from '../styles/theme';
+import { themeData, ThemeContext } from '../styles/theme';
 
 type Props = $ReadOnly<{|
   dispatch: Dispatch,
@@ -21,7 +21,7 @@ class ThemeProvider extends PureComponent<Props> {
 
   render() {
     const { children, theme } = this.props;
-    return <ThemeContext.Provider value={themeColors[theme]}>{children}</ThemeContext.Provider>;
+    return <ThemeContext.Provider value={themeData[theme]}>{children}</ThemeContext.Provider>;
   }
 }
 
diff --git a/src/chat/ChatScreen.js b/src/chat/ChatScreen.js
index bc77a664da3..f95f3e2a4a3 100644
--- a/src/chat/ChatScreen.js
+++ b/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,
diff --git a/src/common/Input.js b/src/common/Input.js
index 39bb61ea1c7..3e4580050c3 100644
--- a/src/common/Input.js
+++ b/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: {
diff --git a/src/common/Label.js b/src/common/Label.js
index 621af02a610..33e2762deac 100644
--- a/src/common/Label.js
+++ b/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: {
diff --git a/src/common/LineSeparator.js b/src/common/LineSeparator.js
index ffc3c97c3c4..9b9d351f3d5 100644
--- a/src/common/LineSeparator.js
+++ b/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: {
diff --git a/src/common/LoadingBanner.js b/src/common/LoadingBanner.js
index 9f75daa6540..957d7a3b434 100644
--- a/src/common/LoadingBanner.js
+++ b/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) {
diff --git a/src/common/OptionButton.js b/src/common/OptionButton.js
index 17e50b43fe3..102c174e787 100644
--- a/src/common/OptionButton.js
+++ b/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,
diff --git a/src/common/OptionDivider.js b/src/common/OptionDivider.js
index eab90410722..777242f147a 100644
--- a/src/common/OptionDivider.js
+++ b/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: {
diff --git a/src/common/OptionRow.js b/src/common/OptionRow.js
index 408954542ad..3f14b5a0453 100644
--- a/src/common/OptionRow.js
+++ b/src/common/OptionRow.js
@@ -6,7 +6,7 @@ import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet
 import type { SpecificIconType } from './Icons';
 import Label from './Label';
 import ZulipSwitch from './ZulipSwitch';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import styles, { ThemeContext } from '../styles';
 
 type Props = $ReadOnly<{|
@@ -19,7 +19,7 @@ type Props = $ReadOnly<{|
 
 export default class OptionRow extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   styles = {
     icon: styles.settingsIcon,
diff --git a/src/common/Popup.js b/src/common/Popup.js
index fe09c6a30a5..1145afe6af9 100644
--- a/src/common/Popup.js
+++ b/src/common/Popup.js
@@ -3,7 +3,7 @@ import React, { PureComponent } from 'react';
 import type { Node as React$Node } from 'react';
 import { View, StyleSheet } from 'react-native';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 
 const styles = StyleSheet.create({
@@ -24,7 +24,7 @@ type Props = $ReadOnly<{|
 
 export default class Popup extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   render() {
     return (
diff --git a/src/common/RawLabel.js b/src/common/RawLabel.js
index 525ebf476ae..058c600b028 100644
--- a/src/common/RawLabel.js
+++ b/src/common/RawLabel.js
@@ -2,7 +2,7 @@
 import React, { PureComponent } from 'react';
 import { Text } from 'react-native';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 
 type Props = $ReadOnly<{|
@@ -23,7 +23,7 @@ type Props = $ReadOnly<{|
  */
 export default class RawLabel extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   styles = {
     label: {
diff --git a/src/common/Screen.js b/src/common/Screen.js
index 1788dbf807e..a333097196d 100644
--- a/src/common/Screen.js
+++ b/src/common/Screen.js
@@ -5,7 +5,7 @@ import type { Node as React$Node } from 'react';
 import { StyleSheet, View, ScrollView } from 'react-native';
 import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import styles, { ThemeContext } from '../styles';
 import type { Dimensions, LocalizableText, Dispatch } from '../types';
 import { connect } from '../react-redux';
@@ -78,7 +78,7 @@ type Props = $ReadOnly<{|
  */
 class Screen extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   static defaultProps = {
     centerContent: false,
diff --git a/src/common/SectionHeader.js b/src/common/SectionHeader.js
index 9d077a2af18..405cc9c5333 100644
--- a/src/common/SectionHeader.js
+++ b/src/common/SectionHeader.js
@@ -2,7 +2,7 @@
 import React, { PureComponent } from 'react';
 import { StyleSheet, View } from 'react-native';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 import Label from './Label';
 
@@ -19,7 +19,7 @@ type Props = $ReadOnly<{|
 
 export default class SectionHeader extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   render() {
     const { text } = this.props;
diff --git a/src/common/SmartUrlInput.js b/src/common/SmartUrlInput.js
index 055f13db4c9..1fdcb08013d 100644
--- a/src/common/SmartUrlInput.js
+++ b/src/common/SmartUrlInput.js
@@ -4,7 +4,7 @@ import { StyleSheet, TextInput, TouchableWithoutFeedback, View } from 'react-nat
 import type { ViewStyleProp } from 'react-native/Libraries/StyleSheet/StyleSheet';
 import type { NavigationEventSubscription, NavigationScreenProp } from 'react-navigation';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 import { autocompleteRealmPieces, autocompleteRealm, fixRealmUrl } from '../utils/url';
 import type { Protocol } from '../utils/url';
@@ -62,7 +62,7 @@ type State = {|
 
 export default class SmartUrlInput extends PureComponent<Props, State> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
   state = {
     value: '',
   };
diff --git a/src/compose/ComposeBox.js b/src/compose/ComposeBox.js
index 7aa749ca26e..3331632eda7 100644
--- a/src/compose/ComposeBox.js
+++ b/src/compose/ComposeBox.js
@@ -4,7 +4,7 @@ import { Platform, View, TextInput, findNodeHandle } from 'react-native';
 import type { LayoutEvent } from 'react-native/Libraries/Types/CoreEventTypes';
 import TextInputReset from 'react-native-text-input-reset';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 import type {
   Auth,
@@ -104,7 +104,7 @@ export const updateTextInput = (textInput: ?TextInput, text: string): void => {
 
 class ComposeBox extends PureComponent<Props, State> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   messageInput: ?TextInput = null;
   topicInput: ?TextInput = null;
diff --git a/src/main/MainScreenWithTabs.js b/src/main/MainScreenWithTabs.js
index be3ad3f9447..fc6356124ac 100644
--- a/src/main/MainScreenWithTabs.js
+++ b/src/main/MainScreenWithTabs.js
@@ -2,14 +2,14 @@
 import React, { PureComponent } from 'react';
 import { View } from 'react-native';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import styles, { ThemeContext } from '../styles';
 import { OfflineNotice, ZulipStatusBar } from '../common';
 import MainTabs from './MainTabs';
 
 export default class MainScreenWithTabs extends PureComponent<{||}> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   render() {
     return (
diff --git a/src/nav/ModalNavBar.js b/src/nav/ModalNavBar.js
index d9b4aec7304..a64ba3a0b39 100644
--- a/src/nav/ModalNavBar.js
+++ b/src/nav/ModalNavBar.js
@@ -4,7 +4,7 @@ import React, { PureComponent } from 'react';
 import { View } from 'react-native';
 
 import type { Dispatch, LocalizableText } from '../types';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import styles, { ThemeContext, NAVBAR_SIZE } from '../styles';
 import { connect } from '../react-redux';
 
@@ -20,7 +20,7 @@ type Props = $ReadOnly<{|
 
 class ModalNavBar extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   render() {
     const { dispatch, canGoBack, title } = this.props;
diff --git a/src/nav/ModalSearchNavBar.js b/src/nav/ModalSearchNavBar.js
index 71ffa085648..189de905828 100644
--- a/src/nav/ModalSearchNavBar.js
+++ b/src/nav/ModalSearchNavBar.js
@@ -3,7 +3,7 @@ import React, { PureComponent } from 'react';
 import { View } from 'react-native';
 
 import type { Dispatch } from '../types';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext, NAVBAR_SIZE } from '../styles';
 import { connect } from '../react-redux';
 import SearchInput from '../common/SearchInput';
@@ -19,7 +19,7 @@ type Props = $ReadOnly<{|
 
 class ModalSearchNavBar extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   static defaultProps = {
     canGoBack: true,
diff --git a/src/pm-conversations/PmConversationsCard.js b/src/pm-conversations/PmConversationsCard.js
index 3474091bc6a..db74fd28b10 100644
--- a/src/pm-conversations/PmConversationsCard.js
+++ b/src/pm-conversations/PmConversationsCard.js
@@ -3,7 +3,7 @@
 import React, { PureComponent } from 'react';
 import { View, StyleSheet } from 'react-native';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 import type { Dispatch, PmConversationData, UserOrBot } from '../types';
 import { connect } from '../react-redux';
@@ -43,7 +43,7 @@ type Props = $ReadOnly<{|
  * */
 class PmConversationsCard extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   render() {
     const { dispatch, conversations, usersByEmail } = this.props;
diff --git a/src/streams/StreamItem.js b/src/streams/StreamItem.js
index 13ba58b748d..d5a22c0bc7f 100644
--- a/src/streams/StreamItem.js
+++ b/src/streams/StreamItem.js
@@ -2,7 +2,7 @@
 import React, { PureComponent } from 'react';
 import { StyleSheet, View } from 'react-native';
 
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import styles, { ThemeContext } from '../styles';
 import { RawLabel, Touchable, UnreadCount, ZulipSwitch } from '../common';
 import { foregroundColorFromBackground } from '../utils/color';
@@ -61,7 +61,7 @@ type Props = $ReadOnly<{|
  */
 export default class StreamItem extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   static defaultProps = {
     isMuted: false,
diff --git a/src/styles/index.js b/src/styles/index.js
index bce2a47bbd2..06e7f36384a 100644
--- a/src/styles/index.js
+++ b/src/styles/index.js
@@ -7,7 +7,7 @@ import { statics as navStyles } from './navStyles';
 import utilityStyles from './utilityStyles';
 
 export * from './constants';
-export type { ThemeColors } from './theme';
+export type { ThemeData } from './theme';
 export { ThemeContext } from './theme';
 
 export default StyleSheet.create({
diff --git a/src/styles/theme.js b/src/styles/theme.js
index 2964e4ddbbd..261454b2512 100644
--- a/src/styles/theme.js
+++ b/src/styles/theme.js
@@ -4,14 +4,14 @@ import type { Context } from 'react';
 
 import type { ThemeName } from '../reduxTypes';
 
-export type ThemeColors = {|
+export type ThemeData = {|
   color: string,
   backgroundColor: string,
   cardColor: string,
   dividerColor: string,
 |};
 
-export const themeColors: { [name: ThemeName | 'light']: ThemeColors } = {
+export const themeData: { [name: ThemeName | 'light']: ThemeData } = {
   night: {
     color: 'hsl(210, 11%, 85%)',
     backgroundColor: 'hsl(212, 28%, 18%)',
@@ -29,6 +29,6 @@ export const themeColors: { [name: ThemeName | 'light']: ThemeColors } = {
     dividerColor: 'hsla(0, 0%, 0%, 0.12)',
   },
 };
-themeColors.default = themeColors.light;
+themeData.default = themeData.light;
 
-export const ThemeContext: Context<ThemeColors> = React.createContext(themeColors.default);
+export const ThemeContext: Context<ThemeData> = React.createContext(themeData.default);
diff --git a/src/user-groups/UserGroupItem.js b/src/user-groups/UserGroupItem.js
index 7dcf0b093e7..5abb67a4bcc 100644
--- a/src/user-groups/UserGroupItem.js
+++ b/src/user-groups/UserGroupItem.js
@@ -5,7 +5,7 @@ import { StyleSheet, View } from 'react-native';
 import { IconPeople } from '../common/Icons';
 import { RawLabel, Touchable } from '../common';
 import styles, { ThemeContext } from '../styles';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 
 const componentStyles = StyleSheet.create({
   text: {
@@ -28,7 +28,7 @@ type Props = $ReadOnly<{|
 
 export default class UserGroupItem extends PureComponent<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   handlePress = () => {
     const { name, onPress } = this.props;
diff --git a/src/webview/MessageList.js b/src/webview/MessageList.js
index 85440fa5df7..fca3aa3baef 100644
--- a/src/webview/MessageList.js
+++ b/src/webview/MessageList.js
@@ -25,7 +25,7 @@ import type {
   UserOrBot,
   EditMessage,
 } from '../types';
-import type { ThemeColors } from '../styles';
+import type { ThemeData } from '../styles';
 import { ThemeContext } from '../styles';
 import { connect } from '../react-redux';
 import {
@@ -144,7 +144,7 @@ const webviewAssetsUrl = new URL('webview/', assetsUrl);
 
 class MessageList extends Component<Props> {
   static contextType = ThemeContext;
-  context: ThemeColors;
+  context: ThemeData;
 
   webview: ?WebView;
   readyRetryInterval: IntervalID | void;

From 68ca746818b08646b44b6cae73cda23767c5caf9 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Fri, 24 Jul 2020 15:33:38 -0700
Subject: [PATCH 6/6] 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 in between) 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 b530f6cba, when we were removing
the `key` hack for styles.)

Fixes: #1946
---
 docs/architecture/react.md      | 35 +++++++++------------
 src/boot/TranslationProvider.js | 54 ++++++++++++++++++++-------------
 2 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/docs/architecture/react.md b/docs/architecture/react.md
index ca16283edff..859ea8aa176 100644
--- a/docs/architecture/react.md
+++ b/docs/architecture/react.md
@@ -103,7 +103,8 @@ long as the code adheres to core Redux principles:
 ### Context
 
 We're on board with the current API where possible; there's a
-third-party library we use that isn't there yet.
+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
 
@@ -164,29 +165,21 @@ updates to the context, and not just by the consumer's own
 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`, but we've made that a `PureComponent`,
-so it doesn't get updated when `intl` changes. Our workaround until
-now has been a "`key` hack":
-
-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.
-
-In the next commit, we stop using the `key` hack and instead make
-`TranslationContextTranslator` "speak" the old API better.
+`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 it's the only component in which we implement
-`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
diff --git a/src/boot/TranslationProvider.js b/src/boot/TranslationProvider.js
index ac14311c17f..b12031da51b 100644
--- a/src/boot/TranslationProvider.js
+++ b/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>
     );