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

[zero] Add useThemeProps processor #40648

Merged
merged 12 commits into from
Jan 24, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jan 17, 2024

Motivation

Currently, all components are client components and will never be server components because they will always access to the theme context to look for default props (via system useThemeProps).

This PR aims to remove the usage of React context to make some components RSC compatible while having backward compatible with emotion.

How

  1. Lift the current useThemeProps inside a component to the outer scope so that zero-runtime can transform the code by creating an adapter function called createUseThemeProps.

    // /mui-material/src/Badge/Badge.js
    - import useThemeProps from '../styles/useThemeProps';
    + import { createUseThemeProps } from '../zero-useThemeProps';
    + const useThemeProps = createUseThemeProps('MuiBadge');
    
       const Badge = React.forwardRef(function Badge(inProps, ref){
       …
  2. At build-time, zero-runtime will replace the createUseThemProps argument with the defaultProps object defined in the theme config that passed to zero runtime.

    // e.g. next.config.js
    module.exports = withZeroPlugin(nextConfig, {
      theme: extendTheme({
        components: {
          MuiBadge: {
            defaultProps: {
              color: 'error',
            }
          }
        }
      })
    });

    After the transformation, the Badge will be:

    // /mui-material/src/Badge/Badge.js
    - const useThemeProps = createUseThemeProps('MuiBadge');
    + const useThemeProps = createUseThemeProps({ color: 'error' });
  3. At runtime, the createUseThemeProps from zero runtime will merge props with the same logic as in system's useThemeProps.

Those components that haven't used createUseThemeProps, will continue to use React context as is.

In summary, we remove the React context usage by providing the defaultProps directly to components at build-time.

Trade-off

Some components, e.g. Select, allow developers to provide a React component for replacing icons. This is possible in React context approach but I won't work with zero-runtime because it is not serializable. cc @brijeshb42


@mui-bot
Copy link

mui-bot commented Jan 17, 2024

Netlify deploy preview

https://deploy-preview-40648--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against b9d0d9d

@danilo-leal danilo-leal added package: pigment-css Specific to @pigment-css/* proof of concept Studying and/or experimenting with a to be validated approach labels Jan 17, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 18, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 19, 2024
@siriwatknp siriwatknp marked this pull request as ready for review January 19, 2024 05:18
@siriwatknp siriwatknp changed the title [POC][zero] Add useThemeProps processor [zero] Add useThemeProps processor Jan 22, 2024
packages/mui-material/src/Badge/Badge.js Outdated Show resolved Hide resolved
packages/zero-runtime/package.json Outdated Show resolved Hide resolved
packages/zero-runtime/src/processors/useThemeProps.ts Outdated Show resolved Hide resolved
packages/zero-runtime/src/processors/useThemeProps.ts Outdated Show resolved Hide resolved
packages/zero-unplugin/src/index.ts Outdated Show resolved Hide resolved
@siriwatknp siriwatknp enabled auto-merge (squash) January 24, 2024 17:00
@siriwatknp siriwatknp merged commit 2d39985 into mui:master Jan 24, 2024
18 checks passed
@siriwatknp siriwatknp removed the proof of concept Studying and/or experimenting with a to be validated approach label Jan 25, 2024
@oliviertassinari oliviertassinari added the package: system Specific to @mui/system label Feb 8, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 23, 2024

This PR aims to remove the usage of React context to make some components RSC compatible while having backward compatible with emotion.

What's the use case to have RSC compatible components? We use the Badge in the PR, but I guess it's not the objective (I don't see why a developer would want this with his component, there would be no state change animations, the onClick listener wouldn't works, etc.), but I guess the Badge is meant as a supplement for layout components. For instance, it makes a lot of sense to me with a <Container> with a different default layout mode propagated with the context or for a static content: mui/mui-x#12180).

Now, I don't think this change is needed, we can have context (maybe with nesting support but not sure) emotion-js/emotion#2978 (comment).

Trade-off

But them, how can we support theme nesting? I don't see this mentioned, but I think it matters. For example, how is the documentation of Material UI supposed to be able to show components in their default form while the docs has a MUI branded theme?

At first sight, I would recommend:

  • We revert this PR.
  • We introduce a server side theme, to live in the server bundle, alongside the a client bundle theme. We need this anyway to get RSC support with Emotion.
  • We update the theme provider and theme customer to support both theme locations.

This way, we get:

  • No theme propagation feature regression on the client bundle, theme nesting continues to work. On the server bundle, the theme nesting mighty not work, it depends on how React calls the cache API, but I wouldn't expect it to be achievable. Maybe one-day if React introduce a true context RSC API.
  • The foundations for Emotion with RSC.

Opportunity moved to #43443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: pigment-css Specific to @pigment-css/* package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants