emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.5k stars 1.11k forks source link

Drastic API change in 10 #961

Closed jfrolich closed 4 years ago

jfrolich commented 6 years ago

I just spend a day trying to update to emotion 10 because emotion 9 uses deprecated lifecycle methods.

It is not trivial to port. Some decisions are impacting the developer experience of the lib (less nice than emotion 9) and it's actually a lot of work to convert an existing project. Are these changes made just for performance? Is it a worthy trade-off?

Wouldn’t it be better to have better backwards compatibility? This feels like a whole new lib with worse developer experience. (I loved the string css prop).

Supporting the old api, with a Babel plugin this shouldn’t be a big problem I think, and would enable a much easier transition.

However if there is a steep breaking new version, it would be really great to support ConcurrentMode in react, and remove deprecated lifecycle methods from emotion 9!

emmatown commented 6 years ago

The documentation isn't totally ready for v10, we are going to keep on supporting the emotion package. If you do what's mentioned here and keep using the emotion package on the next tag on npm, you can keep on using the apis from there and compose them with styled and etc. and it'll all just work (the zero-config SSR stuff won't work though, you have to use the SSR APIs that are currently available)

deprecate the Babel plugin it’s nice not to add the jsx pragma for every file

We might add a Babel preset to do this. You can also use eslint-plugin-emotion and use the jsx-import rule to import it automatically.

deprecating string css props

We might keep supporting it but it gets very confusing because to maintain the current behavior, there would have to be a babel transformation and we're trying to make it so that there is only a very small number of features that require the babel plugin.

removing cx

The idea is that instead of having cx to compose styles, you can do it by passing the className that you want to be composed to an element and pass styles via the css prop.

component composition story? if you pass css to a component it receives className, how to compose without cx? or do we need to use the ClassNames component with render-prop for each invocation?

Pass the className to an element and use the css prop on the element.

jfrolich commented 6 years ago

The documentation isn't totally ready for v10, we are going to keep on supporting the emotion package. If you do what's mentioned here and keep using the emotion package on the next tag on npm, you can keep on using the apis from there and compose them with styled and etc. and it'll all just work (the zero-config SSR stuff won't work though, you have to use the SSR APIs that are currently available)

This sounds good, however if I just add the cache provider and upgrade emotion, react-emotion, and react-theming to next, it will fail with:

" 'react-emotion' does not contain an export named 'css'.

So it's not entirely backwards compatible.

jfrolich commented 6 years ago

Pass the className to an element and use the css prop on the element.

Is this the right approach?

const Comp = ({className, otherClassName}) => 
  <div 
    css={[
      css`background-color: blue;`,
      className, 
      otherClassName
    ]} 
  />

const Comp2 = () => 
  <Comp 
    css={css`color: red`} 
    otherClassName={css`background-color: green`} 
  />

BTW it would be great if you just pass a string, this will be added as a className.

emmatown commented 6 years ago

This sounds good, however if I just add the cache provider and upgrade emotion, react-emotion, and react-theming to next, it will fail with:

It's not meant to be entirely backwards compatible, it's just meant to only require codemoddable changes.

Is this the right approach?

What is otherClassName for, if className and otherClassName are intended to apply to the same element.

Without otherClassName, it should be like this

const Comp = ({ className, otherClassName }) => (
  <div
    className={className}
    css={css`
      background-color: blue;
    `}
  />
);

const Comp2 = () => (
  <Comp
    css={css`
      color: red;
      background-color: green;
    `}
  />
);

BTW it would be great if you just pass a string, this will be added as a className.

We can't do that because we don't know what is a class name and what is css.

jfrolich commented 6 years ago

It's not meant to be entirely backwards compatible, it's just meant to only require codemoddable changes.

Ah ok, but now I have an issue in the components I use the css prop (needs to be @emotion/core I think) and css from the emotion export. They are different functions right?

What is otherClassName for, if className and otherClassName are intended to apply to the same element.

I know this example doesn't make sense, but interested how to compose props that contain different outputs of css``.

Without otherClassName, it should be like this

What is the precedence between className and css, what if I would like to reverse the precedence?

jfrolich commented 6 years ago

I think the previous model was way easier to understand. css would generate a className, cx would glue them together, and they are composable, even mixing normal classNames was possible with cx.

Now we also have a style entity that is not directly usable as a className. So it's less easy to mix with other libraries (and building libraries with emotion is now harder).

Is the added complexity purely for performance. Is there not a way to keep the same old API, but do the optimizations using a compile time optimization?

emmatown commented 6 years ago

Ah ok, but now I have an issue in the components I use the css prop (needs to be @emotion/core I think) and css from the emotion export. They are different functions right?

If you're passing the cache from emotion to the CacheProvider then you can use the css function from emotion with the css prop.

I know this example doesn't make sense, but interested how to compose props that contain different outputs of css``.

It's hard for me to give help without having real world examples.

What is the precedence between className and css, what if I would like to reverse the precedence?

The className has a higher precendence than css, see #753 for details about it.

Now we also have a style entity that is not directly usable as a className. So it's less easy to mix with other libraries

You can use the css prop to pass a className to any other component so I don't see how it makes it less easy to mix with other libraries.

(and building libraries with emotion is now harder).

Could you elaborate how not being able to get classNames directly makes building libraries with emotion harder?

Is the added complexity purely for performance. Is there not a way to keep the same old API, but do the optimizations using a compile time optimization?

It's to enable out of the box server rendering, working more easily with iframes, and being able to provide various other options via context.

jfrolich commented 6 years ago

Thanks for answering, I might have to wrap my head around this a bit more. I would really appreciate if you could have a look at this bug: #960, this is blocking me from converting a project to emotion 10. Tried to isolate everything!

jfrolich commented 6 years ago

Could you elaborate how not being able to get classNames directly makes building libraries with emotion harder?

Passing multiple class names to a component, like, containerClassName.

emmatown commented 6 years ago

I've fixed #960 with #964

jfrolich commented 6 years ago

Any thoughts on updating 9 to support ConcurrentMode. For the coming time 10 is really unstable, and completely breaks the API's, converting to 10 is like adopting a new CSS in JS library (fine, but I would suggest to change the name). This will also help libraries that adopted emotion to support modern react (react-select for instance).

emmatown commented 6 years ago

For the coming time 10 is really unstable, and completely breaks the API's, converting to 10 is like adopting a new CSS in JS library

Like I've said before, if you want to use the old APIs, you still can in v10. It just won't give you the advantages that the new APIs bring. Every change that is breaking is codemoddable or only requires a single change per app.

This will also help libraries that adopted emotion to support modern react (react-select for instance).

That point is irrelevant for react-select, react-select only uses the emotion package which works totally fine with concurrent mode.

jfrolich commented 6 years ago

Ok, was a bit frustrated, migrating the app when posting the comment... But got it in a state that most of the styles are working. Still not a smooth upgrade path, but sorry for the negative comment.

Btw I saw in the React element tree that each (styled) component now has two context parents. Perhaps a performance issue? Perhaps the non-render prop perhaps the new ContextType or hooks will be less heavy?

emmatown commented 6 years ago

Still not a smooth upgrade path

That sucks. Could you talk about what the problems were? Did you use eslint-plugin-emotion with the codemod rules(though you should probably exclude the no-vanilla rule)? I want to make the migration path really smooth so I'm very interested to hear feedback on migration. Also, I want to be totally clear, if you want to keep using the vanilla APIs that's totally fine and it's what we want people to do, especially during migration. We don't want people to have to migrate a huge app all in one go. The intended path is: install the new packages -> run the codemods -> migrate other things that can't be codemodded over time(NOT all in one go) or never migrate those things.

Btw I saw in the React element tree that each (styled) component now has two context parents. Perhaps a performance issue? Perhaps the non-render prop perhaps the new ContextType or hooks will be less heavy?

I've got a hooks implementation in #967 and I expected it to be a sizable performance improvement but it only had a very small performance improvement so, it's not really a performance problem.

jfrolich commented 6 years ago

Still not a smooth upgrade path

That sucks. Could you talk about what the problems were? Did you use eslint-plugin-emotion with the codemod rules(though you should probably exclude the no-vanilla rule)? I want to make the migration path really smooth so I'm very interested to hear feedback on migration. Also, I want to be totally clear, if you want to keep using the vanilla APIs that's totally fine and it's what we want people to do, especially during migration. We don't want people to have to migrate a huge app all in one go. The intended path is: install the new packages -> run the codemods -> migrate other things that can't be codemodded over time(NOT all in one go) or never migrate those things.

I used the css prop a lot: css={`...`} in conjunction with styled components. Needed to update all the css props and add the jsx pragma, and replace imports -> @emotion/styled (it doesn't have an import for css, keyframes anymore, and I imported all non styled imports from react-emotion). I did use the codemod, but I had to spend about a day to fix cases that weren't covered, it was ~ 300 instances that couldn't be solved by simple regex find replaces (forgot exactly which ones sorry!). We also use css3 animations quite a bit and they are all broken (not sure why, still have to fix those).

A number of pending issues are with assuming the css`` produces a className. Most of them can be fixed by passing to css instead of className.

It would be the best upgrade path if everything just works with the packages that are not @emotion/x so there would be zero migration (while injecting the cache). It would also be great to be able to keep passing css=`` to components when using the babel-plugin. Even better if it could inject the pragma (or just use a different createElement for the elements that have a css prop). I still really like the developer experience of that, you don't have to think about styling anymore and it just works. The babel plugin perhaps can codemod legacy stuff at zero performance penalty?

ChristopherBiscardi commented 6 years ago

and add the jsx pragma

you can adjust your config in babel and webpack to use the jsx pragma everywhere as described in these glamor docs

.babelrc

{
  ...
  "plugins": [
    [
      "transform-react-jsx",
      { "pragma": "Emotion.jsx" }
    ]
  ]
}

webpack

plugins: [
  new webpack.ProvidePlugin({
    Emotion: '@emotion/core'
  })
]

This is what I'm doing for application-level code. I'm taking the extra time to add the jsx pragma to library packaged code though.

jfrolich commented 5 years ago

In line of the css prop being introduced in styled components (https://www.styled-components.com/docs/api#css-prop). It would be great to include allowing string css attributes when using the babel plugin. This way we have API parity between styled components and emotion. With emotion's implementation much more elegant of course.

wKovacs64 commented 5 years ago

I've upgraded 2 projects so far and the eslint rules error as expected but --fix is not available (does nothing). That's what you mean by codemod, right? Should be --fixable? Is there some trick to get that to work?

Edit: It worked in a Gatsby project, however. 🤷‍♂️

eliassotodo commented 5 years ago

The idea is that instead of having cx to compose styles, you can do it by passing the className that you want to be composed to an element and pass styles via the css prop.

Somehow that wasn't clear to me from the documentation. I am glad that I found this thread. Did I miss something or shall we do something to make this clear in the docs? At least in the migration guide...

Jayphen commented 5 years ago

I'm in the process of upgrading about 150 React apps to Emotion 10.

The codemod gets us part of the way, but not all of the way. There are a couple of heavily used patterns that are not codemoddable via eslint.

const styles = css`
  color: red;
`

<SomeComponent className={cx(styles, 'something-else', condition && 'another-thing')} />

This becomes:

const styles = css`
  color: red;
`

<SomeComponent css={styles} className=`'somethng-else ${condition ? 'another-thing' : ''` />

I'm not happy with the conditional className string there either — we may have to introduce another library like classnames to better that. This the above implementation you end up with blank spaces in the class name. Not a big deal, but not nice.

This is another approach (and this is not something I want to introduce to any of my codebases…)

const styles = css`
  color: red;
`

<ClassNames>
  {({cx}) =>
    <SomeComponent css={styles} className={cx('something else', condition && 'another-thing')} />
  }
</ClassNames>

This pattern is also a problem:

<SomeComponent className={css`color: red`} />

Thankfully it's not as heavily used, and can be fixed with find-and-replace, unless the className contains more than just a single css tagged template literal.

Are there any methods for easing the upgrade path here, or is it just going to have to be manual?

jackyef commented 4 years ago

Hi! We are a big fan of emotion and we also have problems migrating our react app from emotion 9 to 10.

One of the most used pattern in our app is to use generated css as classnames directly like:

const myclass = css`
  background: red;
`;

const Component = () => {
  return <div className={myclass}>Hello world!</div>
}

Using emotion 10 with the babel presets, this does not work anymore. What would the recommended way of migrating be in this case?

tkh44 commented 4 years ago

@jackyef, In your small example you can simply change className to css

const myclass = css`
  background: red;
`;

const Component = () => {
  return <div css={myclass}>Hello world!</div>
}

This is assuming you've already set up the babel preset (https://emotion.sh/docs/css-prop#babel-preset) which includes the babel plugin and the setup for the css prop.

jackyef commented 4 years ago

@tkh44, ah sorry I wasn't being clear. My intention was to find out if there is way to migrate more easily in that case. We have 10000+ components and changing it manually would not be our first preferred way of migrating. Some components also use the patterns @Jayphen mentioned.

jfrolich commented 4 years ago

@tkh44, ah sorry I wasn't being clear. My intention was to find out if there is way to migrate more easily in that case. We have 10000+ components and changing it manually would not be our first preferred way of migrating. Some components also use the patterns @Jayphen mentioned.

With 10k components it probably pays to write a codemod in Babel. These patterns are usually quite straightforward.

jackyef commented 4 years ago

With 10k components it probably pays to write a codemod in Babel. These patterns are usually quite straightforward.

I totally agree! I actually planned to try to write one (if I ultimately had to), but wanted to make sure if the maintainers are already planning to work on something similar or not.

If they are, is the progress tracked in any issue on GitHub?

If they aren't, why? Was it already tried but it was not possible? Or was it simply not high in their priority list?

If it's possible, would it help if we gather some people to work together on this? Since it seems like a lot of people are having problems migrating and it would be really helpful for the community as well. I would not mind to be a part of it.

jfrolich commented 4 years ago

With 10k components it probably pays to write a codemod in Babel. These patterns are usually quite straightforward.

I totally agree! I actually planned to try to write one (if I ultimately had to), but wanted to make sure if the maintainers are already planning to work on something similar or not.

If they are, is the progress tracked in any issue on GitHub?

If they aren't, why? Was it already tried but it was not possible? Or was it simply not high in their priority list?

If it's possible, would it help if we gather some people to work together on this? Since it seems like a lot of people are having problems migrating and it would be really helpful for the community as well. I would not mind to be a part of it.

There is an existing codemod, but it doesn't cover everything. I already migrated a long time ago :)

Andarist commented 4 years ago

You can also try to add a compat cache on top of your app - to ease the migration time: https://emotion.sh/docs/migrating-to-emotion-10#manual-steps

Andarist commented 4 years ago

I'm closing this issue - as at this point in time it got really stale, focused on different aspects of the API etc But what's more important - the current APIs are here to stay, there are no plans to remove them and as @mitchellhamilton has said - you can totally use Emotion 9 APIs when using Emotion 10 (just use emotion rather than @emotion/core), so we don't believe this is a problem that is really that relevant.

We understand that when Emotion 10 got published it could have been a shock for some, but the time has verified in the long run it really didn't bother that much people and Emotion 10 is widely adopted across the globe.

jfrolich commented 4 years ago

Totally agree 🙌. This was opened more than 2 years ago 😄