Skip to content

emotion 9 --> emotion 10 #3321

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

Merged
merged 9 commits into from
Apr 26, 2019
Merged

emotion 9 --> emotion 10 #3321

merged 9 commits into from
Apr 26, 2019

Conversation

gwyneplaine
Copy link
Collaborator

@gwyneplaine gwyneplaine commented Jan 4, 2019

Refactor our usage of create-emotion as of v2.2.0 to emotion 10, and reverting the addition of the custom emotion instance passed into each component.

Please see the following PR for context on the create-emotion change: #3260
This is also an indirect response to the following issue #3317 caused by the above PR 😄

There's a few valid reasons for the upgrade:

  • Zero configuration SSR is very appealing, especially for users who are using css-in-js solutions other than emotion.
  • nonce support for CSP is trivial, and doesn't involve creating a custom emotion instance.
  • Users on latest version of emotion, don't need to worry about pulling in multiple versions of emotion because of react-select.
  • Not passing in a custom emotion instance to every component that needs styling is actually kind of ok, it means the api for our components aren't bound to a specific css-in-js solution, making future changes to our styling opinions less obtrusive.

Possible arguments against emotion 10:

  • Support for react 15 users who aren't able to use the new context api
  • This could be a breaking change for users using the components api ** (see rebuttal below)

** this is only true for users who are completely rewriting components, and doing their own style composition. Users on emotion 10 who are doing this should be familiar with this syntax already, users who are using any other css-in-js solution should have the necessary tools to create functioning custom components without having to use emotion.

Things I want to validate before this PR goes through:

  • That support for react 15 is a non-issue @JedWatson
  • That we're leveraging the correct patterns in our use of emotion 10 @mitchellhamilton
  • The assertion that while this is a non-trivial change to our internal css-in-js implementation, it is ultimately a non-breaking change for users of the custom components api.

@@ -48,7 +50,7 @@
"css-loader": "^0.28.7",
"cypress": "^1.4.1",
"dotenv": "^5.0.1",
"enzyme": "^3.3.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

upgrade enzyme so it plays well with emotion 10 and new context api

src/Select.js Outdated
import memoizeOne from 'memoize-one';
import createEmotion, { type Emotion } from 'create-emotion';
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed createEmotion in favor of emotion 10s CacheProvider and createCache for adding nonce support.

