Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove singletons in favor of React context (hooks-based implementation) #221

Open
wants to merge 15 commits into
base: master
from

Conversation

@noratarano
Copy link
Contributor

commented Aug 13, 2019

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.

Usage:

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

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

  1. 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.
  2. We listen to directional changes through props using the withDirection HOC now, so styled components are wrapped in two HOCs, which means that to test with enzyme ShallowWrapper you have to dive twice.

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 at lib/deprecated/withStyles during their own migration, though it shouldn't be required, especially since the new version requires react >= 16.8.

Deprecation process that I'm envisioning:

  1. Update this library to rely on context and fall back to ThemedStyleSheet (this PR)
  2. 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.
  3. Once the migration of the airbnb repos are complete, completely deprecate ThemedStyleSheet and the deprecated/withStyles HOC.

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


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

This comment has been minimized.

Copy link
@noratarano

noratarano Aug 13, 2019

Author Contributor

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

@noratarano noratarano requested review from lencioni, ljharb, brucewpaul, goatslacker and majapw and removed request for lencioni Aug 13, 2019

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

This comment has been minimized.

Copy link
@noratarano

noratarano Aug 13, 2019

Author Contributor

This is the old withStyles moved to deprecated/

@noratarano

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

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
Copy link
Collaborator

left a comment

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?


const StylesThemeContext = createContext();

export default StylesThemeContext;

This comment has been minimized.

Copy link
@noratarano

noratarano Aug 14, 2019

Author Contributor

Make into a single context.

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

This comment has been minimized.

Copy link
@noratarano

noratarano Aug 14, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@ahuth

ahuth Aug 14, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@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

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

@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
Copy link

left a comment

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

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

This comment has been minimized.

Copy link
@ahuth

ahuth Aug 14, 2019

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;

This comment has been minimized.

Copy link
@noratarano

noratarano Aug 14, 2019

Author Contributor

OOoh! Interesting. 👍

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

This comment has been minimized.

Copy link
@ahuth

ahuth Aug 14, 2019

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

This comment has been minimized.

Copy link
@ahuth
@@ -75,6 +79,7 @@
"react-with-direction": "^1.1.0"
},
"dependencies": {
"airbnb-prop-types": "^2.14.0",

This comment has been minimized.

Copy link
@ahuth

ahuth Aug 15, 2019

I may have missed it, but is this used anywhere?

This comment has been minimized.

Copy link
@noratarano

noratarano Aug 15, 2019

Author Contributor

@ahuth yes! This is where we get geComponentName from.

@noratarano noratarano force-pushed the nora--remove-singletons branch from 6ef2ed4 to f25e134 Aug 15, 2019

@noratarano noratarano force-pushed the nora--remove-singletons branch from 014e11d to f25e134 Aug 15, 2019

Nora Tarano

@noratarano noratarano force-pushed the nora--remove-singletons branch from cf64f02 to 61a9726 Aug 15, 2019

};

return cacheRef.current;
}

This comment has been minimized.

Copy link
@noratarano

noratarano Aug 15, 2019

Author Contributor

@ahuth What do you think about this now?

Nora Tarano
@@ -1,6 +1,14 @@
{
"extends": "airbnb",

"plugins": [
"react-hooks",

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 15, 2019

Collaborator

fwiw you can bring this in with "extends": ["airbnb", "airbnb/hooks"] instead, if you're on the latest version of the config

@ljharb ljharb dismissed their stale review Aug 15, 2019

peer dep is now correct; although i still don't think using hooks inside this package is worth the breaking change, assuming legacy context would work the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.