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

[system] Fix CssVarsProvider theme mutation #31148

Merged
merged 5 commits into from
Feb 22, 2022

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Feb 19, 2022

Based no #31138, there is a problem when baseTheme provided to createCssVarsProvider contains functions. Those functions are removed because of JSON.stringify to make the object fresh on every render (the logic inside CssVarsProvider mutate the object, so we need to make sure that the object is new on every call, otherwise the multiple CssVarsProvider on a single page would not work).

There are 2 possible ways to fix this:

  1. receive basedTheme as a function that returns a new object
  2. fix the logic inside CssVarsProvider to not mutate the object

In this PR, I chose the 2nd option because it is safer. It guarantees that the object is new on every renders and developers do not need to be concerned about the theme they are passing.

Note: the first option seems not a good idea because if the function returns the same object, we will face the same problem.

Performance

From what I collect, there is no performance difference between before and after. Collected by capturing the time (in millisecond) that the logic used in each render (sample sizes are 5).

Note: 4x & 6x is the CPU slowdown from devtool setting

Before

1x 4x 6x
2.39892578125 8.531982421875 11.440185546875
4.144775390625 9.327880859375 12.02197265625
2.445068359375 10.386962890625 12.864013671875
2.839111328125 8.4951171875 13.427001953125
2.4599609375 7.711669921875 12.79296875
     

After

1x 4x 6x
3.626220703125 8.1669921875 11.341064453125
2.31201171875 10.117919921875 12.72119140625
2.280029296875 9.003173828125 12.02099609375
2.296875 8.082763671875 12.14306640625
2.466064453125 7.845947265625 12.010009765625

Test

Added regression test to make sure that multiple instances of <CssVarsProvider> with different prefix works.


@mui-bot
Copy link

mui-bot commented Feb 19, 2022

Details of bundle changes

Generated by 🚫 dangerJS against ad4b7ca

Comment on lines -137 to -139

// scope is the deepest object in the tree, keys is the theme path keys
scope[keys.slice(-1)[0]] = value;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the part where the theme is mutated.

},
(keys) => keys[0] === 'vars', // skip 'vars/*' paths
);

return { css, vars };
return { css, vars, parsedTheme };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parsedTheme is the new object which will make functions inside baseTheme work.

@siriwatknp siriwatknp requested a review from mnajdova February 21, 2022 07:36
@siriwatknp siriwatknp marked this pull request as ready for review February 21, 2022 07:36
if (key === resolvedColorScheme) {
mergedTheme = {
...mergedTheme,
...parsedScheme,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this override the mergedTheme palette values? Isn't the result of parsedScheme a object containing palette as the first key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should override. When you change the color scheme, you will get a new theme.palette.*

Copy link
Member Author

@siriwatknp siriwatknp Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally must, mergedTheme should not have a palette as a key because the palette is handled inside color schemes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if the original theme contains more values there, like functions in the MD theme, we will loose them again no?

Copy link
Member Author

@siriwatknp siriwatknp Feb 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, this works but the functions need to be in all color schemes.

createCssVarsProvider({
  theme: {
    colorSchemes: {
      light: {
        palette: {
          getContrastText: (...) => {},
        },
      },
      dark: {
        palette: {
          getContrastText: (...) => {},
        },
      }
    }
  }
})

const theme = useTheme();
theme.getContrastText(...)

However, if the function is only declared in light colo scheme, theme.getContrastText will be undefined when you toggle dark color scheme. Is this making sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally, mergedTheme should not have a palette as a key because the palette is handled inside color schemes.

I would then say we should scratch ideally :D The base theme cannot have the palette key, because it is auto generated by the CssVarsProvider.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update my branch with this change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please document this, it's easy to be overlooked

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added.

@danilo-leal danilo-leal added the package: system Specific to @mui/system label Feb 21, 2022
@siriwatknp siriwatknp merged commit cba5151 into mui:master Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants