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

[bug] invalid typings for createTheme #170

Closed
o-alexandrov opened this issue Jul 1, 2024 · 16 comments · Fixed by #169
Closed

[bug] invalid typings for createTheme #170

o-alexandrov opened this issue Jul 1, 2024 · 16 comments · Fixed by #169
Assignees
Labels
bug 🐛 Something doesn't work vite

Comments

@o-alexandrov
Copy link

o-alexandrov commented Jul 1, 2024

Steps to reproduce

Link to live example

Steps:

  1. clone repo & install deps npm ci
  2. see the theme src/theme.ts
    • incorrect typings that allow you to pass extended prop and doesn't enforce you to migrate to the new way of declaring variants. (see screenshot).
Screenshot 2024-07-23 at 8 49 56 AM

Current behavior

extendTheme doesn't change the styles of the mui.Fab

Expected behavior

extendTheme affects mui.Fab

Context

Your environment

npx @mui/envinfo
    "@pigment-css/react": "0.0.16",
    "@pigment-css/vite-plugin": "0.0.16",

  System:
    OS: macOS 14.5
  Binaries:
    Node: 22.1.0 - /opt/homebrew/bin/node
    npm: 10.7.0 - /opt/homebrew/bin/npm
    pnpm: 9.0.2 - /opt/homebrew/bin/pnpm
  Browsers:
    Chrome: 126.0.6478.127
    Edge: 112.0.1722.39
    Safari: 17.5
  npmPackages:
    @emotion/react:  11.11.4 
    @emotion/styled:  11.11.5 
    @mui/base:  5.0.0-beta.51 
    @mui/core-downloads-tracker:  6.0.0-dev.240424162023-9968b4889d 
    @mui/material: 6.0.0-alpha.13 => 6.0.0-alpha.13 
    @mui/private-theming:  6.0.0-dev.20240529-082515-213b5e33ab 
    @mui/styled-engine:  6.0.0-dev.20240529-082515-213b5e33ab 
    @mui/system:  6.0.0-dev.240424162023-9968b4889d 
    @mui/types:  7.2.14 
    @mui/utils:  6.0.0-dev.20240529-082515-213b5e33ab 
    @types/react: 18.3.3 => 18.3.3 
    react: 18.2.0 => 18.2.0 
    react-dom: 18.2.0 => 18.2.0 
    typescript: 5.5.2 => 5.5.2 

Search keywords: pigment, extendTheme

Search keywords:

@o-alexandrov o-alexandrov added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 1, 2024
@o-alexandrov
Copy link
Author

Thank you everyone for your great progress on PigmentCSS.
@siriwatknp pinging you as the repro example might be useful

@aarongarciah
Copy link
Member

@o-alexandrov can you provide a link to a live example? e.g. Codesandbox

@aarongarciah aarongarciah added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 1, 2024
@o-alexandrov
Copy link
Author

@aarongarciah please refer to the attached GitHub repo at the top of the description

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Jul 1, 2024
@aarongarciah
Copy link
Member

aarongarciah commented Jul 1, 2024

@o-alexandrov I saw it, but the link text says "Link to live example", but that's not the case. Whenever possible, we ask for a live example, as stated in the steps when opening a bug report:

Screenshot 2024-07-01 at 15 42 04

Is there a reason why you can't provide a live example?

@aarongarciah aarongarciah added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 1, 2024
@DavidGregory084
Copy link

https://stackblitz.com/github/o-alexandrov/mui-pigment-extend-theme?file=index.html

@aarongarciah
Copy link
Member

@siriwatknp pinging you in case you can have a look.

@aarongarciah aarongarciah added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Jul 1, 2024
@siriwatknp siriwatknp added the bug 🐛 Something doesn't work label Jul 8, 2024
@siriwatknp siriwatknp transferred this issue from mui/material-ui Jul 8, 2024
@siriwatknp siriwatknp added vite and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 8, 2024
@siriwatknp
Copy link
Member

Vite has deps optimization that does not work with Pigment CSS

This is the vite config that work so far:

import { defineConfig } from 'vite'
import react from '@vitejs/plugin-react'
import { pigment } from '@pigment-css/vite-plugin'
import { extendTheme, stringifyTheme } from '@mui/material/styles'

const theme = extendTheme();
// @ts-expect-error ignore
theme.toRuntimeSource = stringifyTheme;

