mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

[RFC] Migrate to styled-components #6115

Closed kybarg closed 3 years ago

kybarg commented 7 years ago

Can we switch to styled-components?

Outdated comparison It has many advantages against JSS Here comparison table, and next version is even going to avoid SSR styles re-render! Features | styled-components | react-jss ------------ | ------------- | ------------- No build requirements | ✅| ✅ Small and lightweight | ✅ | ✅ Supports global CSS | ✅ | ✅ Supports entirety of CSS | ✅ | ✅ Colocated | ✅ | ✅ Isolated | ✅ | ✅ Doesn’t break inline styles | ✅ |✅ Easy to override | ✅ | ✅ Theming | ✅ | ✅ Server side rendering | ✅ | ✅ No wrapper components | ❌ | ✅ ReactNative support | ✅ | ❌ Legend: ✅ = Yes, ❌ = No, 😕 = Kinda, refer to notes or parentheses
oliviertassinari commented 7 years ago

@kybarg Thanks for opening that issue! The CSS-in-JSS is a moving field, choices we made in the past may no longer be valid as problems and solutions changes.

Why not styled-components in the past?

We have been comparing the different styling solution available before selecting JSS.

We did not elect styled-components for the following reasons:

Actually, styled-components wasn't even a thing (existing) when @nathanmarks started working on moving away from inline-style.

The comparison table

Supports global CSS

With https://github.com/cssinjs/jss-global you can write stuff like

const styles = {
  '@global body': {
    color: 'green'
  }
}

Easy to override

How is this not easy? Material-UI side we have a predefined injection order table. On user-land, they can use aphrodisiac that implement great aphrodite override API.

No wrapper components

We don't have any wrapper component on Material-UI side. We could have been using withStyles but we don't for performance reason. We expose that Higher-order Component to users to abstract the context implementation.

Theming

We use jss-theme-reactor internally. JSS is exposing a low-level API that makes it possible.

ReactNative support

That's a good point, react-with-styles API is quite interesting. That's something we could be improving!

The future

Going forward, I think that the most promising path is having an API allowing to switch the style implementation. That would bring us two benefits:

react-toolbox is following that path. My only concern would be with the overhead it's adding. I mean, does it worth it?

I'm adding @kof and @mxstbr to the loop.

oliviertassinari commented 7 years ago

@kybarg Actually, I'm not sure to fully understand what you are suggesting. We are not using react-jss as your question suggest.

When you say we, are you talking about users or Material-UI?

kof commented 7 years ago

My points are:

mxstbr commented 7 years ago

That table is indeed very subjective and based on my own experience. FWIW, styled-components works with any third party component library:

import { Button } from 'material-ui'

const MyButton = styled(Button)`
  // Only these styles will be overridden
  background: red;
`

This works as long as the components attach the className prop internally to some DOM node:

const MaterialUIButton = (props) => {
  /* ... */
  // As long as props.className is attached, the above usage will work
  return <button className={`bla ${props.className}`} />
}

What is react-native support anyways? Isn't it just a subset of the web platform?

Nope, it's not quite that easy 😉 Adding support to JSS shouldn't be hard though, as all you have to do is pass the style object through to StyleSheet.create(). This requires a bit more effort from the styled-components side to make CSS strings work.


I've been talking to @javivelasco for a while now and love where he's going with react-toolbox. His implementation is amazing, and I'd love to see all third party component libraries adopt it. Pinging him so he can chime in with his ideas here!


No server-side rendering concurrency possible. It's relying on a singleton to collect the styles while JSS instantiate a new instance to collect styles at each request. Steaming is really limited.

Totally unrelated, mind commenting in this issue with your ideas for an API that would allow this to be the case? We haven't decided what we're going to do, so your input would very much be appreciated.

rainice commented 7 years ago

Hi, I inquired about this in gitter. Just to get the views of others, I will post it here as well:

I know material-ui next is heavily invested in a custom jss solution. Does anyone discovered any serious advantage of using jss over styled-components?

While jss is good as it enables several patterns like decorators (injectstyles) and plugins, I think styled-components straightforward approach is much cleaner as there is no need for decorators, custom setup and plugins cause it doesn't need to.

