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

[Sugestion] Change style engine to styled-components #6115

Closed
kybarg opened this Issue Feb 11, 2017 · 31 comments

Comments

@kybarg
Copy link
Contributor

kybarg commented Feb 11, 2017

Can we switch to styled-components?
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

This comment has been minimized.

Copy link
Member

oliviertassinari commented Feb 11, 2017

@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:

  • Cost of the abstraction. css-in-js-perf-tests is a good resource, styled-components use glamor internally.
  • Exposing the className and the CSS deep selector power allow performant implementation. For instance with the <Grid />: #6010.
  • 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.

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:

  • Material-UI is less coupled to any styling solution
  • Users using styled-components/aphrodite/... on their side can save the JSS overhead used internally.

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

This comment has been minimized.

Copy link
Member

oliviertassinari commented Feb 11, 2017

@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

This comment has been minimized.

Copy link
Member

kof commented Feb 11, 2017

My points are:

  • styled-components is a much higher level library than the JSS core, you can for sure use it in your application layer.
  • The comparison table above is about react-jss, not JSS core and is a subjective opinion of Max. It is partially not true and partially just a look from some specific perspective which we don't see from the table.
  • We are working on dynamic rules rendering for more efficient dynamic styling, jss-theme-reactor does the job right now already, its just a matter of an optimization, which is probably not very relevant for MUI.
  • What MUI uses internally should be totally out of concern for the end user. Everything a user needs to do should be possible without even knowing what MUI is internally using or at least its more or less regular cssinjs syntax for theming.
  • We need to get use cases MUI doesn't support right now and solve them. I am always happy to help the integration process and available on gitter.
  • What is react-native support anyways? Isn't it just a subset of the web platform? If so it should just work, otherwise let me know what JSS needs to be able to do to support react-native, here is the issue.
@mxstbr

This comment has been minimized.

Copy link

mxstbr commented Feb 11, 2017

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.

@kybarg kybarg closed this Feb 17, 2017

@rainice

This comment has been minimized.

Copy link

rainice commented Mar 17, 2017

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

This comment has been minimized.

Copy link
Member

kof commented Mar 17, 2017

Somebody has to lock this thread.

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

@javivelasco

This comment has been minimized.

Copy link

javivelasco commented Mar 17, 2017

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

@kof

This comment has been minimized.

Copy link
Member

kof commented Mar 17, 2017

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

This comment has been minimized.

Copy link
Member

mbrookes commented Mar 17, 2017

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.

@mbrookes mbrookes closed this Mar 17, 2017

@oliviertassinari

This comment has been minimized.

Copy link
Member

oliviertassinari commented Mar 17, 2017

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.

@oliviertassinari oliviertassinari changed the title [Sugestion] Change style engine [Sugestion] Change style engine to styled-components May 9, 2017

@followbl

This comment has been minimized.

Copy link

followbl commented May 28, 2017

@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

This comment has been minimized.

Copy link

yhaiovyi commented Nov 9, 2017

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

@oliviertassinari

This comment has been minimized.

Copy link
Member

oliviertassinari commented Nov 9, 2017

@yhaiovyi Material-UI doesn't support IE10.

@kof

This comment has been minimized.

Copy link
Member

kof commented Nov 9, 2017

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

This comment has been minimized.

Copy link

yhaiovyi commented Nov 10, 2017

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

@Danilo-Araujo-Silva

This comment has been minimized.

Copy link

Danilo-Araujo-Silva commented Jan 10, 2018

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

This comment has been minimized.

Copy link
Member

oliviertassinari commented Jan 11, 2018

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

@caub

This comment has been minimized.

Copy link
Contributor

caub commented Mar 11, 2018

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

This comment has been minimized.

Copy link
Contributor

caub commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Member

kof commented Mar 13, 2018

@mxstbr

This comment has been minimized.

Copy link

mxstbr commented Mar 13, 2018

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

Just like parsing objects to CSS is 😉 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

This comment has been minimized.

Copy link
Member

kof commented Mar 13, 2018

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

@mxstbr

This comment has been minimized.

Copy link

mxstbr commented Mar 13, 2018

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. 😊

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

This comment has been minimized.

Copy link
Member

kof commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Member

kof commented Mar 13, 2018

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

@kof

This comment has been minimized.

Copy link
Member

kof commented Mar 13, 2018

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

@mxstbr

This comment has been minimized.

Copy link

mxstbr commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Member

kof commented Mar 13, 2018

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.

@mui-org mui-org locked as resolved and limited conversation to collaborators Mar 13, 2018

@mbrookes

This comment has been minimized.

Copy link
Member

mbrookes commented Mar 13, 2018

@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.

@mui-org mui-org unlocked this conversation Mar 13, 2018

@oliviertassinari

This comment has been minimized.

Copy link
Member

oliviertassinari commented Mar 13, 2018

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

This comment has been minimized.

Copy link
Member

kof commented Mar 13, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment