Skip to content

Commit

Permalink
styles: Dismantle pipeline for styles using legacy Context API.
Browse files Browse the repository at this point in the history
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).

So, also, remove a bit of documentation that explained the hack. The
section was necessary, previously, but it hindered an easy
understanding of the "Pure Component Principle" and our use of it,
which is a main idea of that doc.

Fixes: zulip#1946
  • Loading branch information
Chris Bobbe authored and gnprice committed May 1, 2020
1 parent dfd9cda commit c0aac7d
Show file tree
Hide file tree
Showing 5 changed files with 7 additions and 57 deletions.
21 changes: 0 additions & 21 deletions docs/architecture/react.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,27 +100,6 @@ long as the code adheres to core Redux principles:
Redux reducers and treating every `state` value (and hence all its
elements' elements, etc.) as immutable.

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

### The exception: `MessageList`

We have one React component that we wrote (beyond `connect` calls) that
Expand Down
6 changes: 3 additions & 3 deletions src/ZulipMobile.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import React from 'react';
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';
Expand All @@ -25,11 +25,11 @@ export default () => (
<AppEventHandlers>
<AppDataFetcher>
<TranslationProvider>
<StylesProvider>
<ThemeProvider>
<BackNavigationHandler>
<AppWithNavigation />
</BackNavigationHandler>
</StylesProvider>
</ThemeProvider>
</TranslationProvider>
</AppDataFetcher>
</AppEventHandlers>
Expand Down
25 changes: 4 additions & 21 deletions src/boot/StylesProvider.js → src/boot/ThemeProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,42 +6,25 @@ import type { Node as React$Node } from 'react';
import type { ThemeName, Dispatch } from '../types';
import { connect } from '../react-redux';
import { getSettings } from '../directSelectors';
import { stylesFromTheme, themeColors, ThemeContext } from '../styles/theme';

const Dummy = props => props.children;
import { themeColors, ThemeContext } from '../styles/theme';

type Props = $ReadOnly<{|
dispatch: Dispatch,
theme: ThemeName,
children: React$Node,
|}>;

class StylesProvider extends PureComponent<Props> {
static childContextTypes = {
styles: () => {},
};

class ThemeProvider extends PureComponent<Props> {
static defaultProps = {
theme: 'default',
};

getChildContext() {
const { theme } = this.props;
const styles = stylesFromTheme(theme);
return { styles };
}

render() {
const { children, theme } = this.props;

return (
<ThemeContext.Provider value={themeColors[theme]}>
<Dummy key={theme}>{children}</Dummy>
</ThemeContext.Provider>
);
return <ThemeContext.Provider value={themeColors[theme]}>{children}</ThemeContext.Provider>;
}
}

export default connect(state => ({
theme: getSettings(state).theme,
}))(StylesProvider);
}))(ThemeProvider);
7 changes: 0 additions & 7 deletions src/styles/theme.js
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -12,8 +9,6 @@ export type ThemeColors = {|
dividerColor: string,
|};

export type AppStyles = $ReadOnly<{||}>;

export const themeColors: { [string]: ThemeColors } = {
night: {
color: 'hsl(210, 11%, 85%)',
Expand All @@ -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({});
5 changes: 0 additions & 5 deletions src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

export type * from './reduxTypes';
export type * from './api/apiTypes';
Expand Down Expand Up @@ -146,10 +145,6 @@ export type TopicExtended = {|
unreadCount: number,
|};

export type Context = {|
styles: AppStyles,
|};

/**
* A message we're in the process of sending.
*
Expand Down

0 comments on commit c0aac7d

Please sign in to comment.