src/Select.js Outdated
@@ -1703,7 +1696,7 @@ export default class Select extends Component<Props, State> {
onTopArrive={onMenuScrollToTop}
onBottomArrive={onMenuScrollToBottom}
>
<ScrollBlock emotion={this.emotion} isEnabled={menuShouldBlockScroll}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't want to be doing this anymore, it locks us into emotion as an intrinsic part of our components API, while this is true of our own implementations, this should ultimately not be true for power users.

{({ css }) => (
<div
ref={innerRef}
className={cx(css(getStyles('control', props)), {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we're using the ClassNames fn from emotion 10 to access the css method so we can pass the processed style objects from css(getStyles()) to our custom cx implementation.

@mitchellhamilton I just wanted to sanity check the fact that we're using a custom cx implementation her instead of the one provided to us by ClassNames is ok.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using your own cx function is totally fine, it just means that emotion styles won't be merged together which in this case, I don't believe it matters. I'm not really sure about this whole approach though, ClassNames is intended to be a workaround when a component doesn't accept a className prop, I'd recommend an approach like this:

diff --git a/src/components/Control.js b/src/components/Control.js
index a964547..c38a9ad 100644
--- a/src/components/Control.js
+++ b/src/components/Control.js
@@ -1,6 +1,7 @@
 // @flow
-import React, { type Node, type ElementRef } from 'react';
-import { ClassNames } from '@emotion/core';
+import { type Node, type ElementRef } from 'react';
+/** @jsx jsx */
+import { jsx } from '@emotion/core';

 import type { CommonProps, PropsWithStyles } from '../types';

@@ -56,22 +57,19 @@ export const css = ({
 const Control = (props: ControlProps) => {
   const { children, cx, getStyles, className, isDisabled, isFocused, innerRef, innerProps, menuIsOpen } = props;
   return (
-    <ClassNames>
-      {({ css }) => (
-        <div
-          ref={innerRef}
-          className={cx(css(getStyles('control', props)), {
-            'control': true,
-            'control--is-disabled': isDisabled,
-            'control--is-focused': isFocused,
-            'control--menu-is-open': menuIsOpen
-          }, className)}
-          {...innerProps}
-        >
-          {children}
-        </div>
-      )}
-    </ClassNames>
+    <div
+      ref={innerRef}
+      css={getStyles('control', props)}
+      className={cx({
+        'control': true,
+        'control--is-disabled': isDisabled,
+        'control--is-focused': isFocused,
+        'control--menu-is-open': menuIsOpen
+      }, className)}
+      {...innerProps}
+    >
+      {children}
+    </div>
   );
 };

diff --git a/src/utils.js b/src/utils.js
index 0787bfd..d37f037 100644
--- a/src/utils.js
+++ b/src/utils.js
@@ -41,11 +41,10 @@ function applyPrefixToName(prefix, name) {

 export function classNames(
   prefix?: string | null,
-  cssKey?: string | null,
   state?: ClassNamesState,
   className?: string,
 ) {
-  const arr = [cssKey, className];
+  const arr = [className];
   if (state && prefix) {
     for (let key in state) {
       if (state.hasOwnProperty(key) && state[key]) {

viewBox="0 0 20 20"
aria-hidden="true"
focusable="false"
className={css({
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitchellhamilton We technically don't need to be using ClassNames here to access the css method, since we aren't generating a custom className using our own cx implementation, however I noticed that using the css prop with the jsx pragma generated a style tag without a nonce attribute. Not sure why this is the case, but using the ClassNames renderProp to circumnavigate this for now

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very strange, gonna look into it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to latest of babel-plugin-emotion resolves this issue

})}
/>
type DotProps = { color: string, delay: number, offset: boolean };
const LoadingDot = ({ color, delay, offset }: DotProps) => (
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mitchellhamilton same as with the svg example above, technically not necessary to use here, but noticed that nonces were not being added, and the keyframes`` call above was not working.

Copy link
Collaborator

@jossmac jossmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gwyneplaine gwyneplaine changed the title initial emotion 9 --> emotion 10 emotion 9 --> emotion 10 Jan 7, 2019
@gwyneplaine
Copy link
Collaborator Author

@JedWatson after talking to @jossmac we agreed that this is a breaking change, and should be a 3.0.0.

@williamtstanley
Copy link

williamtstanley commented Jan 18, 2019

With this update how would I style custom Options? I used to get emotion as a prop but nolonger... I also don't seem to be getting css as a replacement any longer...

Sorry I just realized that css can come from my import of {css} from emotion...

@Faradey27
Copy link

@gwyneplaine are we plan to merge this?

@gwyneplaine
Copy link
Collaborator Author

@Faradey27 plan is to merge this as a 3.x, we're doing a bit of clean up to release another 2.x.x before this happens, thanks for the patience

@ematipico
Copy link

ematipico commented Mar 15, 2019

This is critically needed.
We can't afford to install install emotion only to support SSR. Thanks! Is there any rough idea on when v3 will be released?

@gwyneplaine
Copy link
Collaborator Author

hey @ematipico, thanks for raising interest in this body of work.
I've been a little swamped over the past few weeks, but am intending to ship 3.0.0 with emotion 10 towards the end of the month. Thanks for your patience :)

@leerob
Copy link

leerob commented Apr 16, 2019

Would love to see this get released! We've had to do a bunch of workarounds to get react-select working with Next.js and this would prevent that.

@jossmac
Copy link
Collaborator

jossmac commented Apr 16, 2019

@gwyneplaine give me a yell if you need a hand with anything. Happy to pair on this.

@piesrtasty
Copy link

Any ETA on when this will be merged in?

@gwyneplaine gwyneplaine changed the base branch from master to v3.0.0 April 26, 2019 04:54
@gwyneplaine gwyneplaine merged commit 8ac29a9 into v3.0.0 Apr 26, 2019
@gwyneplaine gwyneplaine mentioned this pull request Apr 29, 2019
10 tasks
@senorgeno
Copy link

@leerob I am using this with Gatsby and having issues after SSR. Any advice on what workarounds you used to get it to work with Next.js? Looking forward to v3.0.0 👍

@vaunus
Copy link

vaunus commented May 22, 2019

@senorgeno I am just forcing client-side only rendering for now until this is released by wrapping the select with <NoSSR>. I'm importing NoSSR from MaterialUI as I am using that in my project, but I imagine any of the other implementations of it would work as well.

@senorgeno
Copy link

Thanks @vaunus <NoSSR> worked for me in the mean time

@gwyneplaine gwyneplaine mentioned this pull request May 23, 2019
@dcousens dcousens deleted the emotion-10 branch October 19, 2022 05:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants