[core] Update overridesResolver for slots #25850

mnajdova · 2021-04-20T11:29:42Z

TL;DR

I proposed we go with Option 1 which is using overridesResolver per slot as original done in #25809 by @siriwatknp Perf compared to current implementation is way better and it is not slower compared to Option 2, but gives us much better readability + flatten specificity 🚀

This PR set up benchmark test for the Slider component, where we can measure perf differences between two options of how we may define the overridesResolver for the slots components.

Option 1 - use `overridesResolver` for each slot of the component:

index e55139e3d9..3578ed4ef9 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -42,19 +42,6 @@ const overridesResolver = (props, styles) => {
       ...styles[`color${capitalize(styleProps.color)}`],
       ...(marked && styles.marked),
       ...(styleProps.orientation === 'vertical' && styles.vertical),
-      [`& .${sliderClasses.rail}`]: styles.rail,
-      [`& .${sliderClasses.track}`]: {
-        ...styles.track,
-        ...(styleProps.track === 'inverted' && styles.trackInverted),
-        ...(styleProps.track === false && styles.trackFalse),
-      },
-      [`& .${sliderClasses.mark}`]: styles.mark,
-      [`& .${sliderClasses.markLabel}`]: styles.markLabel,
-      [`& .${sliderClasses.valueLabel}`]: styles.valueLabel,
-      [`& .${sliderClasses.thumb}`]: {
-        ...styles.thumb,
-        ...styles[`thumbColor${capitalize(styleProps.color)}`],
-      },
     },
     styles.root || {},
   );