In styled-comp every component is already styled so no need to pass styles. and you pass props that can evaluated to produce a different style no setup (createJss) no plugins (prefix) no JSON DSL

kof commented 7 years ago

Somebody has to lock this thread.

@rainice jss doesn't have decorators, a HOC is an implementation detail of react-jss and is not used here.

javivelasco commented 7 years ago

Why should this be locked? It's a sane discussion IMO

kof commented 7 years ago

Because end user based content here (not lib maintainers) is highly superficial and this is understandable because they didn't read a single line of code behind of what they use.

mbrookes commented 7 years ago

Since the choice of styling solution has been discussed at length, the rational behind the decision documented, and development work towards the next major release is ongoing, this is not a useful issue, so I am going to close it.

Hopefully people are mature enough to not continue posting to a closed thread, but we can lock it if the need arises.

oliviertassinari commented 7 years ago

I wish we could have moved forward that thread with a less coupled styling solution! However, I think that our priority, for now, should be around finishing the migration/overall improvement of the components.

followbl commented 6 years ago

@mxstbr thanks for styled-components

This works as long as the components attach the className prop internally to some DOM node

this might be worth highlighting somewhere in your usage guide when mui:next is released. This comment just saved me.

yhaiovyi commented 6 years ago

Flex styles for IE10 don't work with jss but work with styled-components like a charm

oliviertassinari commented 6 years ago

@yhaiovyi Material-UI doesn't support IE10.

kof commented 6 years ago

Vendor prefixer evtl. Will be fixed soon for jss, doesn't mean though it will fix all issues if mui was never tested on IE10

yhaiovyi commented 6 years ago

Anyway I haven't found any other problems then css flex with IE10 so far

Danilo-Araujo-Silva commented 6 years ago

Looks like we have 3 ways (could be easier, but not everything is flowers) to override Material UI styles with Styled Components. Here is my Gist.

oliviertassinari commented 6 years ago

You can also get a styled-components API like with few lines of code: https://material-ui.com/customization/css-in-js/#styled-components-api-15-lines-

caub commented 6 years ago

You can also use styled-jss as well, example: https://codesandbox.io/s/32mvjyjyxq

the only downside with JSS in general is the lack of autocompletion in code editors, like said here too, but the benefits are there, parsing css to js like in styled-components is a bit an overload

edit: just noticed the referenced issue just above, interesting

caub commented 6 years ago

What's annoying is Mui's context and withStyles HOC don't seem to play well with the core react-jss and styled-jss ThemeProvider https://codesandbox.io/s/32mvjyjyxq (I tried putting a Typography but that doesn't work, edit: nvm, still fiddling with it)

I wonder if later (post v1 I guess) it wouldn't be worth to simplify src/styles/withStyles and MuiThemeProvider + JSSProvider double layer, and have something a bit more simple like how react-jss and styled-jss have

kof commented 6 years ago

Totally for it!

On Mar 13, 2018 13:55, "Cyril Auburtin" notifications@github.com wrote:

What's annoying is Mui's context and withStyles HOC don't seem to play well with the core react-jss and styled-jss ThemeProvider https://codesandbox.io/s/32mvjyjyxq

I wonder if later (post v1 I guess) it wouldn't be worth to simplify src/styles/withStyles and MuiThemeProvider + JSSProvider double layer

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mui-org/material-ui/issues/6115#issuecomment-372655385, or mute the thread https://github.com/notifications/unsubscribe-auth/AADOWAbwLOnRoypx9ANCZnKyalZyD0M9ks5td8HNgaJpZM4L-GwD .

mxstbr commented 6 years ago

parsing css to js like in styled-components is a bit an overload

Just like parsing objects to CSS is :wink: Parsing strings is about the same speed, it honestly doesn't matter. https://github.com/A-gambit/CSS-IN-JS-Benchmarks/blob/master/RESULT.md

Solution Use CSS Use Inline-Styles Mount Time (ms) Rerender time (ms)

... styled-components | + | - | 182 | 146.84 styled-components-decouple-cell | + | - | 213.53 | 152.39 ... react-jss | + | - | 198.97 | 297.74

kof commented 6 years ago

@mxstbr a full css parser written in js at runtime has definitely a price. That benchmark is not measuring its cost.

mxstbr commented 6 years ago

a full css parser written in js at runtime has definitely a price.

Sure, but not more than a full CSS parser that handles objects rather than strings. On top of that CSS parsers that operate on actual CSS strings have been optimized and explored for a long time, much less so with ones that handle objects. :blush:

I'd be curious about benchmarking bootstraps CSS as an object with your parser vs bootstraps CSS with stylis, our parser, but I'm pretty certain the difference will be negligible at best.

kof commented 6 years ago

I'd be curious about benchmarking bootstraps CSS as an object with your parser vs bootstraps CSS with stylis, our parser, but I'm pretty certain the difference will be negligible at best.

Yes that would be an appropriate benchmark, I have tested a bit, working with objects is a lot faster, like 10-20x faster.

kof commented 6 years ago

But then again, it depends on which jss plugins you would include, we have a lot of syntactic sugar plugins.

kof commented 6 years ago

Also tbh. it doesn't matter if we all move to ISTF.

mxstbr commented 6 years ago

I have tested a bit, working with objects is a lot faster, like 10-20x faster.

Sure but 10ms (that's how long stylis takes to parse the entire bootstrap stylesheet) vs 1ms of parsing the entire CSS of an application won't matter in the grand scheme of things, you know? It won't make or break anyones app.

Anyways, let's stop annoying people in this issue with more notifications than necessary.

kof commented 6 years ago

Btw. this benchmark seems to be more accurate: http://necolas.github.io/react-native-web/benchmarks/ I am not sure though it is not relying on a cache after first parse.

mbrookes commented 6 years ago

@mxstbr While this issue is now locked, a bit of healthy competition is good for everyone. Come back any time - you can find us in gitter chat if an issue isn't the appropriate venue for discussion.

oliviertassinari commented 6 years ago

This issue is more than one year old. Guys, please stop commenting on it. We can move the discussion to a new one. A lot has changed since the discussion started.

But let's try to avoid locking issues. We need to keep gather as much feedback as we can possible to make better decisions.

kof commented 6 years ago

I think locking issue as a tool to clearly communicate this is fine, no one was offended by this nor freedom of speech was taken. In this case it is really fine.

oliviertassinari commented 5 years ago

I'm reopening to collect more feedback, I'm curious if this is something people are still interested in:

Capture d’écran 2019-03-10 à 10 38 56

We have a running developer survey we gonna use the results to update our ROADMAP. We have the following requirements with the styles solution:

  1. Bundle size. We want people to be able to use a single component without having to pay 20 KB gzipped for it. By ranking:
    1. emotion is the best candidate in this direction: 10kB gzipped. However, it's used by few people. If you look at the download stats, 80% comes from storybook?
    2. styled-components weight about 16 KB gzipped. His advantage comes from the number of people that uses it. Maybe 20% of the React users?
    3. JSS with a react wrapper weight about 15 KB gzipped. If you look at the download stats, a lot of the usage comes from Material-UI.
  2. Performance. We want people ot be able to customize our components. They should be as fast as possible. The benchmark I could do yields the following ranking:
    1. emotion
    2. jss
    3. styled-components
  3. Customization API. Customizing a variant should be easy, customizing a nested element should be easy, adding a new variant should be easy. Right now, we have a classes API, a theme.overrides API and the capability to have global deterministic class names.
  4. Interoperability. Using !important is not a solution.
  5. RTL support

I have only mentioned styled-components, emotion, and JSS but they are different alternatives: linaria, SASS, etc.

o-alexandrov commented 5 years ago

I believe it might be worth adding the following two issues as they might help to affect the decision when you would have time to work on CSS-in-JS library that is used by Material-UI:

oliviertassinari commented 5 years ago

@o-alexandrov Is JSS following the same strategy than styled-components? I don't understand your point. Could you clarify? What's the difference? How is it better or worth?

o-alexandrov commented 5 years ago

@oliviertassinari In the previous message, I say nothing about styled-components, except that I welcome the idea to evaluate usage of styled-components or any other CSS-in-JS library that would have the perfect balance of:

The links to the two issues above only discuss the current negative aspect of working with JSS. I forgot where I saw the following, but I remember you made a good point that to be able to make decisions, we should first add all types of benchmarks so the decisions could be evaluated easier.

Personally, I like:

oliviertassinari commented 5 years ago

@o-alexandrov Ok, in this case, I'm marking your first comment as off-topic. In v5, we want to isolate the core components from the styling solution. I wish we can provide a naked, JSS, emotion, linaria and styled-components (as the default) versions of the components. That's the vision. Now the implementation is going to be challenging!

yordis commented 4 years ago

Bundle size

styled-components had proven to be improving 12.3kB in @5.0.0-beta.8

Customization API

If they introduce https://github.com/styled-components/styled-components/pull/2625 either in package or external package I believe the API parity to Mui since it is lacking non-styled-components API that sometime we may need, especially in legacy systems but I don't think you would need that API in Mui itself that often.

Outside that,

I am not sure what info you would like to gather since from my personal experience styled-components offers the features that we need.

Interoperability.

What is this for you in this context? You can override things in styled-components as any other framework as far as I know.

RTL support

Isn't this based on how we code the components? What exactly does styled-components or any CSS-in-JS library have to offer for this?

My thoughts

Speed

It used to be slower compared with other solutions but the support and the amazing effort from contributors made it to the point where is becoming even faster than JSS.

Size

v5 is becoming even leaner like I said 12.3kB from 16.2kB which represent their effort on this topic.

Familiarity

People are familiar with the API since it is the most popular, in fact, most people call styled-components referring to the API rather than the actual package when we used styled(...) ... API, for the most part.

React Native

It does weight that much for Mui is the web, but people will continue adopting styled-components because of it.

Core Team

Strong Core team, as of today 81 issues and 14 PRs open, these are good numbers thou for the number of people that uses the package.

Also, people like @mxstbr actually use the package in spectrum so he has real-world experience using the package, that is amazing, that means he actually knows how it feels to use the package.

Tookit

Well, couldn't be better https://www.styled-components.com/docs/tooling

For UI authors

As of today, the adoption of styled-components for Design System components has been increasing a lot; Atlassian, GitHub, Orbit and many others.

This is good for Mui since you will no be alone so probably people already deal with potential situations that you may encounter and they figured out how to deal with it.

TL;DR

I support styled-components.

caub commented 4 years ago

I like JSS because the object syntax for CSS has become easier for me, sometimes I'm lazy and I even just pass those styles as style={{styles.dialogTitle}} inline style, it's easy to refactor later

And it can be used in different ways, with an element wrapper like styled-components https://material-ui.com/styles/basics/#styled-components-api

yordis commented 4 years ago

@caub https://www.styled-components.com/docs/advanced#style-objects

mbrowne commented 4 years ago

I really like styled-components, but I found recently that it introduces a number of issues that make it difficult to make tree-shaking work consistently. I know the material-ui team just did a lot of work to get tree-shaking fully working for this library, and obviously any regression on that should be avoided.

Fortunately most of its tree-shaking issues are resolved by using babel-plugin-styled-components and setting pure: true (see https://www.styled-components.com/docs/tooling#dead-code-elimination). But there are still some remaining issues. For example: https://github.com/styled-components/babel-plugin-styled-components/issues/245

Another is that using an external helper function inside your styles can break tree-shaking (unless you configure terser/uglify to ignore that specific function), e.g.:

const Button = styled.button`
  font-size: ${externalHelperFunction()};
`

I think it should be possible to fix all of these issues to get tree-shaking to work correctly, but it can certainly be tricky and doesn't just work in an ideal way out of the box as things currently stand. I actually still think that switching to styled-components might be a good idea, but only if these issues can be worked out.

eps1lon commented 4 years ago

@mbrowne I could not reproduce any issue with tree-shaking and styled components. The issue does not include a reproducible example so I tried to replicate it with

// components.js
import React from "react";
import styled from "styled-components/macro";

const Wrapper = styled.div`
  color: blue;
`;

export function MyComponent() {
  return <Wrapper>styled</Wrapper>;
}

MyComponent.displayName = "FancyName";

export function OtherComponent() {
  return "only";
}

// App.js
import React from 'react';
import { OtherComponent } from "./components";

/* code */

with a default create-react-app. MyComponent will not appear in the production bundle.

Is this something only rollup has issues with? I would appreciate a reproducible example to understand the issue.

mxstbr commented 4 years ago

@eps1lon I think the issue appears when setting the static property on the styled component, can you try that?

const Wrapper = styled.div`
  color: blue;
`;

Wrapper.displayName = "FancyName";

export function MyComponent() {
  return <Wrapper>styled</Wrapper>;
}

export function OtherComponent() {
  return "only";
}
eps1lon commented 4 years ago

@mxstbr Yeah though in both cases styled-components is bundled. While setting the displayName on MyComponent did not include MyComponent in the bundle it does still include styled-components. It basically removes anything done to MyComponent which is why I originally thought it properly tree-shakes (just searched for FancyName.

But it still includes styled-components. Even if you consider a styled call to have side-effects I would consider

import React from "react";
import styled from "styled-components";

export function Wrapper() {
  // nonsense
  return styled.div``;
}

export function MyComponent() {
  return <Wrapper>styled</Wrapper>;
}

export function OtherComponent() {
  return "only";
}

as side-effect free when importing { OtherComponent } but styled-components will still appear in the bundle (this is even without the macro). So either this is some bug or I'm missing something big. Even

// Wrapper.js
import React from "react";
import styled from "styled-components";

export default function Wrapper() {
  // side-effect free module even if styled has side-effects
  const Component = styled.div``;
  return <Component />;
}

// components.js
// commenting this out removes styled-components from the bundle
export { default as Wrapper } from "./Wrapper";

export function OtherComponent() {
  return "only";
}

// index.js
import React from 'react';
import ReactDOM from 'react-dom';
import { OtherComponent } from "./components";
import * as serviceWorker from './serviceWorker';

ReactDOM.render(<OtherComponent />, document.getElementById('root'));

// If you want your app to work offline and load faster, you can change
// unregister() to register() below. Note this comes with some pitfalls.
// Learn more about service workers: https://bit.ly/CRA-PWA
serviceWorker.unregister();

will include styled-components (https://github.com/eps1lon/styled-components-shake).

Might be a bug in react-scripts, webpack or styled-components. In any case debugging these issues is incredibly hard without a repro.

mxstbr commented 4 years ago

Makes sense, thank you for investigating that.

For this use case I don't think it matters whether styled-components is included in the bundle (since all components should use it), but rather whether the components you don't use are included—which it doesn't sound like it's the case, so that's good, right?

oliviertassinari commented 4 years ago

@mxstbr It could be an important concern, we have a couple of components that are unstyled, and generic (Modal, Popper, TextareaAutosize, useMediaQueries, etc.), let's say somebody uses

import { Modal } from '@material-ui/core';

with SASS. We would expect a +5kB gzipped increase (as of today), not +20kB gzipped.

However, assuming we push @material-ui/unstyled forward, in practice, it might not make any difference as people could use this package.

mbrowne commented 4 years ago

@eps1lon Sorry, I thought the issue with static properties would be easier for others to reproduce since it seemed to be affecting all of our components...it turns out there are a lot of different things that trigger the issue, but at least in some simple cases it works.

I created a reproducible demo here: https://github.com/mbrowne/CRA-error-template/tree/styled-components-tree-shaking git clone --single-branch --branch styled-components-tree-shaking git@github.com:mbrowne/CRA-error-template.git

Note how only Component1 tree-shakes correctly.

The good news is that I think these issues are all solvable, and @mxstbr seems very engaged here. I will be working on a PR soon that will address at least the static props issue (I already have a working POC using a separate Babel plugin I wrote).

mbrowne commented 4 years ago

I opened a PR into babel-plugin-styled-components to address the tree-shaking issues. If anyone here wants to help test it, I imagine @mxstbr would welcome that (and I would too of course): https://github.com/styled-components/babel-plugin-styled-components/pull/248

South-Paw commented 4 years ago

Hi all, where is this ticket at currently? I'd be more than happy to get involved and write some styled components for MUI if that is indeed the direction the project is heading in v5

eps1lon commented 4 years ago

I guess unless we want to do this in one go (I doubt we want that). We should either start by making styled-components read the theme from jss or jss read the theme from styled components. The goal should be that we can migrate styles on a per component basis.

This should probably happen on another branch. While we don't want to change everything on master in one go we should probably release it with one change (in v5). Otherwise this gets even more confusing.

eps1lon commented 4 years ago

Comment deletion was requested.