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

[plugin] Allow runtime libraries to have their own theme and styles css path #165

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

brijeshb42
Copy link
Contributor

@brijeshb42 brijeshb42 commented Jul 2, 2024

For ex, this change will allow the plugins to process theme/styles.css import from @mui/material-pigment-css

@@ -115,7 +115,7 @@ export const plugin = createUnplugin<PigmentOptions, true>((options) => {
...rest
} = options;
const finalTransformLibraries = transformLibraries
.concat(process.env.RUNTIME_PACKAGE_NAME as string)
.concat([process.env.RUNTIME_PACKAGE_NAME as string, '@mui/material-pigment-css'])
Copy link
Member

Choose a reason for hiding this comment

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

Is this a quick solution for @mui/material-pigment-css? Because if someone wants to build an adapter on top of Pigment CSS, they have to add it to the list here.

Copy link
Member

Choose a reason for hiding this comment

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

Nothing to change at this point but it's something to think about for Pigment CSS v1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not only for material-pigment-css.
It's for any other similar libraries. They'll just have to ask users to add their lib to the transformLibraries list.

Copy link
Member

Choose a reason for hiding this comment

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

Then why do we need to add @mui/material-pigment-css here? It should be inside a config @mui/material-pigment-css.

Something like:

import { withPigment } from '@pigment-css/nextjs-plugin';
import { presets } from '@mui/material-pigment-css';

withPigment({
  theme,
  …presets, // transformLibraries: ['@mui/material-pigment-css']
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need any config in @mui/material-pigment-css itself. There are two ways to handle this library through Pigment -

  1. In the Material docs for configuring Pigment, ask users to add @mui/material-pigment-css to the transformLibraries option. This would also be applicable for other 3rd party libraries built on Pigment.
  2. Or, make this option built-in since we control both the libraries.

Copy link
Member

Choose a reason for hiding this comment

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

There is an overrideContext that we might need to add as a workaround for the toString issue.

This is the config that works for Material UI x Pigment:

export default withPigment(nextConfig, {
  theme: customTheme,
  transformLibraries: ["@mui/material"],
  overrideContext: (context) => {
    if (!context.$RefreshSig$) {
      context.$RefreshSig$ = outerNoop;
    }
    return {
      ...context,
      require: (id) => {
        if (id === "@mui/styled-engine" || id === "@mui/styled-engine-sc") {
          return {
            __esModule: true,
            default: () => () => () => null,
            internal_processStyles: () => {},
            keyframes: () => "",
            css: () => "",
          };
        }
        return context.require(id);
      },
    };
  },
});

Where do you think the overrideContext should be?

Copy link
Contributor Author

@brijeshb42 brijeshb42 Jul 2, 2024

Choose a reason for hiding this comment

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

I don't want to have packages like @mui/material-pigment-vite-plugin/nextjs-plugin. So it'd be better to have this hardcoded in Pigment itself. I'll added this in a new commit.

@brijeshb42 brijeshb42 force-pushed the dynamic-theme-path branch 4 times, most recently from 863e795 to 7000b90 Compare July 8, 2024 07:16
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 9, 2024
@@ -213,18 +231,26 @@ export const plugin = createUnplugin<PigmentOptions, true>((options) => {
themeArgs: {
theme,
},
packageMap: transformLibraries.reduce(
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
packageMap: transformLibraries.reduce(
packageMap: finalTransformLibraries.reduce(

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 9, 2024
@brijeshb42 brijeshb42 merged commit 86ee5ee into mui:master Jul 9, 2024
11 of 12 checks passed
@brijeshb42 brijeshb42 deleted the dynamic-theme-path branch July 9, 2024 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants