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

[pigment-css] Add stringifyTheme for Pigment CSS integration #42476

Merged
merged 20 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/mui-material/src/styles/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ export { default as withTheme } from './withTheme';
export * from './CssVarsProvider';

export { default as extendTheme } from './extendTheme';

export * from './stringifyTheme';

export type {
ColorSchemeOverrides,
SupportedColorScheme,
Expand Down
4 changes: 3 additions & 1 deletion packages/mui-material/src/styles/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export { default as unstable_createMuiStrictModeTheme } from './createMuiStrictM
export { default as createStyles } from './createStyles';
export { getUnit as unstable_getUnit, toUnitless as unstable_toUnitless } from './cssUtils';
export { default as responsiveFontSizes } from './responsiveFontSizes';
export { duration, easing } from './createTransitions';
export { default as createTransitions, duration, easing } from './createTransitions';
export { default as useTheme } from './useTheme';
export { default as useThemeProps } from './useThemeProps';
export { default as styled } from './styled';
Expand All @@ -50,6 +50,8 @@ export { default as experimental_extendTheme } from './experimental_extendTheme'
export { default as getOverlayAlpha } from './getOverlayAlpha';
export { default as shouldSkipGeneratingVar } from './shouldSkipGeneratingVar';

export * from './stringifyTheme';

// Private methods for creating parts of the theme
export { default as private_createTypography } from './createTypography';
export { default as private_createMixins } from './createMixins';
Expand Down
127 changes: 127 additions & 0 deletions packages/mui-material/src/styles/stringifyTheme.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
import { expect } from 'chai';
import { stringifyTheme } from './stringifyTheme';
import extendTheme from './extendTheme';

describe('StringifyTheme', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe('StringifyTheme', () => {
describe('stringifyTheme', () => {

it('should serialize the theme', () => {
const theme = extendTheme();
const result = stringifyTheme({
breakpoints: theme.breakpoints,
transitions: theme.transitions,
});
expect(result).to.equal(`import createBreakpoints from '@mui/system/createBreakpoints';
import { createTransitions } from '@mui/material/styles';

const theme = {
"breakpoints": {
"keys": [
"xs",
"sm",
"md",
"lg",
"xl"
],
"values": {
"xs": 0,
"sm": 600,
"md": 900,
"lg": 1200,
"xl": 1536
},
"unit": "px"
},
"transitions": {
"easing": {
"easeInOut": "cubic-bezier(0.4, 0, 0.2, 1)",
"easeOut": "cubic-bezier(0.0, 0, 0.2, 1)",
"easeIn": "cubic-bezier(0.4, 0, 1, 1)",
"sharp": "cubic-bezier(0.4, 0, 0.6, 1)"
},
"duration": {
"shortest": 150,
"shorter": 200,
"short": 250,
"standard": 300,
"complex": 375,
"enteringScreen": 225,
"leavingScreen": 195
}
}
};

theme.breakpoints = createBreakpoints(theme.breakpoints || {});
theme.transitions = createTransitions(theme.transitions || {});

export default theme;`);

// test that non-seriazable values still exist in the original theme
expect(typeof theme.generateStyleSheets).to.equal('function');
});

it('should serialize the custom theme', () => {
const theme = extendTheme({
breakpoints: {
values: {
mobile: 0,
tablet: 640,
laptop: 1024,
desktop: 1280,
} as any,
},
transitions: {
duration: {
standard: 432,
},
},
});
const result = stringifyTheme({
breakpoints: theme.breakpoints,
transitions: theme.transitions,
});
expect(result).to.equal(`import createBreakpoints from '@mui/system/createBreakpoints';
import { createTransitions } from '@mui/material/styles';

const theme = {
"breakpoints": {
"keys": [
"mobile",
"tablet",
"laptop",
"desktop"
],
"values": {
"mobile": 0,
"tablet": 640,
"laptop": 1024,
"desktop": 1280
},
"unit": "px"
},
"transitions": {
"duration": {
"standard": 432,
"shortest": 150,
"shorter": 200,
"short": 250,
"complex": 375,
"enteringScreen": 225,
"leavingScreen": 195
},
"easing": {
"easeInOut": "cubic-bezier(0.4, 0, 0.2, 1)",
"easeOut": "cubic-bezier(0.0, 0, 0.2, 1)",
"easeIn": "cubic-bezier(0.4, 0, 1, 1)",
"sharp": "cubic-bezier(0.4, 0, 0.6, 1)"
}
}
};

theme.breakpoints = createBreakpoints(theme.breakpoints || {});
theme.transitions = createTransitions(theme.transitions || {});

export default theme;`);

// test that non-seriazable values still exist in the original theme
expect(typeof theme.generateStyleSheets).to.equal('function');
});
});
62 changes: 62 additions & 0 deletions packages/mui-material/src/styles/stringifyTheme.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/* eslint-disable import/prefer-default-export */
import { isPlainObject } from '@mui/utils/deepmerge';

function isSerializable(val: any) {
return (
isPlainObject(val) ||
typeof val === 'undefined' ||
typeof val === 'string' ||
typeof val === 'boolean' ||
typeof val === 'number' ||
Array.isArray(val)
);
}

/**
* `baseTheme` usually comes from `createTheme` or `extendTheme`.
*
* This function is intended to be used with zero-runtime CSS-in-JS like Pigment CSS
* For example, in a Next.js project:
*
* ```js
* // next.config.js
* const { extendTheme } = require('@mui/material/styles');
*
* const theme = extendTheme();
* // `.toRuntime` is Pigment CSS specific to create a theme that is available at runtime.
siriwatknp marked this conversation as resolved.
Show resolved Hide resolved
* theme.toRuntime = () => stringifyTheme(theme);
siriwatknp marked this conversation as resolved.
Show resolved Hide resolved
siriwatknp marked this conversation as resolved.
Show resolved Hide resolved
*
* module.exports = withPigment({
* theme,
* });
* ```
*/
export function stringifyTheme(baseTheme: Record<string, any> = {}) {
const serializableTheme: Record<string, any> = { ...baseTheme };

function serializeTheme(object: Record<string, any>) {
const array = Object.entries(object);
// eslint-disable-next-line no-plusplus
for (let index = 0; index < array.length; index++) {
const [key, value] = array[index];
if (!isSerializable(value) || key.startsWith('unstable_')) {
delete object[key];
} else if (isPlainObject(value)) {
object[key] = { ...value };
serializeTheme(object[key]);
}
}
}
Comment on lines +37 to +49
Copy link
Member

@oliviertassinari oliviertassinari Jun 8, 2024

Choose a reason for hiding this comment

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

I think we need to remove all the CSS variables values: colors, typography, etc., so it breaks at runtime if developers try to access them. For example theme.color.primary.main would be wrong as soon as the theme changes. So if it's impossible to access, we would avoid a footgun.

Then for these dynamic values (have CSS variables), solve it with: https://github.com/mui/pigment-css/issues/129.

But honestly, the theme replacement at build-time feels wrong in the first place. I don't think we should invest more time into it. From what I understand, we are only doing it for RSC, but it feels like the worst technical solution available if there is a working React.cache() alternative: either like yak is doing it, or like this: emotion-js/emotion#2978 (comment).

Copy link
Member Author

@siriwatknp siriwatknp Jun 9, 2024

Choose a reason for hiding this comment

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

But how would you make it work for components that are using useTheme() at runtime, for example, Material UI Tooltip?

At this point, I think we need it (can be an internal API) otherwise v6 will be delayed for at least a month.

From what I understand, we are only doing it for RSC

No, we are doing this because some components are accessing the theme at runtime (those that call useTheme().

but it feels like the worst technical solution available if there is a working React.cache() alternative

Again, I think I mentioned in some issue to you that React.cache is still experimental. The options we have are:

  1. Use wyw-in-js to provide runtime theme (small effort)
  2. Go with your proposal, could be small or large effort because we don't know the edge cases and React.cache is still experimental.

To me, both options give almost the same result, (1) is a bit better because there is no useContext involved compare to option (2). That's why I would go with option (1) and keep the API internal to launch v6 and then when the time is right, switch to option (2) if necessary.

What's your opinion? cc @mnajdova @brijeshb42

Copy link
Member

@oliviertassinari oliviertassinari Jun 9, 2024

Choose a reason for hiding this comment

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

React.cache is still experimental

https://nextjs.org/docs/app/building-your-application/caching#react-cache-function doesn't make it so clear.

But how would you make it work for components that are using useTheme() at runtime, for example, Material UI Tooltip?

The way I see it:

For the server bundle, you have a full duplicate of the theme object, you can import the same one that the plugin uses. This theme is made available with the <CssVarProvider> which injects the theme for the children's server components that were not extracted at build time. The theme is propagated ideally with the React.cache() API because it has no bundler integration needs or a global window value (maybe with some capabilities to have multiple versions of Pigment CSS run in the same app)
Then, the children use useTheme(), but this module must not be flagged as a client bundle, so we remove "use client" to have it as a shared moduie: https://github.com/reactjs/rfcs/blob/main/text/0188-server-components.md#conventions-for-serverclient-components. That would work like this: emotion-js/emotion#2978 (comment)
A theme replacement during the build could be an option but it sounds more work and depends more on the bundler.

For the client bundle, you have the whole theme available, you can import the same one that the plugin uses. We make it propagate in the application with the React Context API, from <CssVarProvider> who injects it with a child. I mean, the CssVarProvider stays a server component but uses a client component inside himself using https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#supported-pattern-passing-server-components-to-client-components-as-props. The challenge there is that for this to work, the server effectively needs to serialize the theme https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#passing-props-from-server-to-client-components-serialization, which breaks all its functions.

One way to solve this is to use Yak a plugin that I assume will somehow make this theme available in the client component (see how they use client hints https://yak.js.org/features#theming to create the theme).

Another, I don't know if it works, is to have both a client and a server component imports that theme, and inject it, so it's never serialized like in https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#passing-props-from-server-to-client-components-serialization.

Personally, I see no need to serialize/stringify a theme with this model.

Copy link
Member Author

Choose a reason for hiding this comment

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


serializeTheme(serializableTheme);

return `import createBreakpoints from '@mui/system/createBreakpoints';
import { createTransitions } from '@mui/material/styles';

const theme = ${JSON.stringify(serializableTheme, null, 2)};

theme.breakpoints = createBreakpoints(theme.breakpoints || {});
theme.transitions = createTransitions(theme.transitions || {});
Copy link
Contributor

Choose a reason for hiding this comment

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

I also found that we'll have to have theme.shadows for Paper. But I see that it uses shadows in non production env. Have you tried running the demo apps in dev mode and see if it works ?

Copy link
Member Author

@siriwatknp siriwatknp Jun 12, 2024

Choose a reason for hiding this comment

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

Yes, it works. The theme.shadows are in serializableTheme in line 56.

Copy link
Member Author

Choose a reason for hiding this comment

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

image

This proves that the runtime theme works!


export default theme;`;
}