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

[pigment] Exception when importing xxxClasses from @mui/material barrel index #43722

Open
abriginets opened this issue Sep 12, 2024 · 7 comments
Assignees
Labels
package: pigment-css Specific to @pigment-css/* regression A bug, but worse v6.x migration

Comments

@abriginets
Copy link
Contributor

abriginets commented Sep 12, 2024

Steps to reproduce

Link to live example: https://codesandbox.io/p/devbox/lingering-shape-s9cvl7?file=%2Fsrc%2Fapp%2Fpage.tsx

Steps:

  1. Use styled from @mui/material-pigment-css
  2. Try styling component that can be styled with imported MUI classes
import { styled } from "@mui/material-pigment-css";
import { Button, buttonClasses } from "@mui/material";

export const StyledButton = styled(Button)(({ theme }) => ({
  [theme.breakpoints.up("md")]: {
    [`& > .${buttonClasses.startIcon}`]: {
      display: "none",
    },
  },
}));

Current behavior

NextJS fails to build and re-throws an exception from pigment

 ⨯ unhandledRejection: TypeError: /project/sandbox/src/app/page.tsx: Cannot read properties of undefined (reading 'startIcon')
    at /project/sandbox/src/app/page.tsx:16:153
    at StyledProcessor.processCss (/project/sandbox/node_modules/@pigment-css/react/processors/styled.js:373:59)
    at StyledProcessor.processStyle (/project/sandbox/node_modules/@pigment-css/react/processors/styled.js:306:31)
    at /project/sandbox/node_modules/@pigment-css/react/processors/styled.js:209:12
    at Array.forEach (<anonymous>)
    at StyledProcessor.build (/project/sandbox/node_modules/@pigment-css/react/processors/styled.js:208:20)
    at /project/sandbox/node_modules/@wyw-in-js/transform/lib/plugins/collector.js:26:17
    at /project/sandbox/node_modules/@wyw-in-js/transform/lib/utils/getTagProcessor.js:384:5
    at Array.forEach (<anonymous>)
    at applyProcessors (/project/sandbox/node_modules/@wyw-in-js/transform/lib/utils/getTagProcessor.js:375:10)

Expected behavior

Pigment should be able to successfully build styles using class names just like it does with colors for instance

import { red } from "@mui/material/colors";

export const StyledButton = styled(Button)(({ theme }) => ({
  color: red[500], // no error
}));

Context

Migrating to V6 + pigment

Your environment

npx @mui/envinfo
System:
    OS: Linux 6.1 Ubuntu 20.04.6 LTS (Focal Fossa)
  Binaries:
    Node: 20.12.1 - /home/codespace/nvm/current/bin/node
    npm: 10.5.0 - /home/codespace/nvm/current/bin/npm
    pnpm: 8.15.6 - /home/codespace/nvm/current/bin/pnpm
  Browsers:
    Chrome: Not Found
  npmPackages:
    @emotion/react:  11.13.3 
    @emotion/styled:  11.13.0 
    @mui/core-downloads-tracker:  6.0.0 
    @mui/icons-material: 6.1.0 => 6.1.0 
    @mui/material: latest => 6.0.0 
    @mui/material-pigment-css: latest => 6.0.0 
    @mui/private-theming:  6.0.0 
    @mui/styled-engine:  6.0.0 
    @mui/system:  6.0.0 
    @mui/types:  7.2.16 
    @mui/utils:  6.0.0 
    @pigment-css/nextjs-plugin: latest => 0.0.20 
    @pigment-css/react:  0.0.20 
    @pigment-css/unplugin:  0.0.20 
    @types/react: latest => 18.3.4 
    react: latest => 18.3.1 
    react-dom: latest => 18.3.1 
    typescript: latest => 5.5.4
__

Search keywords: pigment, pigment-css, buttonClasses

@abriginets abriginets added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 12, 2024
@zannager zannager added the package: pigment-css Specific to @pigment-css/* label Sep 12, 2024
@brijeshb42
Copy link
Contributor

Could you try importing from @mui/material/Button ?

@abriginets
Copy link
Contributor Author

Hey, @brijeshb42. No errors when importing it like this.

import Button, { buttonClasses } from "@mui/material/Button";

Although I would prefer not to import default and named exports at the same time as well as importing each component from separate path. Is there a solution where importing everything from @mui/material is acceptable when using pigment?

@brijeshb42
Copy link
Contributor

I just wanted to check the behaviour. Ideally, we strive to support direct module level imports as well besides the path imports, but you might encounter slower build times due to that.
Thanks for reporting though and I'll look into the actual issue.

@brijeshb42
Copy link
Contributor

At this point, I'd recommend using path imports instead of direct package import as far as Pigment CSS is concerned since that'll also affect the overall build time.

Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

@abriginets How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

@github-actions github-actions bot removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 16, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 16, 2024

@brijeshb42 Is this Material UI specific or does it impact any dependencies built on Pigment CSS? That is then published on npm?

If it's the latter, I think we need an open issue in https://github.com/mui/pigment-css to keep track of this. It's no longer about Material UI but about:

  • Build performance
  • DX

In any case, it looks like we need an issue as it looks like there is more to do even. Shouldn't we:

  • throwing a clear error that explains how to fix this. We can't expect developers to figure this out by themselves. Only a fraction of the people will find this issue. "Cannot read properties of undefined (reading 'startIcon')" is far from a great DX.
  • maybe documenting this in the migration guide: https://mui.com/material-ui/migration/migrating-to-pigment-css/

@oliviertassinari oliviertassinari changed the title [pigment] Exception when using exported class names from MUI [pigment] Exception when using exported class names from @mui/material barrel index Oct 16, 2024
@oliviertassinari oliviertassinari changed the title [pigment] Exception when using exported class names from @mui/material barrel index [pigment] Exception when importing xxxClasses from @mui/material barrel index Oct 16, 2024
@abriginets
Copy link
Contributor Author

So, this is expected and it's just a matter of missing documentation, right? I would like to proceed with the migration to Pigment but I'm not sure if I should spend time on changing imports across the entire codebase. It could be very annoying to track sometimes since VSCode's autocomplete always suggest using top-level imports

image

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/* regression A bug, but worse v6.x migration
Projects
Status: Done
Development

No branches or pull requests

5 participants