mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.96k stars 32.27k forks source link

[system] Remove createUseThemeProps, RtlProvider, DefaultPropsProvider, useDefaultProps API #43443

Open oliviertassinari opened 8 months ago

oliviertassinari commented 8 months ago

Summary

From https://github.com/mui/material-ui/pull/40648#issuecomment-1961591574

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: https://github.com/mui/mui-x/issues/12180).

Now, I don't think this change is needed, we can have context (maybe with nesting support but not sure) https://github.com/emotion-js/emotion/issues/2978#issuecomment-1935131675.

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:

This way, we get:

Opportunity moved to https://github.com/mui/material-ui/issues/43443

Search keywords:

Search keywords:

brijeshb42 commented 8 months ago

I can share a working POC later (I have a minimal demo working). But nested themes will work like this -

  1. You provide a single theme object through the bundler config. This is used as a reference to generate and inject css variables.
  2. If you want to override parts of the app, you can use a custom ThemeProvider (which is not based on context

Suppose you have this as the main theme -

{
  palette: {
    background: "0 0% 100%",
    foreground: "240 10% 3.9%",
    primary: "240 5.9% 10%",
    border: "240 5.9% 90%",
  },
}

And this is how you'll override the global values to scoped areas on the page

<ThemeProvider theme={{
  palette: {
    border: "240 59.7% 56.9%",
  },
}}>
  <div>Your demo</div>
</ThemeProvider>

It'll generate inline styles of css variables that have overriden values that are provided, like this -

Screenshot 2024-02-24 at 12 54 45 AM

Any component inside this ThemeProvider will use css variables with local values for styling. So ThemeProvider is not actually a context in this case, it's just a div that does not have it's own layout. Note that this only handles theme token but not other overrides, like component defaultProps. For those, we can still have ThemeProvider with context similar to what we have right now.

oliviertassinari commented 8 months ago

Off-topic (style theme nesting isn't something I was thinking of covering in this issue)

As far as I understand, nested theming for styles should be supported by using CSS variables. It's the <CssVarProvider> component's responsibility to get this to work (which IMHO would be better called ThemeProvider). The zero-runtime should only hard code theme values that aren't CSS variables, or use CSS variables when they are.

It'll generate inline styles of css variables that have overriden values that are provided, like this

We should try to have no inline style in the framework for strict CSP policies. If we can have this in a class name, this would be better.

siriwatknp commented 8 months ago

We introduce a server side theme, to live in the server bundle, alongside the a client bundle theme

I don't see a picture of it. Is server-side theme is the one you define in the config (e.g. next.config.js)?

We should try to have no inline style in the framework for strict CSP policies. If we can have this in a class name, this would be better.

Then, how do you support dynamic runtime styles? Inline style seems to be the only way to connect JS with CSS statically.

// css
.hash {
  opacity: var(--hashed-var);
}

// js
<div style={{ '--hashed-var': opacity }}>
oliviertassinari commented 8 months ago

how do you support dynamic runtime styles?

@siriwatknp Property assignment: mui/material-ui#19938

Is server-side theme is the one you define in the config

For Emotion RSC and runtime theme dependencies, you define it once in a RSC and once in a React Client Component. Actually because of this: https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#supported-pattern-passing-server-components-to-client-components-as-props we can likely have a single component that does both client and server context assignment under the hood.

For zero-runtime RSC, yeah, you also need to define it in the plugin that does the transpilation.

oliviertassinari commented 8 months ago

They use the React.cache for the server context: https://yak.js.org/how-does-it-work

oliviertassinari commented 7 months ago

I think the same applies to the new <RtlProvider> API mui/material-ui#41241. With the information I have access to, I don't understand why we are doing this. It feels like we should revert this PR to change the approach.

React.cache() seems like a better solution, keeping the ThemeProvider notion:

Practical example: https://github.com/jantimon/next-yak/blob/main/packages/next-yak/runtime/context/index.server.tsx#L14

siriwatknp commented 7 months ago

I think the same applies to the new <RtlProvider> API mui/material-ui#41241. With the information I have access to, I don't understand why we are doing this. It feels like we should revert this PR to change the approach.

React.cache() seems like a better solution, keeping the ThemeProvider notion:

  • the less we rely on transpilation, the more developers can add console.log, the less magic the better will make the migration the smoothest
  • this also mean a smoother migration and backward compatibility for projects that depend on Material UI.
  • there are use cases for using the theme in JavaScript render functions, outside of CSS, e.g. inline style, so if we can support nesting, we can't rely on transpilation.

Practical example: https://github.com/jantimon/next-yak/blob/main/packages/next-yak/runtime/context/index.server.tsx#L14

I think we can work on this until React.cache becomes stable, right? It's still in canary. Yak can do it because its purpose is for Next.js

brijeshb42 commented 7 months ago

From what I see in the example you posted, it's still using a client component with React context. And as @siriwatknp said, we can't experiment or try cache API till it's available through a stable release.

oliviertassinari commented 7 months ago

From what I see in the example you posted, it's still using a client component with React context

In their case, they use the entry point to split: https://github.com/jantimon/next-yak/blob/47d2ed61429335cb87caf68a2776aa582bc7e21c/packages/next-yak/package.json#L28 (I was using a try catch in my POC)

we can't experiment or try cache API till it's available through a stable release.

Likely something to test with other bundlers: https://twitter.com/leeerob/status/1772651496544317804, my understanding is that RSC is experimental as the cache API.

brijeshb42 commented 7 months ago

True. But their server entry point then imports a client file, ie, YakThemeProvider

oliviertassinari commented 4 months ago

I'm extending the scope of this issue to <DefaultPropsProvider>, https://github.com/mui/material-ui/issues/42635. This feature means that we don't support default prop value configuration for RSC. If we solve https://github.com/mui/material-ui/pull/42476#discussion_r1632325822, we would be able to support it.

Now, the only concern for me is the support of theme nesting on RSC and client-side components. This looks more important to support theme nesting than the default prop on RSC. The React.cache() or window.__theme solution doesn't support theme nesting. As I understand it, today, Pigment CSS doesn't support it either. So I would only explore how to support theme nesting before removing all of these APIs (createUseThemeProps, RtlProvider, and DefaultPropsProvider).

oliviertassinari commented 1 month ago

A quick POC: https://github.com/oliviertassinari/test-theme it seems to work, so it seems that we can move forward with removing all of those APIs:

as well as fixing https://github.com/emotion-js/emotion/issues/2978.