diff --git a/docs/architecture/react.md b/docs/architecture/react.md index 37d1e285900..859ea8aa176 100644 --- a/docs/architecture/react.md +++ b/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 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 => ( - + - + 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..dbfa14cb444 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 { themeData, ThemeContext } from '../styles/theme'; type Props = $ReadOnly<{| dispatch: Dispatch, @@ -16,32 +14,17 @@ type Props = $ReadOnly<{| children: React$Node, |}>; -class StylesProvider extends PureComponent { - static childContextTypes = { - styles: () => {}, - }; - +class ThemeProvider extends PureComponent { static defaultProps = { theme: 'default', }; - getChildContext() { - const { theme } = this.props; - const styles = stylesFromTheme(theme); - return { styles }; - } - render() { const { children, theme } = this.props; - - return ( - - {children} - - ); + return {children}; } } export default connect(state => ({ theme: getSettings(state).theme, -}))(StylesProvider); +}))(ThemeProvider); 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 ( - + {this.props.children} ); @@ -84,21 +110,7 @@ class TranslationProvider extends PureComponent { 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. - */ - + {children} ); 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 { 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 { 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 { 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 { 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 { 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 { 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 { 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 { 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 { 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 { 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 { 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 { 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 { 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 { 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 { static contextType = ThemeContext; - context: ThemeColors; + context: ThemeData; render() { const { dispatch, conversations, usersByEmail } = this.props; 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/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 { 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 0085ee00984..261454b2512 100644 --- a/src/styles/theme.js +++ b/src/styles/theme.js @@ -1,20 +1,17 @@ /* @flow strict-local */ import React from 'react'; import type { Context } from 'react'; -import { StyleSheet } from 'react-native'; -import type { ThemeName } from '../types'; +import type { ThemeName } from '../reduxTypes'; -export type ThemeColors = {| +export type ThemeData = {| color: string, backgroundColor: string, cardColor: string, dividerColor: string, |}; -export type AppStyles = $ReadOnly<{||}>; - -export const themeColors: { [string]: ThemeColors } = { +export const themeData: { [name: ThemeName | 'light']: ThemeData } = { night: { color: 'hsl(210, 11%, 85%)', backgroundColor: 'hsl(212, 28%, 18%)', @@ -32,8 +29,6 @@ export const themeColors: { [string]: ThemeColors } = { dividerColor: 'hsla(0, 0%, 0%, 0.12)', }, }; -themeColors.default = themeColors.light; - -export const ThemeContext: Context = React.createContext(themeColors.default); +themeData.default = themeData.light; -export const stylesFromTheme = (name: ThemeName) => StyleSheet.create({}); +export const ThemeContext: Context = React.createContext(themeData.default); diff --git a/src/subscriptions/__tests__/subscriptionsReducer-test.js b/src/subscriptions/__tests__/subscriptionsReducer-test.js index d1acbadde71..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,40 +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', - 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); @@ -137,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); @@ -184,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 = []; @@ -216,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); }); @@ -271,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], }, }); @@ -297,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 = []; @@ -328,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 = []; @@ -365,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 = []; 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. * 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 { 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 { static contextType = ThemeContext; - context: ThemeColors; + context: ThemeData; webview: ?WebView; readyRetryInterval: IntervalID | void;