@@ -135,7 +122,7 @@ export const SliderRoot = experimentalStyled(
 export const SliderRail = experimentalStyled(
   'span',
   {},
-  { name: 'MuiSlider', slot: 'Rail' },
+  { name: 'MuiSlider', slot: 'Rail', overridesResolver: (props, styles) => styles.rail },
 )(({ styleProps }) => ({
   display: 'block',
   position: 'absolute',
@@ -156,7 +143,15 @@ export const SliderRail = experimentalStyled(
 export const SliderTrack = experimentalStyled(
   'span',
   {},
-  { name: 'MuiSlider', slot: 'Track' },
+  {
+    name: 'MuiSlider',
+    slot: 'Track',
+    overridesResolver: ({ styleProps = {} }, styles) => ({
+      ...styles.track,
+      ...(styleProps.track === 'inverted' && styles.trackInverted),
+      ...(styleProps.track === false && styles.trackFalse),
+    }),
+  },
 )(({ theme, styleProps }) => ({
   display: 'block',
   position: 'absolute',
@@ -184,7 +179,14 @@ export const SliderTrack = experimentalStyled(
 export const SliderThumb = experimentalStyled(
   'span',
   {},
-  { name: 'MuiSlider', slot: 'Thumb' },
+  {
+    name: 'MuiSlider',
+    slot: 'Thumb',
+    overridesResolver: ({ styleProps = {} }, styles) => ({
+      ...styles.thumb,
+      ...styles[`thumbColor${capitalize(styleProps.color)}`],
+    }),
+  },
 )(({ theme, styleProps }) => ({
   position: 'absolute',
   width: 12,
@@ -250,7 +252,11 @@ export const SliderThumb = experimentalStyled(
 export const SliderValueLabel = experimentalStyled(
   SliderValueLabelUnstyled,
   {},
-  { name: 'MuiSlider', slot: 'ValueLabel' },
+  {
+    name: 'MuiSlider',
+    slot: 'ValueLabel',
+    overridesResolver: (props, styles) => styles.valueLabel,
+  },
 )(({ theme }) => ({
   // IE 11 centering bug, to remove from the customization demos once no longer supported
   left: 'calc(-50% - 4px)',
@@ -273,7 +279,7 @@ export const SliderValueLabel = experimentalStyled(
 export const SliderMark = experimentalStyled(
   'span',
   {},
-  { name: 'MuiSlider', slot: 'Mark' },
+  { name: 'MuiSlider', slot: 'Mark', overridesResolver: (props, styles) => styles.mark },
 )(({ theme, styleProps }) => ({
   position: 'absolute',
   width: 2,
@@ -289,7 +295,7 @@ export const SliderMark = experimentalStyled(
 export const SliderMarkLabel = experimentalStyled(
   'span',
   {},
-  { name: 'MuiSlider', slot: 'MarkLabel' },
+  { name: 'MuiSlider', slot: 'MarkLabel', overridesResolver: (props, styles) => styles.markLabel },
 )(({ theme, styleProps }) => ({
   ...theme.typography.body2,
   color: theme.palette.text.secondary,

Option 2 - use one `overridesResolver` without `deepmerge`

index e55139e3d9..f11cadd859 100644
--- a/packages/material-ui/src/Slider/Slider.js
+++ b/packages/material-ui/src/Slider/Slider.js
@@ -1,7 +1,7 @@
 import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
-import { chainPropTypes, deepmerge } from '@material-ui/utils';
+import { chainPropTypes } from '@material-ui/utils';
 import { generateUtilityClasses, isHostComponent } from '@material-ui/unstyled';
 import SliderUnstyled, {
   SliderValueLabelUnstyled,
@@ -37,27 +37,25 @@ const overridesResolver = (props, styles) => {

   const marked = marks.length > 0 && marks.some((mark) => mark.label);

-  return deepmerge(
-    {
-      ...styles[`color${capitalize(styleProps.color)}`],
-      ...(marked && styles.marked),
-      ...(styleProps.orientation === 'vertical' && styles.vertical),
-      [`& .${sliderClasses.rail}`]: styles.rail,
-      [`& .${sliderClasses.track}`]: {
-        ...styles.track,
-        ...(styleProps.track === 'inverted' && styles.trackInverted),
-        ...(styleProps.track === false && styles.trackFalse),
-      },
-      [`& .${sliderClasses.mark}`]: styles.mark,
-      [`& .${sliderClasses.markLabel}`]: styles.markLabel,
-      [`& .${sliderClasses.valueLabel}`]: styles.valueLabel,
-      [`& .${sliderClasses.thumb}`]: {
-        ...styles.thumb,
-        ...styles[`thumbColor${capitalize(styleProps.color)}`],
-      },
+  return {
+    [`& .${sliderClasses.rail}`]: styles.rail,
+    [`& .${sliderClasses.track}`]: {
+      ...styles.track,
+      ...(styleProps.track === 'inverted' && styles.trackInverted),
+      ...(styleProps.track === false && styles.trackFalse),
     },
-    styles.root || {},
-  );
+    [`& .${sliderClasses.mark}`]: styles.mark,
+    [`& .${sliderClasses.markLabel}`]: styles.markLabel,
+    [`& .${sliderClasses.valueLabel}`]: styles.valueLabel,
+    [`& .${sliderClasses.thumb}`]: {
+      ...styles.thumb,
+      ...styles[`thumbColor${capitalize(styleProps.color)}`],
+    },
+    ...styles.root,
+    ...styles[`color${capitalize(styleProps.color)}`],
+    ...(marked && styles.marked),
+    ...(styleProps.orientation === 'vertical' && styles.vertical),
+  };
 };

 export const SliderRoot = experimentalStyled(

Benchmark Azure Job build - https://dev.azure.com/mui-org/Material-UI/_build/results?buildId=26337&view=logs&j=9ef21fd1-5d60-5fa4-f8b2-6dc79e173863&t=516d74c5-15a9-50a0-24d9-dba41a487a2d

noop (baseline):
  05.47 ±00.29ms
slider-current-implementation:
  769.04 ±27.65ms
slider-multiple-overrides-resolver:
  97 ±3%
slider-single-overrides-resolver:
  96 ±4%
Done in 56.24s.

Based on the benchmark, seems like both options are giving us way better perf over the current implementation that uses deepmerge 🚀

769.04 ±27.65ms -> 97 ±3%
769.04 ±27.65ms ->96 ±4%

As the perf difference between the two options is negligible, I'd say we go with Option 1, which gives us better readability and flatter specificity. Credits go to @siriwatknp for giving us this idea in #25809

mui-pr-bot · 2021-04-20T11:31:46Z

Bundle size will be reported once Azure build #26336 finishes.

Generated by 🚫 dangerJS against 91fd13d

mnajdova · 2021-04-20T11:52:00Z

@oliviertassinari @eps1lon should I proceed with updating the components to use Option 1 (overridesResolver per slot component)?

oliviertassinari · 2021-04-20T13:27:27Z

👍 for option 1. I think that it will be cleared for developers that look at our sources for making overrides too.

I'm confused about how the performance build was run but seeing no difference seems coherent, so all good from my end.

mnajdova · 2021-04-20T13:30:00Z

👍 for option 1. I think that it will be cleared for developers that look at our sources for making overrides too.

~~Great, will update all components as part of the PR.~~ Will open new PR for changing all components, so that we can have this one for reference.

I'm confused about how the performance build was run but seeing no difference seems coherent, so all good from my end.

It was run as described in #23895 In addition to this, local run gave pretty much the same results.

mnajdova · 2021-04-20T13:38:39Z

I noticed now that I didn't remove the deepmerge in Option 1, but as we don't need it anymore it means that it could be even faster 🚀

Edit: Actually it shouldn't affect perf, as we didn't have anything to deep merge depending on the styles.

mnajdova · 2021-04-20T14:31:56Z

Closing this one, will be creating new PRs for converting components to this approach

siriwatknp · 2021-04-20T15:01:25Z

adding another benefit of option 1 compare to others is the flat css specificity. I think it is totally better than having nested css. The DX is also close to v4 (which I like) because if dev want to custom the track slot, they will do

MuiSlider: {
  styleOverrides: {
    track: {
      boxShadow: 'inset 0 0 0 1px rgba(0,0,0,0.12)',
      backgroundColor: '#fff',
    }
  }
}

and they expect to see this in devtool.

.css-MuiSlider-track {
  ....other css
  boxShadow: inset 0 0 0 1px rgba(0,0,0,0.12);
  backgroundColor: #fff
}

mnajdova · 2021-04-20T15:04:34Z

@siriwatknp exactly, two main benefits that I could find are better readability + flatten specificity :)

mnajdova · 2021-04-20T15:10:35Z

The only issue I could find is that sometimes, on components, where we allow users to use their own components instead of the slots we create with styled() the overrides are not applied, as they are no longer added on the root :(

I could spot this issue by the first 10 components I've tried to migrate - #25853 I believe that in some places, we will still need to have the previous class overrides, for example on the AvatarGroup's avatar slot. However I would not use the deepmerge there, and we can simply expect that if you define some override on the root they would overwrite. cc @oliviertassinari

mnajdova · 2021-04-20T15:15:17Z

Also I believe we should disable the testDeepOverrides as we no longer support that.

siriwatknp · 2021-04-20T15:51:19Z

@mnajdova yeah, that is one downside of the approach. I faced it when doing the migration for TreeItem. I need to fix the demo (Component override example) that the styles in child is no longer there. But may be it make sense because I override the whole Component so the styling should be my responsibility 🤔
https://github.com/mui-org/material-ui/pull/25835/files#diff-d13f58feeb07c436bd15577c6cba85464bac9727185eae50072879b67ed22601

mnajdova · 2021-04-21T15:30:13Z

@mnajdova yeah, that is one downside of the approach. I faced it when doing the migration for TreeItem. I need to fix the demo (Component override example) that the styles in child is no longer there. But may be it make sense because I override the whole Component so the styling should be my responsibility 🤔
https://github.com/mui-org/material-ui/pull/25835/files#diff-d13f58feeb07c436bd15577c6cba85464bac9727185eae50072879b67ed22601

We need to be careful when we need to keep the styles on the root. On some components we need combination of both, for example: https://github.com/mui-org/material-ui/pull/25853/files#diff-f6f8d57d4a4564948650e3965be686e345389e93ec13cba5923d0bddacbc5c2fR35

init

91fd13d

mnajdova requested review from oliviertassinari and eps1lon April 20, 2021 11:51

mnajdova mentioned this pull request Apr 20, 2021

[Tabs] Migrate Tabs to emotion #25824

Merged

1 task

mnajdova closed this Apr 20, 2021

mnajdova mentioned this pull request Apr 20, 2021

[core] Update slot components to use overridesResolver part 1 #25853

Merged

zannager added the core Infrastructure work going on behind the scenes label Jan 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Update overridesResolver for slots #25850

[core] Update overridesResolver for slots #25850

mnajdova commented Apr 20, 2021 •

edited

Loading

mui-pr-bot commented Apr 20, 2021

mnajdova commented Apr 20, 2021

oliviertassinari commented Apr 20, 2021

mnajdova commented Apr 20, 2021 •

edited

Loading

mnajdova commented Apr 20, 2021 •

edited

Loading

mnajdova commented Apr 20, 2021

siriwatknp commented Apr 20, 2021

mnajdova commented Apr 20, 2021

mnajdova commented Apr 20, 2021

mnajdova commented Apr 20, 2021

siriwatknp commented Apr 20, 2021

mnajdova commented Apr 21, 2021

[core] Update overridesResolver for slots #25850

[core] Update overridesResolver for slots #25850

Conversation

mnajdova commented Apr 20, 2021 • edited Loading

TL;DR

Option 1 - use overridesResolver for each slot of the component:

Option 2 - use one overridesResolver without deepmerge

mui-pr-bot commented Apr 20, 2021

mnajdova commented Apr 20, 2021

oliviertassinari commented Apr 20, 2021

mnajdova commented Apr 20, 2021 • edited Loading

mnajdova commented Apr 20, 2021 • edited Loading

mnajdova commented Apr 20, 2021

siriwatknp commented Apr 20, 2021

mnajdova commented Apr 20, 2021

mnajdova commented Apr 20, 2021

mnajdova commented Apr 20, 2021

siriwatknp commented Apr 20, 2021

mnajdova commented Apr 21, 2021

mnajdova commented Apr 20, 2021 •

edited

Loading

Option 1 - use `overridesResolver` for each slot of the component:

Option 2 - use one `overridesResolver` without `deepmerge`

mnajdova commented Apr 20, 2021 •

edited

Loading

mnajdova commented Apr 20, 2021 •

edited

Loading