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

[RFC] v5 styling solution 💅 #22342

Closed
mnajdova opened this issue Aug 24, 2020 · 239 comments
Closed

[RFC] v5 styling solution 💅 #22342

mnajdova opened this issue Aug 24, 2020 · 239 comments
Milestone

Comments

@mnajdova
Copy link
Member

mnajdova commented Aug 24, 2020

This RFC is a proposal for changing the styling solution of Material-UI in v5.

TL:DR; the core team proposes we go with emotion

What's the problem?

  • Maintaining & developing a great styling engine takes a considerable amount of time. We have experienced it first hand. Over the last 12 months, we have preferred to invest time on our core value proposition: the UI components, rather than improve the style engine. Working on it has a high opportunity cost.
  • We have been facing issues with supporting dynamic styles for the components. The performance of our custom dynamic styles implementation (based on props) isn't great (see the performance benchmarks below). This is seriously limiting the quality of the Developer Experience we can provide. It's a blocker for improving our API around customizability or ease of writing styles. For instance, it will unlock: style utils props, color variant, and custom variant.
  • The React community, at large, hasn't voted for using JSS at scale (JSS is great and still used). 3 years ago we bet on the best option available. We have to recognize better options are available now. We can move faster and unlock better DX/UX by building on top of a more popular, existing, styling solution.
  • Many developers use styled-components to override Material-UI's styles. End-users find themselves with two CSS-in-JS libraries in their bundle. Not great. It would be better if we could offer different adapters for different CSS-in-JS libraries. (Potential problems: we may need to re-write the core styles to match the syntax of the engine used 🤷‍♀️)

What are the requirements?

Whatever styling engine we choose to go with we have to consider the following factors:

  • performance: the faster the better but we are willing to trade some performance to improve the DX.
  • bundle size: below our current 14.3 kB gzipped would be great.
  • support concurrent mode: @material-ui/styles has partial support as I'm writing.
  • support SSR
  • simple customization
  • allow dynamic styling
  • good community size
  • theming
  • flat specificity
  • RTL
  • TypeScript

It would be nice if it can support the following:

What are our options?

  • styled-components
  • emotion
  • JSS (currently wrapped in material-ui)
  • styletron
  • Aphrodite
  • fela
  • else?

Comparison

Performance

Here are benchmarks with dynamic styles of several popular libraries (note the Material-UI v4 only use static styles which have good performance):

PR for reference: mnajdova/react-native-web#1

Based on the performance, I think that we should eliminate: JSS (currently wrapped in @material-ui/styles), styletron, and fela. That would leave us with:

  • styled-components
  • emotion
  • Aphrodite
  • JSS
  • react-styletron
  • fela

Dynamic props

Based on the open issues, it seems that Aphrodite doesn't support dynamic props: Khan/aphrodite#141
which in my opinion means that we should drop that one from our options too, leaving us with:

  • styled-components
  • emotion
  • Aphrodite
  • react-styletron

npm

While styled-components and emotion are both libraries are pretty popular, react-styletron at the time or writing is much behind with around 12500 downloads per week (this in my opinion is a strong reason why we should eliminate it, as if we decide to go with it, the community will again need to have two different styling engine in their apps).

Here is the list rang by the number of Weekly downloads at the time of writing:

Note that storybook has a dependency on emotion. It significantly skews the stats.

  • styled-components
  • emotion
  • react-styletron

Support concurrent mode

  • emotion: YES. Since v10 it is strict mode compatible based on their announcement post. I have tested it on a simple project that works as expected.
  • styled-components: Partial. There is at least one bug with global styles in strict mode.

SSR

Stars

  • styled-components: 30.6k
  • emotion: 11.4k
  • JSS: 5.9k

Trafic on the documentation

SimilarWeb estimated sessions/month:

  • sass-lang.com: ~476K/month (for comparison)
  • styled-components.com: ~239K/month
  • emotion.sh: ~59K/month
  • cssinjs.org: <30k/month (for comparison)

Users feedback

Based on the survey, 53.8% percent are using the Material-UI styles (JSS), which is not a surprise as it is the engine coming from Material-UI. However, we can see that 20.4% percent are already using styled-components, which is a big number considering that we don't have direct support for it. Emotion is used by around 1.9% percent of the developers currently based on the survey.

Having these numbers we want to push with better support for styled-components, so this is something we should consider.

Browser support

  • emotion: modern evergreen browsers + IE11
  • styled-components: not documented for v5, but the previous versions support the following

Bundle size

What's the best option?

Default engine

Even if we decide to support multiple engines, we would still need to advocate for one by default and have one documented in the demos.

styled-components

Pros:

  • Has the biggest community, people love to use it.
  • Performance starting from v5 is good.

Cons:

  • It will mean that all components styles need to be created using the styled API, which means for developers they will always have wrapper components if they need to re-style.
  • Lack of full concurrent support, which may create blockers down the road.

emotion

Pros:

  • Relatively large community, growing.
  • Good performance.
  • Concurrent mode + SSR would be possible out of the box.
  • The CSS prop can be useful for overrides.
  • Source map support.
  • A bit smaller.

Cons:

Support multiple

We may try to support multiple CSS-in-JS solutions, by providing our in house adapters for them. Some things that we need to consider is that, that we may have duplicate work on the styles, as the syntax is different between them (at least jss compared to styled-components/emotion). We will reuse the theme object no matter what solution we will pick up.

The less involved support for this may come from the usage of the styled, as people may do some webpack config to decide which one to use - (this is just something to consider).

Additional comments

Deterministic classnames on the components that can be targeted for custom styles

Regarding how the classes look and how developers may target them, I want to show a comparison of what we currently have and how the problem can be solved with the new approach.

As an example, I will take the Slider component. Here is currently how the generated DOM look like:

Each of the classes has a very well descriptive semantic and people can use these classes for overriding the styles of the component.

On the other hand, emotion, styled-components or any other similar library will create some hash as a class name. For us to solve this and offer the developers the same functionality for targeting classes, each of the components will add classes that can be targeted by the developers based on the props.

This would mean that apart from the classes generated by emotion, each component will still have the classes that we had previously, like MuiSlider-root & MuiSlider-colorPrimary, the only difference would be that this classes will now be used purely as selectors, rather than defining the styles for the components. This could be implemented like a hook - useSliderClasses

Conclusion

No matter which solution we would choose, we would use the styled API, which is supported by the two of them. This will allow us down the road to have easier support for styled + unstyled components (probably with webpack aliases, like for using preact).

After we investigated the two options we had in the end, the core team proposes we go with emotion. Some key elements:

A small migration cost between styled-components and emotion

Developers already using styled-components should be able to use emotion with almost no effort.

There are different ways for adding overrides other than wrapper components

The support of cx + css from emotion can be beneficial for developers to use it as an alternative for adding style overrides if they don't want to create wrapper components.

Concurrent mode is for sure supported 👍

Kudos to @ryancogswell for doing a deeper investigation on this topic. So far we did not find anything in @emotion's code that would give us concern that concurrent mode wouldn't work.
We were also looking into createGlobalStyle from styled-components as a comparison to emotion's Global component. It is doing most of its work during render (inherently problematic for Strict/Concurrent Mode) and just using useEffect for removing styles in its cleanup function. createGlobalStyle needs a complete rewrite before it will be usable in concurrent mode -- it isn't OK for it to add styles during render if that render is never committed. It looks like someone has tried rewriting it with some further changes in the last month, so we will need to follow this progress.

How is the specificity handled

Emotion's docs recommend doing composition of CSS into a single class rather than trying to leverage styles from multiple class names. In v5, our existing global class names would be applied without any styles attached to them. The composition of emotion-styled components automatically combines the styles into a single class. This potentially gets rid of these stylesheet order issues at least internal to the styles defined by Material-UI, because every component's styles are driven by a single class name 👍.
So we would have the global class names (for developers to target in various ways for customizations) and then a single generated (by emotion) class name per element that would consolidate all the CSS sources flowing into it.
Specificity is then handled by emotion based on the order of composition.
All compositions using emotion (whether render-time or definition-time composition) results in a single class on the element.
styled-components does NOT work this way concerning render-time composition (definition-time composition does get combined into a single class). The same composition in styled-components results in multiple classes applied to the same element and the specificity does not work as I would have intended.

Alternatives


What do you think about it?

@mnajdova mnajdova added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 24, 2020
@oliviertassinari oliviertassinari added discussion and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 24, 2020
@mbrookes

This comment has been minimized.

@MathiasKandelborg
Copy link
Contributor

MathiasKandelborg commented Aug 24, 2020

Emotion sounds great! I love getting TS support, e.g., autocomplete and all the benefits of typing - with CSS-in-JS - when building UI or styling components, will that still be possible? How is this going to affect that, if at all? TS support is apparently better, amazing!

The last bit got me! I'd love helping by doing this behind a beta flag or develop on some features:

All compositions using emotion (whether render-time or definition-time composition) results in a single class on the element.
Styled-components do NOT work this way concerning render-time composition (definition-time composition does get combined into a single class).

I also noted that the theme object is going to stay the same, the - in my opinion - the absolute best way to theme an application! I have nothing else to say 😲

Thanks for the great work on M-UI. I love the direction the project is going.

Moving to a more standardized way of styling is the way to go. I know the team and community will work out the kinks, and v5 - by the sounds of it - is going to be even more awesome! 🚀

@shilangyu
Copy link

shilangyu commented Aug 24, 2020

As someone who has used both styled-components and emotion I can confirm transitioning between them is easy and painless.

+ emotion is more typescript friendly

@Andarist
Copy link
Contributor

Andarist commented Aug 24, 2020

Speaking as Emotion's maintainer - this sounds great 🚀

We should also be able to release a new major soon-ish which won't be any revolution, just some cleanups, overall improvements, hooks under the hood and TS types improvements (better inference and performance).

Oh, and rewritten parser! that eliminates some edge bugs in Emotion and Styled-Components (as currently, they are both using the same parser). It is both smaller and faster.

@sag1v
Copy link

sag1v commented Aug 24, 2020

What about breaking changes, which option introduce more breaking changes (if any)?

@re-thc
Copy link

re-thc commented Aug 24, 2020

Not sure if it makes a difference but were the benchmarks done with emotion and/or style-component's babel plugins? They help to optimize things further.

@ee0pdt
Copy link
Contributor

ee0pdt commented Aug 24, 2020

It was my understanding that MUI had previously indicated it was going with styled so this is a surprise. I think emotion is a great library, but with more people using styled currently it's important you find ways to give them good options if they don't want to migrate to emotion

@MathiasKandelborg
Copy link
Contributor

MathiasKandelborg commented Aug 24, 2020

@ee0pdt Emotion is very, very much like styled. I think that's part of the whole choice for Emotion over styled-components. There are clear benefits, and migration debt is little to none.

And there's a whole section about supporting both by giving a choice to developers. That could be a way to go, but then again, standardizing would probably help future us more. The full concurrency and no wrapper components is a deal-breaker for me. I get that others might want something styled provides, and that should be considered. I'd rather push toward standardization

@re-thc
Copy link

re-thc commented Aug 24, 2020

Why was styletron-react ruled out? It leaves out a whole metric that wasn't considered, which is memory consumption. The default styletron engine (and fela) is atomic. Whilst a bit slower it saves a lot of memory. Having seen a lot of react pages do nothing and go >1GB after a while it's a bit of a concern. The browser freezes after that.

With an atomic framework performance improves over time globally, across components as each atomic "class" is cached. Likely not reflected in the test either.

@mbrookes mbrookes pinned this issue Aug 24, 2020
@cr101
Copy link

cr101 commented Aug 24, 2020

Happy with either as long as it supports SSR

@sakulstra
Copy link
Contributor

I just withdrew my vote from the original styled components issue 😅 - first learned to know emotion through storybook, but It will mean that all components styles need to be created using the styled API, which means for developers they will always have wrapper components if they need to re-style. got me to switch.

@mnajdova
Copy link
Member Author

mnajdova commented Aug 24, 2020

Thanks everyone for the quick feedback. Here are answers to some of the comments/questions.

What about breaking changes, which option introduce more breaking changes (if any)?

@sag1v using styled-components vs emotion does not introduce any more or less breaking changes that we will need to handle. The overall breaking changes would be around how the overrides inside the theme would look like:

// previosly
root: {
  contained: {
    '&$disabled': { // <-- this part will need to be transformed
      color: 'red',
    },
  },
  containedPrimary: {
    color: 'blue',
  },
}

// after
root: {
  contained: {
     '&.Mui-disabled': {
      color: 'red',
    },
  },
}

However as the styles syntax between emotion & styled-components is identical, it won't make any difference.

Not sure if it makes a difference but were the benchmarks done with emotion and/or style-component's babel plugins? They help to optimize things further.

@hc-codersatlas nope, but the perfs are those are anyway between the top few, so I don't believe it would make any difference. Good call tough!

Why was styletron-react ruled out? It leaves out a whole metric that wasn't considered, which is memory consumption. The default styletron engine (and fela) is atomic. Whilst a bit slower it saves a lot of memory. Having seen a lot of react pages do nothing and go >1GB after a while it's a bit of a concern. The browser freezes after that.

With an atomic framework performance improves over time globally, across components as each atomic "class" is cached. Likely not reflected in the test either.

My comments around why styletron-react was ruled out may be a bit misleading sorry about that, will update the PR description rightaway. The perf are good, but the biggest concern I have with styletron is the community: https://www.npmtrends.com/styletron-react-vs-@emotion/core-vs-styled-components While both emotion and styled-components are over 2000000 downloads in the last 6 months, styletron is around 15000.

Also atomic css may cause issues with overrides, as each classname contains only one styler rule.

I have got a question if we decide using emotion we want to add below code on top of all files?
/** @jsx jsx */
This is JSX pragma, by default JSX pragma is React.createElement

You need to add it if you are using the css property in emotion. For the styled API as well as the regular className API you don't need it.

@Andarist
Copy link
Contributor

It is possible to skip adding JSX pragma, but it requires extra Babel setup and it comes with worse support from the tooling - for example, TS is not able to type-check your CSS prop as accurately as it does when using JSX pragma. More information can be found here: https://github.com/emotion-js/emotion/pull/1941/files#diff-9abe25e5d2b00958d4b9849f5f20c139R5

@re-thc
Copy link

re-thc commented Aug 24, 2020

@mnajdova thanks. I was just hoping memory usage is covered more than vouching for styletron in particular. As to downloads or community - "only" Uber uses styletron :) so no worries. Voted for emotion in the first place.

Would be cool if there was a babel plugin or similar that can transform static styles to real css classes. There's already a similar library called compiled. Most styles realistically are static.

@layershifter
Copy link

To be useful Fela will require a set of plugins like fela-plugin-rtl, fela-plugin-prefixer which will make performance even worse (microsoft/fluentui#12289) 🐢 And then you will probably end up with Emotion (microsoft/fluentui#13547) as sometimes it can be twice faster than Fela 📦

@yordis
Copy link
Contributor

yordis commented Aug 24, 2020

My only concern is having to use css thingy from Emotion. That is a big red-flag based on my experience. I had to remove such thing from a large codebase and wasn't fun. Why? Because it is an abstraction that brings more problems than the one that solves in the long term.

Most of the time, we try to use styled function, but we are quite happy with makeStyles function for generating some classes in some cases.

Hopefully, there is no breaking change for those two APIs, and the I am not forced to use css macro.

@ryancogswell
Copy link
Contributor

ryancogswell commented Aug 24, 2020

My only concern is having to use css thingy from Emotion.

@yordis You definitely won't be forced to use the css prop. I suspect there will be some degree of change for usages of makeStyles and withStyles. Hopefully the amount of change necessary can be mostly limited to a codemod on the imports. I don't think the approach used in makeStyles or withStyles will be practical to support using emotion, so I would expect any ongoing support of those APIs to be through a separate package (so that "@material-ui/core" no longer has a JSS dependency) which would probably receive only minimal maintenance after v5 is released (the details around what happens to makeStyles and withStyles aren't nailed down yet, so this is just my speculation about the implications of moving forward with emotion).

@seanconnollydev
Copy link

👍 the choice to use Emotion. Avoiding the styled API of styled-components is one reason my team chose Emotion (which also supports a similar styled API in addition to the css prop). We presently use Emotion for Material UI style customization and it works pretty well. Built in support would just be icing on the cake.

@petermikitsh
Copy link
Contributor

Regarding these facts:

Many developers use styled-components to override Material-UI's styles. End-users find themselves with two CSS-in-JS libraries in their bundle

Support concurrent mode
styled-components: Partial

If styled-components had full support for concurrent mode, would it be a more sensible choice, given most developers are overriding MUI with styled-components (excluding JSS)? The point about emotion being smaller in bundle size is moot if 2 css-in-js solutions need to be included. And I would presume most practical applications of MUI involve overriding its styles.

@sko-kr
Copy link

sko-kr commented Aug 25, 2020

Where I and my team use Emotion as a main way of styling components, I came across some inefficiencies present in emotion library. And I wonder what you guys think about these inefficiencies explained below.

Consider below Emotion StyledComponent;

const StyledComponent = styled.div`
  ${({color}) => color && `color: ${color}`};
  display: flex;
  justify-content: center;
  align-items: center;
  background: teal;
  font-size: 20px;
  padding-top: 8px;
  padding-bottom: 8px;
  margin-top: 12px;
  margin-bottom: 12px;
  border: 1px solid grey;
`

When color prop changes new css class is generated with all css props (display: flex, justify-content, ..., border: 1px soild grey) copied over. Which would result in css classes with the exact same css props for each color prop, see below;
image

Another inefficiency we have found is when a new component is derived from above StyledComponent it will create a new class with all the css props copied over from base StyledComponent. Consider below;

const DerivedComponent = styled(StyledComponent)`
  font-family: monospace;
`

It creates another css class that only adds font-family: monospace to the above css class generated from StyledComponent. That is, It creates a css that has all the props copied over from StyledComponent as can be seen below;

image

Now if above StyledComponent and DerivedComponent are used together we (initially) have two css classes which have duplicate css props, (differing only in font-family). As can be seen below;

image

As you can imagine a number of css classes that have duplicate css props of each other can grow quite quickly.

I found that with Emotion, as component styles are composed together, we end up with css classes with many duplicate css props.

I am not sure this duplicate of css props in each css class has any noticeable impact on apps, but I wonder if this is taken into account on choosing to use emotion.

I am not questioning the performance of Emotion in creating and applying CSSStyle at run time. Emotion is one of the fastest on applying CSSstyles as evident in perf tests.
I am just concerned resulting CSSstyle is bloated. And as Emotion can be SSR(ed) which means styles are inlined in HTML, we just might get unnecessarily bloated (with css style tags) HTML file. Which in turn results in lot more tags to parse with unnecessary css props by browsers.

@mnajdova
Copy link
Member Author

mnajdova commented Aug 25, 2020

If styled-components had full support for concurrent mode, would it be a more sensible choice, given most developers are overriding MUI with styled-components (excluding JSS)? The point about emotion being smaller in bundle size is moot if 2 css-in-js solutions need to be included. And I would presume most practical applications of MUI involve overriding its styles.

@petermikitsh the reasons why we concluded on emotion are actually the four points in the conclusion

  • A small migration cost between styled-components and emotion
    Developers already using styled-components should be able to use emotion with almost no effort.
  • There are different ways for adding overrides other than wrapper components
    The support of cx + css from emotion can be beneficial for developers to use it as an alternative for adding style overrides if they don't want to create wrapper components.
  • Concurrent mode is for sure supported 👍
  • How the specificity is handled

Having the first point in mind, if developers really want to avoid having two css-in-js solutions in the bundle, the migration cost is really small for moving to emotion + it supports different API other than the styled.

@mnajdova
Copy link
Member Author

@ko-toss thanks for writing this up these are all good points. The fact that emotion generates one className with all styles, makes it the better engine for resolving overrides. The problem we may have with generated multiple classNames is that the last class written will win, which may become problematic in the future.

On another project, I was using atomic css, where from memory consumption is much better because all css rules are written only once, but doing predictable overrides there is pretty hard, as each className is one atomic rule, and than again which one wins, depends on the order of which they are written, if you do not decide to previously process all styles and merge them correctly, which may impact perf in the end.

On the other hand, I believe that using any css-in-js solution, people will not just randomly create infinite combination of styles, they are still pretty structured based on the props value.

However, again these are good points, that we will have in mind, thanks a lot for sharing them 👍

@smmoosavi
Copy link
Contributor

smmoosavi commented Aug 25, 2020

  • else?

what about stylex idea compatibility (like style9)

@shilangyu
Copy link

  • else?

style9 or any other CSS engine that is compatible with stylex idea

This seems to require a build time setup which would discourage many people from using it, especially beginners.

@cztomsik
Copy link

cztomsik commented Aug 25, 2020

(this is rather FYI, emotion is a good choice)

https://github.com/cristianbote/goober (1kB, perf little worse than emotion)

I have no experience with that yet but I want to try it one day.
image
image

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 25, 2020

@cztomsik Similar to https://github.com/kuldeepkeshwar/filbert-js but has no JavaScript syntax support (only CSS template string)

@CrocoDillon
Copy link

The problem if you want to use makeStyles is that you need somehow the theme object, but yes, for sure there is a way to handle this.

Same way as you do now, it's still a hook even though it's returned from a function.

const makeStyles = styles => props => {
  const theme = useTheme()
  // ...
}

@lud-hu
Copy link

lud-hu commented Sep 20, 2021

So I'm currently fiddling around with the new styling philosophy of v5 and I can't really see how I should solve this problem:
I have a <ListItem/> component, that gets the react-router's NavLink component passed like this:

<ListItem
  key="myKey"
  button
  exact
  to="/"
  component={NavLink}
>
  <ListItemText primary="Home" />
</ListItem>

This returns html like this:

<a href="/" ... ><div><span>Home</span></div></a>

So if I now want to use the activeStyle or activeClassName prop of NavLink (see docs). What do I do?

In v4 world, I'd have done it like this:

const useStyles = makeStyles({
  activeNavLink: {
    "> span": {
        fontWeight: "bold"
      }
  }
});

<ListItem
  key="myKey"
  button
  exact
  to="/"
  component={NavLink}
  activeClassName={classes.activeNavLink}
>
  <ListItemText primary="Home" />
</ListItem>

I know there are different ways to solve this. But how to approach this particular use case?

@mnajdova
Copy link
Member Author

@lud-hu please create an issue with a codesandbox link where we can take a look at the specific scenario. You can probably do something similar to:

<ListItem
  key="myKey"
  button
  exact
  to="/"
  component={NavLink}
  sx={{'&.custom-dense-class': { '> span': { fontWeight: "bold" } }}
  classes={{ dense: 'custom-dense-class'}}
>
  <ListItemText primary="Home" />
</ListItem>

@lud-hu
Copy link

lud-hu commented Sep 20, 2021

@mnajdova Thanks!
It works pretty much like you suggested:

<ListItem
  key="myKey"
  button
  exact
  to="/"
  component={NavLink}
  sx={{
    '&.custom-dense-class': { '& span': { fontWeight: 'bold' } },
  }}
  activeClassName='custom-dense-class'
>
  <ListItemText primary="Home" />
</ListItem>

I guess it needs some time to get used to the new mechanism.

@cristobalchao
Copy link

Pretty sad to see that JSS is no longer supported

@garronej
Copy link
Contributor

@cristobalchao You can checkout tss-react.

@mbrookes
Copy link
Member

Pretty sad to see that JSS is no longer supported

It's still supported - you can use whichever styling solution you chose in your app and to restyle MUI components. It's just more efficient for bundle size to use the same as the MUI components.

@garronej
Copy link
Contributor

garronej commented Oct 10, 2021

tss-react finally features a, type safe by design, implementation of withStyles(). With that I release v1 🚀

demo_withStyles

@EloB
Copy link
Contributor

EloB commented Nov 6, 2021

A question using like non primivites (object, functions and arrays etc) has always been tricky in React performance. I've recently went over to v5 and saw the sx using objects. Won't that always break pure functions or is there some black magic in the background that solves this?

By passing new objects all the time it won't be easy to add React.memo for heavy components or is there any smart plugin that moves static objects outside the component or is the preferred way to put all sx object outside the component from the beginning?

@mnajdova
Copy link
Member Author

mnajdova commented Nov 8, 2021

@EloB there is no magic, nothing is done. If you notice perf issues on a component, you can memoize the object inside the sx prop to see if there will be a difference.

@EloB
Copy link
Contributor

EloB commented Nov 8, 2021

@mnajdova At that point it's often really late. When a complete app built the "wrong way" you need to refactor a lot of code. These type of performance issues often happens in the "late game" and are really hard to fix at that stage.

For me React development is like game development in a sense. Where game developers historically looked a lot on amount of polygons and where React developers have to think about amount of virtual renders. Game developers don't think about that when the product was "finished". It was part of their everyday thoughts. React on small scale is rarely a problem same as games. These types of issues occur when you move from being a prototype to a fullscale enterprise app. All the wrong doing is hard to correct at that stage and also hard to pinpoint because you suddenly "hit the roof" of what the software/hardware could handle (also often discovered by end-users because they don't have the expensive laptops that developers have) and at that point doesn't exists a easy/quick fix. So documenating best practice would be really nice :)

Don't get me wrong! I love material ui and want it to be part of everything I do. It helped me a lot but it would be nice if it was self educational by using it. Like follow these patterns and you won't get these problems in the end when using sx or provide a plugin that automatically does that 😃. For me these best practises putting like "static objects" outside "render function" is mandatory but being a consultant seeing a lot of codebases I seen that people tend to not do that and making computer fans sound like harley davidssons for simple applications 😄

@garronej

This comment has been minimized.

@EloB
Copy link
Contributor

EloB commented Nov 8, 2021

@garronej I often create something like that. I've been working with Smart TV applications where hardware/software are really crap. So I've been really interested in performance and React.

I tend to create all my callbacks inside a useMemo and use it like useMemo(() => ({ handle1: () => {}, handle2: () => {] }), []) combined with either ref value or useState if I need dynamic data.

// helpers
const getValues = (...setters) => setters.map((setter) => {
  let value;
  setter(current => {
    value = current;
    return current;
  });
  return value;
});

const update = (setter, states) => setter((current) => {
  const merged = { ...current, ...states };
  return shallowEqual(current, merged) ? current : merged;
});

// used like

const [state, setState] = useState();
const handles = useMemo(() => ({
  handleUsingState: () => {
    const [currentState] = getValues(setState);
    console.log(currentState);
  },
  handleUsingRef: () => console.log(refProp.current),
}), []);

Also only use one useState per component. To prevent multiple renders.

const [state1, setState1] = useState(null);
const [state2, setState2] = useState(null);

// Better todo something like this so you can change multiple states at the same time
const [state, setState] = useState(defaultState); // { state1: null, state2: null }

// Because
<button onClick={() => {
  // Will trigger one virtual render
  setState1("one1");
  setState2("two1");
  
  setTimeout(() => {
    // and this will trigger two virtual renders because it's not in a react triggered event loop
    // even if the timeout was created during a react event loop but that isn't something that react keeps track of
    setState1("one2");
    setState2("two2");
    // setState({ state1: "one2", state2: "two2" }) would always trigger one virtual render
    // or using a helper like that update function. To prevent even further rerenders if nothing changed.
    // update(setState, { state1: "one2", state2: "two2" })
  });
}} />

This is a bit of side track from the initial conversation 😅

@a-tonchev
Copy link

@EloB @garronej guys I think it would be not bad idea to make blog/articles in this direction. The topic is quite interesting!

@a-tonchev
Copy link

@EloB If you want to optimize the styles more, you could try to use global css variables:

import { css } from '@emotion/react';
import { useTheme } from '@mui/material';

const GlobalStyles = () => {
  const theme = useTheme();

  const styles = useMemo(() => css({
    ':root': {
      '--theme-breakpoints-values-sm': `${theme.breakpoints.values.sm}px`,
      '--theme-spacing-1': theme.spacing(1),
      '--theme-spacing-2': theme.spacing(2),
      '--theme-spacing-3_0_2': theme.spacing(3, 0, 2),
      '--theme-spacing-0_1': theme.spacing(0, 1),
      '--theme-spacing-8': theme.spacing(8),
      '--theme-palette-primary-main': theme.palette.primary.main,
      '--theme-palette-success-main': theme.palette.success.main,
      '--theme-palette-warning-main': theme.palette.warning.main,
      '--theme-palette-secondary-main': theme.palette.secondary.main,
      '--theme-palette-success-contrastText': theme.palette.success.contrastText,
      '--theme-palette-primary-contrastText': theme.palette.primary.contrastText,
      '--theme-palette-error-contrastText': theme.palette.error.contrastText,
      '--theme-palette-error-main': theme.palette.error.main,
    },
  }), [theme]);

  return <Global styles={styles} />;
};

And put this element inside some top-level component.

Then you can use everywhere the styles like this:

const mySxValues = {
  backgroundColor: 'var(--theme-palette-secondary-main)',
};

Or in styles:

const styles = {
  someClass: {
    backgroundColor: 'var(--theme-palette-secondary-main)',
  },
};

Thus you don't need to generate dynamically the theme object for your styles and you get better performance.
Beside that you can use pure css using dynamically the values from your material themeprovider.

@EloB
Copy link
Contributor

EloB commented Nov 8, 2021

@a-tonchev Cool idea! How is the support of css variables?

@a-tonchev
Copy link

@EloB all except IE supports them

https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_custom_properties

@garronej
Copy link
Contributor

garronej commented Nov 9, 2021

@EloB while I prefer the useConstCallback approach to your first example, I am very grateful to you for sharing the fact that the sequential states updates in the timer's callback triggers two re-renderers.

@cristobalchao
Copy link

Pretty sad to see that JSS is no longer supported

It's still supported - you can use whichever styling solution you chose in your app and to restyle MUI components. It's just more efficient for bundle size to use the same as the MUI components.

It won't be supported after v5 and to be honest the options to use instead are not convincing me, an example is "style-components", IMO from a development experience doesn't seem right to me. I might need to start looking somewhere else

@garronej
Copy link
Contributor

garronej commented Nov 29, 2021

@mnajdova @oliviertassinari

We finally managed to come up with a nested selector API that is much closer to how it worked in v4's makeStyles while still being fully type safe. 🎉
Quality of life improvement is particularly noticeable with the withStyles API:

demo_nested_selectors_withStyles_tss.mp4

The new changes should be merged soon, after that I should be able to start working on the codemod for migration from @material-ui/styles's makeStyles to tss-react

@mnajdova
Copy link
Member Author

@garronej nice! This is much closer to how it was in v4. Great work 🎉 Looking forward to the codemod, I guess lots of people that haven't migrated yet, would be able to do it more easily after it is available. Thanks 🙏

@godmar
Copy link

godmar commented Jan 7, 2022

Could the documentation on this change perhaps be improved? As is, I find it very difficult to distinguish what is now preferred and what is legacy. In particular, is the use of styled now legacy or still recommended? And if so, which version of styled?
Is the creation of wrapper components that use sx now recommended (I believe yes (?), but can't be sure from reading the documentation.)

I'm also wondering about the compatibility with other frameworks. For instance, I thought that in order to express relationships such as "make an element visible or change color when the user hovers over it" I would need to have the ability use the components selector API. According to How to use the components selector API this means I need to use a custom babel configuration even when using MUI's default styling solution, emotion. This, in turns, kicks me out of the use of SWC and requires more custom configuration when used with next.js.

I would much prefer to stick with the default configuration and recommended options in all of the frameworks and libraries I'm integrating.

@mnajdova
Copy link
Member Author

@godmar thanks for your feedback. For sure, we need to work on the documentation. @danilo-leal had a great suggestion on how we may approach this. As for now, everything under the System and Customization is preferred, everything under Styles (legacy) is legacy. You can find these as section in the Docs' drawer.

@godmar
Copy link

godmar commented Jan 11, 2022

@godmar thanks for your feedback. For sure, we need to work on the documentation. @danilo-leal had a great suggestion on how we may approach this. As for now, everything under the System and Customization is preferred, everything under Styles (legacy) is legacy. You can find these as section in the Docs' drawer.

Thank you. I'm still confused, though.

For instance, Styled components API is under the "Styles (legacy) -> basics" drawer and describes the use of const MyButton = styled(Button)({ .... }) - so this would be legacy.

However, under System -> styled -> API here it describes the use of styled in a very similar or identical way - so is styled simultaneously legacy and recommended?
Am I missing a subtle difference between one styled and the other?

Side by side:

Under the documentation portion that you say is recommended (under System -> styled), it says:

Introduction
All the MUI components are styled with this styled() utility. This utility is built on top of the styled() module of @mui/styled-engine and provides additional features.
Import path
You can use the utility coming from the @mui/system package, or if you are using @mui/material, you can import it from @mui/material/styles. The difference is in the default theme that is used (if no theme is available in the React context).

Yet, elsewhere it says:

@mui/styles is the legacy styling solution for MUI. It is deprecated in v5. It depends on JSS as a styling solution, which is not used in the @mui/material anymore. If you don't want to have both emotion & JSS in your bundle, please refer to the @mui/system documentation which is the recommended alternative.

@mnajdova
Copy link
Member Author

mnajdova commented Mar 2, 2022

@godmar the @mui/styles package is the legacy. Everything imported from that package is considered legacy API.

What we use in v5, and recommend developers to use are the utilities from @mui/system or @mui/material/styles if they are already using the material package. The styled utility happens to exists in both packages, I guess this is where the confusion comes from. I hope this clarifies things a bit.

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

No branches or pull requests