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] Remove createUseThemeProps and RtlProvider API #41255

Open
oliviertassinari opened this issue Feb 23, 2024 · 10 comments
Open

[system] Remove createUseThemeProps and RtlProvider API #41255

oliviertassinari opened this issue Feb 23, 2024 · 10 comments
Assignees
Labels
package: system Specific to @mui/system

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 23, 2024

Summary

From #40648 (comment)

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 #41255

@oliviertassinari oliviertassinari added package: system Specific to @mui/system status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 23, 2024
@brijeshb42
Copy link
Contributor

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
Copy link
Member Author

oliviertassinari commented Feb 23, 2024

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
Copy link
Member

siriwatknp commented Feb 28, 2024

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
Copy link
Member Author

oliviertassinari commented Feb 28, 2024

how do you support dynamic runtime styles?

@siriwatknp Property assignment: #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.

@danilo-leal danilo-leal removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Mar 5, 2024
@oliviertassinari
Copy link
Member Author

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

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 23, 2024

I think the same applies to the new <RtlProvider> API #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

@oliviertassinari oliviertassinari changed the title [system] Remove createUseThemeProps API [system] Remove createUseThemeProps and RtlProvider API Mar 23, 2024
@siriwatknp
Copy link
Member

siriwatknp commented Mar 26, 2024

I think the same applies to the new <RtlProvider> API #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
Copy link
Contributor

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
Copy link
Member Author

oliviertassinari commented Apr 1, 2024

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
Copy link
Contributor

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

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
Status: Backlog
Development

No branches or pull requests

4 participants