Material-ui: [system] Boxes with identical style props use the same class

[johnnynia]

Every call of a Box component with identical style props creates a new CSS class.

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

All calls of a Box component with identical style props use the same CSS class.

Current Behavior 😯

Every call to a Box component with identical props creates and uses a new CSS class.

Examples 🌈

https://codesandbox.io/s/create-react-app-3its9
Box1 and Box2 have different classes.

Context 🔦

Using the Box component instead of CSS classes looks very promising. But while the multiplication of CSS classes may may be negligible in the browser, I'm hesitant to use it in our application which is very reliant on SSR and injecting critical CSS into HTML.

Maybe it would be possible to add some sort of a cache, in which the style props would act as key and the class as value?

A solution like this would lend itself to an "atomic CSS classes" approach (see related issue) while still only serving classes that are actually used.

– Or maybe I'm just missing something.

Related: https://github.com/mui-org/material-ui/issues/15499

[ypresto]

Box is currently using makeStyles() via styled(), which inserts <style> tag for every style using dynamic props.
Inlining styles into HTML instead of using styled() performs much faster and not slower than static styled().

Rendering 3000 elements (tried two times, not production):

• MBP 15 inch 2019 i7

• Chrome 75.0.3770.100

• inlined style version of Box: 620ms, 640ms

• static styled(): 780ms, 765ms
• styled() version of Box (original): 3.28s, 3.28s

(They have different border each other, to distinguish whether rendering is completed or not.)

You can test it here:
https://codesandbox.io/embed/box-inlined-css-perf-b9buv

I think it is a good choice to let Box inline styles, as it's style is always dynamic and perhaps relatively small.
(The challenge is to support media query.)

[ypresto]

Implemented caching makeStyles() result in Box (#16858)

Considerations

Put cache logic to Box, styled or makeStyles?

• makeStyles is responsible to cache styles, but logic around cache key is bit specific to styled or Box.
• Box only accepts non-function types (as style() and responsivePropType()). We cannot restrict props to JSON-compatible values if it used for arbitrary style functions.
• styled handles meta data from style functions (filterProps and propTypes). We can use some flag like isStaticProps or cachePerProps.
• maybe it is jss-side issue: https://github.com/cssinjs/jss/issues/501

Which the cache key is better, JSON.stringify(props) or whole style sheet?

• Props of Box should be relatively small and expects fast performance.
• Whole stylesheet requires large string comparison and large memory, but
  
  
  
  • has no JSON-compatibility issue.
  
  
  • alias-aware cache (e.g. m and margin)
  
  
  • insensitive for order in props object
  
  

[ypresto]

Which the cache key is better, JSON.stringify(props) or whole style sheet?

stylesOptions.jss.createStyleSheet() is the slowest part of dynamic style handling (2nd seems to be dynamicSheet.update(props).attach()).
Whole style sheet cache is more difficult because we should generate cache key without touching jss; to evaluate style object by hand.

Put cache logic to Box, styled or makeStyles?

makeStyles could be completely replaced by react-jss in the future, as #16180.
I chose styled() in my PR #16858.

[elxris]

@ypresto I forked your codesandbox and added styled-components implementation with @material-ui/system styled functions. I'm surprised to found that styled-components don't generate different class names for same props and are super performant.

https://codesandbox.io/embed/box-inlined-css-perf-2rztv

This seems like a beneficial point to the recent discussion about migrating to styled-components (#6115)

[oliviertassinari]

From what I understand this issue is all about performance. The v5.0.0-alpha.13 version of the Box component is a lot faster (x3) than v4, see https://github.com/mui-org/material-ui/issues/21657#issuecomment-707111575.

We can use ui-box which pushes in the atomic CSS path for a Box (but without shorthand, theme, breakpoints, etc.) as an approximation to understand the potential. If you take the benchmark @mnajdova is building #23180, and run a similar test case, you will find:

• noop (baseline):
  Mean: 2.45ms
• React primitives:
  Mean: 70.73ms
• Cost of React:
  Mean: 76.71ms
• UI Box:
  Mean: 99.68ms
• Styled alternative:
  Mean: 116.12ms
• Box emotion:
  Mean: 162.04ms

Test cases in https://github.com/oliviertassinari/material-ui/commit/f01a3fa992, build on top of #23180 (@mnajdova hopefully it's helpful and you can leverage some of it)

So yes, I think that using atomic class-names has the potential to help with performance, in the order of winning 60% of performance. Which is about what using Tailwind would yield (but come with different issues: CSS purging that doesn't scale, low TypeScript safety, large API to remember).

Atomic class-names also has significant challenges. Does the win potential worth spending time on it? What about bundle size? ui-box is almost larger than emotion. If we use emotion can we really implement it without making performance worse?

We have also seen https://stitches.dev/ try that direction, I haven't seen a compelling benchmark. It also requires to drop the support for nested selectors (but rarely used in the system anyway).

I'm adding the "waiting for upvotes" in order to edge against the potential high-opportunity cost.

[tarwich]

I hope this doesn't just add noise, but I arrived here from specific use case which I'd like to share in case it helps.

When I have a page with 1000 items on it, and I'm in the browser developer tools, I'd like to make an adjustment to a single class and see them all update. Sometimes this is my flow for testing out padding or sizing in odd lists. I aware that I can write styled('div') or makeStyles(), but the wonderful thing about Box was having the css right there in the JSX so it was easy to tweak and manage.

When I noticed that every render was creating 1000 new class names, it scared me. I thought it would slow the browser down or eat up memory. However, even when I chose to accept those potential issues, the developer tools thing was still important to me.

[oliviertassinari]

@tarwich Would this cut it?

import React from "react";
import Box from "@material-ui/core/Box";

export default function App() {
  return (
    <Box sx={{ "& > .child": { m: 1, bgcolor: "primary.main" } }}>
      {new Array(1000).fill().map(() => (
        <div className="child">a</div>
      ))}
    </Box>
  );
}

https://codesandbox.io/s/gallant-star-iqlxo?file=/src/App.js:0-288

---

The performance tradeoff is now documented in https://next--material-ui.netlify.app/system/basics/#performance-tradeoff. We can likely make it faster.

[tarwich]

I like that thought, @oliviertassinari and I didn't know you could even do that. I'm not sure it will work precisely for my situation. I've ported a real example to CodeSandbox to demonstrate the issue.

1. Each grid box has a separate class, so if I wanted to mess around with font size or borders, for example, I can't use a single class to do it.
2. Every time you type in the filter box, it generates a minimum of 54 new classes, which is probably fine, but is a scary thought.

https://codesandbox.io/s/material-theme-colors-l9jxv?file=/src/App.js

Once again, I'm hoping this helps support this issue and not detract from it.

[mbrookes]

@tarwich Off topic, but mind if I ask what you're working on? Looks intriguing. 🙂

[tarwich]

What are we doing with the colors?

That's part of a "Kitchen Sink" inspired by Semantic UI's Kitchen Sink. Most of the components that make up our app have a demo section on our kitchen sink page so that new developers can easily see what to use and how to implement it. It also helps when developing new components as it has some minor Storybook functionality. The particular colors page is so I can easily see the colors defined in our theme and quickly find one i.e. error. Since I'm not intimately familiar with the Material UI palette, sometimes it's easier to find a color visually, then see what it's called instead of the other way around. In other cases, the names aren't immediately obvious to me. The linked theme, for example has secondary.main, and secondary.contrastText, so I would expect to see secondary.text, but instead it's text.secondary 🤯.

What are we doing in general?

Our application is an online education platform, and was built using makeStyles all over the place. That works, and is similar to CSS, but there's a bit of a cognitive break when you have to leave your spot, jump back up to the styles, tweak something, then go back. We found <Box /> and went nuts, refactoring everything to use Box was easy and makes our code much cleaner. Our rule of thumb is basically that if you need the same Box styles in multiple places, it should probably be a component. However, even doing that results in different classes being output every time. For example, with the colors we could have made <Swatch /> components which would've been cleaner, but if internally the component output <Box /> components every time, we would still have had thousands of classes. In the end we might have to go back to makeStyles, but I was really hopeful for Box as it was very clean.

What are we doing with box?

If I want all "Swatch" instances to have the same class, then I might do something like this:

const gatherMap = new Map<Component, CssDefinition>();

const Swatch = () => {
  return <Box gather={Swatch} name="swatch" />
}

Wherein the component definition itself could be used to group class definitions. That helps prevent two components from using the same "key" for example having two distinct "swatch" components. I toyed with this idea a bit like so:

const gatherMap = new Map<Component, CssDefinition>();

const builtStyle = (component, name, style, child) => {
  if (gatherMap.has(component)) return gatherMap.get(component);

  const newComponent = styled(child)(style, {name});

  gatherMap.set(component, newComponent);

  return newComponent;
}

After writing this a bit, I realized I might be too small-brained to make this work, so I started googling and found I'm not the only one with this problem and better men than me are struggling, so I've decided I need to rethink life. Either we can't use Box the way I wanted (go back to makeStyles), we have to wait for a solution, or we have to accept thousands of classes being spit out, or (_insert someone smarter's idea here_).

[mbrookes]

@tarwich I'm so glad I asked! Thank you for sharing your experience in such detail.