Remove singletons in favor of React context (hooks-based implementation)(backward compatible with all versions of react) #221

noratarano · 2019-08-13T19:54:22Z

Summary

Replace the withStyles implementation with one that uses React.Context, not singletons. We fall back to the singletons if no react-with-styles themes or interfaces are provided through context. The ThemedStyleSheet API is still supported as we now detect Hooks/Context and conditionally use the old hooks-based implementation and the new one.

Usage:

// The same as before!
const ComponentWithStyles = withStyles(theme => styles, options)(Component);

// Before
ThemedStyleSheet.registerTheme(Theme);
ThemedStyleSheet.registerInterface(Theme);

// After!
<WithStylesContext.Provider 
  value={{ 
    stylesInterface: AphroditeInterface, 
    stylesTheme: AirbnbTheme, 
  }}
>
  <App />
</WithStylesContext.Provider>

This change also introduces a few notable differences in the way that withStyles works:

Because the theme is no longer available outside of the React tree, every instance of a component will “create” styles from the theme, as opposed to the one time per withStyles call that the other component was doing before.

Updates to the README are captured here: #223

Details

Singletons don't scale, and we're feeling that at Airbnb. In an effort to modernize our repos and scale our infrastructure, we're moving away from this singleton implementation and relying on the latest React context API to register themes and interfaces. Because we have so many systems sharing this library (react-dates, rheostat, and internal libs), including ones that require the singleton implementation (ahem hypernova) this implementation falls back to the singleton implementation (through ThemedStyleSheet) to ensure that bundles that don't set theming can continue to work until the process is complete.

Lastly, while this is a breaking change, consumers of this lib can still access the old version of withStyles depending on their version of React. Those with React <= 16.8 will get the old implementation. Those with the new React will get the new one based on hooks.

Deprecation process that I'm envisioning:

Update this library to rely on context and fall back to ThemedStyleSheet (this PR)
Update all Airbnb production and development code to provide the interface and theme through context using WithStylesContext.Provider. If some aren't captured, we fall back to the previous implementation.
Once the migration of the airbnb repos are complete, completely deprecate ThemedStyleSheet and the deprecated/withStyles HOC.

Before making useStyles available:

Make sure stylesFn is static (doesn't change)
Make sure stylesFn doesn't depend on props (cache created results?)
useDirection or DirectionContext so that useStyles can be used alone

Future performance improvements:

Cache created styles in some styles cache so that we don't have to create styles for every instance of the component

Testing

I wrote a lot of tests for this. Please take a look at all the assertions.

I also tested this implementation on the internal design system storybook, rheostat and react-dates (with both the Aphrodite and CSS interfaces). 👍

Reviewers

@lencioni @ljharb @goatslacker @brucewpaul @TaeKimJR @majapw @indiesquidge

noratarano · 2019-08-13T19:55:20Z

src/ThemedStyleSheet.js

+
+// Exported until we deprecate this API completely
+export { get as _getTheme };
+


This is so we can fall back to these singletons from the contextual implementation.

noratarano · 2019-08-13T19:57:20Z

src/deprecated/withStyles.jsx

@@ -0,0 +1,220 @@
+/* eslint react/forbid-foreign-prop-types: off */


This is the old withStyles moved to deprecated/

noratarano · 2019-08-13T20:35:22Z

~~Sounds like there may be an issue with our implementation of page slots. I'm taking a look and will ping again once I confirmed this implementation is OK. Apologies!~~

ljharb

Using hooks means it's a breaking change, and we'll need to bump the peer dep to react 16.8 or higher.

Is there a reason we can't use traditional context?

noratarano · 2019-08-14T02:00:40Z

src/StylesThemeContext.js

+
+const StylesThemeContext = createContext();
+
+export default StylesThemeContext;


Make into a single context.

noratarano · 2019-08-14T02:01:26Z

src/useThemedStyleSheet.js

+      resolve,
+      flush,
+    }),
+    [create, resolve, flush],


Avoid diffing so many things. Only diff the values that matter once, so no need to diff create/resolve/flush separately.

The values returned by useCallback are memoized already, and I'm not sure we need to re-memo them again. In fact, useCallback could be implemented with useMemo.

Should be able to:

return { create, resolve, flush }

Or

return [create, resolve, flush];

noratarano · 2019-08-14T03:06:25Z

@ljharb This is intended to be a breaking change. This API is just much easier to maintain and read through. There's no particular reason other than maintainability for this, and Airbnb is using React 16.8, and I believe we're comfortable with this breaking change.

Can you share why it's not recommended to introduce this restriction?

ljharb · 2019-08-14T03:47:37Z

@noratarano the X is just for updating the peer dep; the rest is just my opinion.

Public open source projects aren't just for Airbnb, and while it's great that Airbnb is now on 16.8 (congrats!), making it a requirement excludes some portion of the internet from being able to use it - as of a month or two ago, it'd have excluded Airbnb too. In general, it's best to aim for maximal backwards compatibility, so that this project can belong anywhere.

If the API is identically achievable and maintainable using legacy context - even if it's slightly less fun to work with - it seems to me like a moral imperative to maximize how many people can use the project, not to optimize for the experience of the < 5 developers who will touch this repo.

All that said, if the peer dep is bumped, i'll remove my X, and I'll trust yall to make the decisions you feel best.

ahuth

Looks good so far! Left a couple comments regarding hooks, and will leave some more comments

ahuth · 2019-08-14T17:48:04Z

src/useStylesInterface.js

+  // Fallback
+  const registeredInterface = _getInterface();
+  if (!fallbackInterface) {
+    setFallbackInterface(registeredInterface);


If _getInterface is a simple getter, would there be any issues with executing it each time the hooks run?

const stylesInterface = useContext(StylesInterfaceContext); if (stylesInterface) { return stylesInterface; } // Fallback return _getInterface();

Or, if we don't want to run it every time the hook is called, we could avoid the setState (and its re-render) with a ref:

const stylesInterface = useContext(StylesInterfaceContext); const fallbackRef = useRef(); if (stylesInterface) { return stylesInterface; } if (!fallbackRef.current) { fallbackRef.current = _getInterface(); } return fallbackRef.current;

OOoh! Interesting. 👍

ahuth · 2019-08-14T18:00:57Z

src/useThemedStyleSheet.js

+      resolve,
+      flush,
+    }),
+    [create, resolve, flush],


The values returned by useCallback are memoized already, and I'm not sure we need to re-memo them again. In fact, useCallback could be implemented with useMemo.

Should be able to:

return { create, resolve, flush }

Or

return [create, resolve, flush];

ahuth · 2019-08-14T18:01:36Z

src/withStyles.jsx

+      // Calculate and cache the styles definition for this combination of global state values. This
+      // value will only be recalculated if the create function changes, which in turn will only
+      // change if any of the global state we depend on changes.
+      const styles = useMemo(() => create(stylesFn), [create]);


package.json

indiesquidge · 2019-08-22T19:50:37Z

src/deprecated/withStyles.jsx

@@ -0,0 +1,218 @@
+import React from 'react';


Is this file completely untouched? I.e., you just moved what was previous at src/withStyles.jsx into this deprecated folder? Or did you change other things around as well?

indiesquidge · 2019-08-22T19:52:16Z

test/deprecated/withStyles_test.jsx

@@ -0,0 +1,555 @@
+import React from 'react';


Same question here: is this file untouched other than the move from test/withStyles_test.jsx → test/deprecated/withStyles_test.jsx?

indiesquidge · 2019-08-22T19:54:08Z

test/withStyles_test.jsx

@@ -1,555 +1,619 @@
+/* eslint-disable no-unused-vars */
+


Why this tho?

Without disabling this I can't do:

describe('foo', () => { let myUsedVar; beforeEach(() => { myUsedVar = imUsingIt(); }); it('uses this var', () => { console.log(myUsedVar); }); });

This fails. 😢

Ah. Well that's a silly failure, but also Just Another Reason™ to avoid setup functions like beforeEact et al that share state, especially given that people can nest these types of things and make it super hard in a long test suite to figure out what is actually going on (I experienced this often on the Host product code with tests with nested blocks and setup helpers). I don't believe tests are were we should be keeping things DRY, and would much rather see each test verbosely express everything it needs.

Good point. I'll definitely keep that in mind going forward ^_^

…--remove-singletons

ljharb

Awesome, this is looking great!

ljharb · 2019-08-23T04:13:20Z

package.json

@@ -55,12 +56,16 @@
    "chai": "^4.2.0",
    "enzyme": "^3.10.0",
    "enzyme-adapter-react-16": "^1.14.0",


this should be removed, since the helper is being used now

ljharb · 2019-08-23T04:13:57Z

src/WithStylesContext.js

+
+  return {
+    Provider: () => {
+      throw new ReferenceError('WithStylesContext requires React 16.6 or later');


createContext works in 16.3+, i believe

src/useStyles.js

ljharb · 2019-08-23T04:14:51Z

src/useStyles.js

+  let { stylesInterface, stylesTheme: theme } = useContext(WithStylesContext);
+
+  // Fallback to the singleton implementation
+  stylesInterface = stylesInterface || _getInterface();


does _getInterface() make it available for future useContext calls, or should this somehow be persisted?

No it doesn't... I see what you mean. 🤔

ljharb · 2019-08-23T04:16:33Z

test/ifReactHelpers.js

+import ifReact from 'enzyme-adapter-react-helper/build/ifReact';
+
+export function describeIfReact(version, ...rest) {
+  return ifReact(version, describe, describe.skip)(...rest);


test/withStylesWithHooks_test.jsx

Co-Authored-By: Jordan Harband <ljharb@gmail.com>

noratarano · 2019-08-26T20:58:12Z

Merging this :) Will update README or make any additional changes in separate PRs before publishing the next version.

Nora Tarano added 8 commits August 9, 2019 11:06

Remove singletons / Checkpoint

aa24736

update version of jsdom to maintain support for node 6

8989282

Add support for pureComponent as React.memo and fix direction

19ab350

Add fallback to the ThemedStyleSheet API when no context was provided

bc754c7

Move contextual folder to root

ca6340d

Add direction to proptypes

7d4eed1

Fix component name

df6b7d5

Copy statics to the memoized component as well

efcd432

noratarano commented Aug 13, 2019

View reviewed changes

noratarano requested review from lencioni, ljharb, brucewpaul, goatslacker and majapw and removed request for lencioni August 13, 2019 19:56

noratarano commented Aug 13, 2019

View reviewed changes

ljharb previously requested changes Aug 13, 2019

View reviewed changes

noratarano commented Aug 14, 2019

View reviewed changes

ahuth reviewed Aug 14, 2019

View reviewed changes

Nora Tarano added 2 commits August 14, 2019 17:22

Use only one context type, WithStylesContext

19d0f48

Use a cache to avoid re-creating and re-resolving styles

00afde9

ahuth reviewed Aug 15, 2019

View reviewed changes

package.json Show resolved Hide resolved

Nora Tarano added 2 commits August 15, 2019 11:57

Do not cache resolved props since they should change if props change

dc8894c

Add react-hooks eslint plugin

f25e134

noratarano force-pushed the nora--remove-singletons branch from 6ef2ed4 to f25e134 Compare August 15, 2019 18:57

Nora Tarano added 2 commits August 21, 2019 17:25

Remove utilities we don't want to make public

8cbb1bd

Change error messaging

5f0ca91

noratarano requested review from ahuth and TaeKimJR August 22, 2019 00:32

majapw approved these changes Aug 22, 2019

View reviewed changes

TaeKimJR approved these changes Aug 22, 2019

View reviewed changes

Nora Tarano added 2 commits August 22, 2019 12:32

Feature detect inside WithStylesWithHooks

5ac8a14

Improve test error messages by using to.have.property

c13de70

indiesquidge reviewed Aug 22, 2019

View reviewed changes

Nora Tarano added 2 commits August 22, 2019 14:48

Fix tests so they run for the right versions of react

241de38

Merge branch 'nora--remove-singletons-backwards-compatible' into nora…

2c4c254

…--remove-singletons

noratarano changed the title ~~Remove singletons in favor of React context (hooks-based implementation)~~ Remove singletons in favor of React context (hooks-based implementation)(backward compatible) Aug 22, 2019

noratarano changed the title ~~Remove singletons in favor of React context (hooks-based implementation)(backward compatible)~~ Remove singletons in favor of React context (hooks-based implementation)(backward compatible with all versions of react) Aug 23, 2019

Add back css export for backward compatibility

9b447c6

noratarano force-pushed the nora--remove-singletons branch from d733f77 to 9b447c6 Compare August 23, 2019 00:31

Copy over all statics to avoid errors

58ce44b

noratarano force-pushed the nora--remove-singletons branch from 713e189 to 58ce44b Compare August 23, 2019 03:02

ljharb reviewed Aug 23, 2019

View reviewed changes

ljharb added semver-minor: new stuff and removed semver-major: breaking change labels Aug 23, 2019

noratarano and others added 2 commits August 23, 2019 14:13

Update test/withStylesWithHooks_test.jsx

d1ad51f

Co-Authored-By: Jordan Harband <ljharb@gmail.com>

Apply suggestion: Update React version for createContext

1c783f6

noratarano force-pushed the nora--remove-singletons branch from f65ff92 to 1c783f6 Compare August 23, 2019 21:14

noratarano merged commit 599f5ef into master Aug 26, 2019

noratarano deleted the nora--remove-singletons branch August 26, 2019 20:58

This was referenced Aug 29, 2019

Move the hooks implementation to its own HOC and re-implement withStyles with context as a class component #225

Merged

Version 4.0.0 #228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove singletons in favor of React context (hooks-based implementation)(backward compatible with all versions of react) #221

Remove singletons in favor of React context (hooks-based implementation)(backward compatible with all versions of react) #221

noratarano commented Aug 13, 2019 •

edited

Loading

noratarano Aug 13, 2019

noratarano Aug 13, 2019

noratarano commented Aug 13, 2019 •

edited

Loading

ljharb left a comment

noratarano Aug 14, 2019

noratarano Aug 14, 2019 •

edited

Loading

ahuth Aug 14, 2019

noratarano commented Aug 14, 2019 •

edited

Loading

ljharb commented Aug 14, 2019

ahuth left a comment

ahuth Aug 14, 2019 •

edited

Loading

noratarano Aug 14, 2019

ahuth Aug 14, 2019

ahuth Aug 14, 2019

indiesquidge Aug 22, 2019

indiesquidge Aug 22, 2019

indiesquidge Aug 22, 2019

noratarano Aug 23, 2019 •

edited

Loading

indiesquidge Aug 23, 2019

noratarano Aug 23, 2019

ljharb left a comment

ljharb Aug 23, 2019

ljharb Aug 23, 2019

ljharb Aug 23, 2019

noratarano Aug 23, 2019

ljharb Aug 23, 2019

noratarano commented Aug 26, 2019


		// Exported until we deprecate this API completely
		export { get as _getTheme };

		@@ -0,0 +1,220 @@
		/* eslint react/forbid-foreign-prop-types: off */


		const StylesThemeContext = createContext();

		export default StylesThemeContext;

Remove singletons in favor of React context (hooks-based implementation)(backward compatible with all versions of react) #221

Remove singletons in favor of React context (hooks-based implementation)(backward compatible with all versions of react) #221

Conversation

noratarano commented Aug 13, 2019 • edited Loading

Summary

Details

Testing

Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noratarano commented Aug 13, 2019 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noratarano Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noratarano commented Aug 14, 2019 • edited Loading

ljharb commented Aug 14, 2019

ahuth left a comment

Choose a reason for hiding this comment

ahuth Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noratarano Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noratarano commented Aug 26, 2019

noratarano commented Aug 13, 2019 •

edited

Loading

noratarano commented Aug 13, 2019 •

edited

Loading

noratarano Aug 14, 2019 •

edited

Loading

noratarano commented Aug 14, 2019 •

edited

Loading

ahuth Aug 14, 2019 •

edited

Loading

noratarano Aug 23, 2019 •

edited

Loading