-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Conversation
@@ -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']) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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']
})
There was a problem hiding this comment.
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 -
- In the Material docs for configuring Pigment, ask users to add
@mui/material-pigment-css
to thetransformLibraries
option. This would also be applicable for other 3rd party libraries built on Pigment. - Or, make this option built-in since we control both the libraries.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
863e795
to
7000b90
Compare
@@ -213,18 +231,26 @@ export const plugin = createUnplugin<PigmentOptions, true>((options) => { | |||
themeArgs: { | |||
theme, | |||
}, | |||
packageMap: transformLibraries.reduce( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packageMap: transformLibraries.reduce( | |
packageMap: finalTransformLibraries.reduce( |
c124c70
to
ebe4219
Compare
ebe4219
to
747b907
Compare
For ex, this change will allow the plugins to process theme/styles.css import from
@mui/material-pigment-css