// https://vitejs.dev/config/
export default defineConfig({
  optimizeDeps: {
    include: [
      "prop-types",
      "react-is",
      "hoist-non-react-statics",
      "react",
      "react-dom",
      '@emotion/react',
      '@emotion/styled',
    ],
    exclude: ["@mui/material"],
  },
  plugins: [
    react(),
    pigment({
      theme,
      transformLibraries: ["@pigment-css/react", "@mui/material"],
    }),
  ],
})

Note: this is a workaround to get the app working, need mui/material-ui#169 to be released.

@o-alexandrov
Copy link
Author

o-alexandrov commented Jul 21, 2024

@siriwatknp @aarongarciah
Please reopen the issue, as it still doesn't work using the latest versions of pigment and @mui/material and following exactly the Migration Guide.

Updated the repro. Related issue #180

There are issues:

  1. A developer still must pass @mui/material to transformLibraries
    • w/o this option, root styleOverrides from theme.ts (in the provided repro) file are not applied
  2. extended Fab styles overrides are not applied even if you pass transformLibraries
  3. vite fails to start for the first cold start
    • to test, run rm -rf node_modules/.vite and then npm run start

@aarongarciah
Copy link
Member

@o-alexandrov

  1. A developer still must pass @mui/material to transformLibraries

True, I just reproduced it.

  1. extended Fab styles overrides are not applied even if you pass transformLibraries

This is not a bug. extended is not a slot, it's a variant. The only slot in Fab is root. You can override styles based on props (like variant) like this:

 const themeRaw = extendTheme({
   components: {
     MuiFab: {
       styleOverrides: {
-        extended: {
-          borderRadius: 12,
-        },
         root: {
           background: `red !important`,
+          variants:[{
+            props: { variant: 'extended' },
+            style: { borderRadius: 12 }
+          }]
         },
       },
     },
   },
 });
  1. vite fails to start for the first cold start

True, I just reproduced it.

@siriwatknp can you have a look?

@aarongarciah aarongarciah reopened this Jul 23, 2024
@o-alexandrov
Copy link
Author

o-alexandrov commented Jul 23, 2024

@aarongarciah
cc @siriwatknp
Thank you for confirming and describing point 2 in more detail.
The issue in point 2 is: incorrect typings then, that allow you to pass extended prop and doesn't enforce you to migrate to the new way of declaring variants. (see screenshot).

  • updated repro to showcase this in theme.ts
Screenshot 2024-07-23 at 8 49 56 AM

@royporter7
Copy link

I'm beginning to get this exact error:
Details posted over on #211

@tripolskypetr
Copy link

How do I exclude libraries which make building stuck until you make this production ready?

@marvinruder
Copy link

extended is not a slot, it's a variant. The only slot in Fab is root. You can override styles based on props (like variant) like this

@aarongarciah I have a custom theme with lots of overrides for both variants and slots in many styleOverrides objects like:

createTheme({
  components: {
    MuiPaper: {
      styleOverrides: {
        elevation: ({ theme }) => ({
          boxShadow: "0px 9px 16px rgb(176 176 176 / 18%), 0px 2px 2px rgb(176 176 176 / 32%)",
        }),
      }
    }
  }
})

At first glance, it seems impossible to distinguish variant overrides from slot overrides, so it will be very time-consuming to figure out one-by-one which override to migrate to the variants array notation.

Is there a way to handle this better? Are there any plans to make styleOverrides for variants working in Pigment CSS also? Perhaps a codemod could assist with this kind of migration? Apparently, the theme-v6 codemod only moves variants from outside the styleOverrides property inside the root override, but does not cover the case shown above. TypeScript also does not complain about unknown properties.

@o-alexandrov o-alexandrov changed the title [bug] extendTheme doesn't work w/ pigment [bug] invalid typings for createTheme Oct 15, 2024
@o-alexandrov
Copy link
Author

o-alexandrov commented Oct 15, 2024

@aarongarciah @siriwatknp
This issue is probably hard to follow due to the many changes made to PigmentCSS.

I created a new repro example based on your vite example app.

The current issue here is based on typings, so I cleaned up the description:

  • in this newly created one
    • since the issue is material-ui related, I opened it in that repo instead

@marvinruder apologies for closing the issue.
Could you please move your comment to the new issue?

@o-alexandrov o-alexandrov closed this as not planned Won't fix, can't repro, duplicate, stale Oct 15, 2024
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

@o-alexandrov 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.

@aarongarciah
Copy link
Member

Thanks @o-alexandrov! We'll continue in the new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work vite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants