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] Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled #38626

Open
2 tasks done
erik-d opened this issue Aug 24, 2023 · 6 comments
Assignees
Labels
bug 🐛 Something doesn't work package: system Specific to @mui/system ready to take Help wanted. Guidance available. There is a high chance the change will be accepted typescript

Comments

@erik-d
Copy link

erik-d commented Aug 24, 2023

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

No error on 5.14.0: https://codesandbox.io/s/issue-38626-5-14-0-q8xyfk

Error on 5.14.1: https://codesandbox.io/s/issue-38626-5-14-1-n7z85w?file=/src/App.tsx

Local repro steps:

  1. npx create-react-app mui-foo-chip --template typescript
  2. npm i @mui/material@5.14.0 @emotion/react @emotion/styled
  3. Replace the src/index.tsx with code below
  4. run ./node_modules/typescript/bin/tsc --noEmit - no typing errors
  5. Install 5.14.1: npm i @mui/material@5.14.1
  6. run ./node_modules/typescript/bin/tsc --noEmit again
// src/index.tsx
import React from 'react';
import ReactDOM from 'react-dom/client';
import { CSSInterpolation } from '@mui/system';
import {
  Chip,
  ChipProps,
  ThemeProvider,
  alpha,
  createTheme,
  styled,
} from '@mui/material';
import App from './App';

export interface FooChipProps extends Omit<ChipProps, 'color'> {
  color?: ChipProps['color'] | 'peace';
}

export const FooChip = styled(
  Chip, {
    name: 'FooChip',
    slot: 'root',
    // Note the `propName is keyof ...` construct is important here since we we'll not
    // propagate the color prop to the underlying Chip and omitted we'd get typing error on the FooChip component
    shouldForwardProp: (propName: string): propName is keyof Omit<ChipProps, 'color'> => propName !== 'color',
    overridesResolver: (props, styles) => [styles.root, styles[props.color || 'default']],
  },
)<FooChipProps>(null);

declare module '@mui/material/styles' {
  export interface Components<Theme = unknown> {
    FooChip?: {
      styleOverrides?: { peace: CSSInterpolation };
    },
  }
}

const fooTheme = createTheme({
  components: {
    FooChip: {
      styleOverrides: {
        peace: {
          backgroundColor: alpha('#2fbcc6', 0.2),
          color: '#209ba4',
        },
      },
    },
  },
});

const root = ReactDOM.createRoot(
  document.getElementById('root') as HTMLElement
);

root.render(
  <React.StrictMode>
    <ThemeProvider theme={fooTheme}>
      <FooChip color="peace" label="A Foo chip" />
    </ThemeProvider>
  </React.StrictMode>
);

Current behavior 😯

With 5.14.1 and later version you'll get the typescript error:

error TS2322: Type '"peace"' is not assignable to type '"default" | "primary" | "secondary" | "error" | "info" | "success" | "warning" | undefined'.

Expected behavior 🤔

I expected no typescript error.

Context 🔦

We want to re-use in this case the Chip component, but it will also have other properties such as emphasised and more states, hence we need to wrap the Chip component and at the same time we don't want to change Mui's own Chip component.

Your environment 🌎

npx @mui/envinfo
npx @mui/envinfo
Need to install the following packages:
  @mui/envinfo@2.0.9
Ok to proceed? (y) 

  System:
    OS: macOS 12.6.8
  Binaries:
    Node: 16.17.0 - ~/.nvm/versions/node/v16.17.0/bin/node
    Yarn: 1.22.19 - ~/.yarn/bin/yarn
    npm: 8.15.0 - ~/.nvm/versions/node/v16.17.0/bin/npm
  Browsers:
    Chrome: 116.0.5845.96
    Edge: Not Found
    Safari: 16.6
  npmPackages:
    @emotion/react: ^11.11.1 => 11.11.1 
    @emotion/styled: ^11.11.0 => 11.11.0 
    @mui/base:  5.0.0-beta.11 
    @mui/core-downloads-tracker:  5.14.5 
    @mui/material: ^5.14.5 => 5.14.5 
    @mui/private-theming:  5.14.5 
    @mui/styled-engine:  5.13.2 
    @mui/system:  5.14.5 
    @mui/types:  7.2.4 
    @mui/utils:  5.14.5 
    @types/react: ^18.2.21 => 18.2.21 
    react: ^18.2.0 => 18.2.0 
    react-dom: ^18.2.0 => 18.2.0 
    typescript: ^4.9.5 => 4.9.5 

tsconfig.json (as it comes with the create-react-app):
{
  "compilerOptions": {
    "target": "es5",
    "lib": [
      "dom",
      "dom.iterable",
      "esnext"
    ],
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "noFallthroughCasesInSwitch": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react-jsx"
  },
  "include": [
    "src"
  ]
}
@erik-d erik-d added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 24, 2023
@zannager zannager added typescript package: system Specific to @mui/system labels Aug 24, 2023
@mnajdova mnajdova assigned DiegoAndai and unassigned mnajdova Aug 25, 2023
@mj12albert
Copy link
Member

It repros in a sandbox – I've added 2 sandboxes to the description, one showing the error on 5.14.1 and one without errors on 5.14.0

@mj12albert mj12albert removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 25, 2023
@DiegoAndai
Copy link
Member

DiegoAndai commented Aug 28, 2023

Thanks for the report! I'm taking a look.

It's important to note that the color prop is not being forwarded, so shouldForwardProp is working. This seems to be purely a Typescript issue.

Here's the comparison between 5.14.0 and 5.14.1 versions if anyone wants to look. At a glance, nothing stood up to me as an obvious cause for this issue.

@DiegoAndai DiegoAndai added the bug 🐛 Something doesn't work label Aug 28, 2023
@DiegoAndai DiegoAndai changed the title Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled [material] Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled Aug 28, 2023
@DiegoAndai
Copy link
Member

DiegoAndai commented Aug 30, 2023

I've identified that this is the cause of the issue, which was added to 5.14.1 in #35924 and later refactored in #38084. Adding component to the Chips props causes, for some reason, the type of FooChip coming from styled to become this instead of this. The ForwardedProps parameter is not passed and thus not picked with Pick.

I'll continue to investigate, sharing this for anyone interested in taking a look or who might have insight into why this is happening.

cc: @michaldudak

@DiegoAndai DiegoAndai moved this to In progress now in Joy UI Aug 30, 2023
@DiegoAndai DiegoAndai removed this from Joy UI Sep 13, 2023
@DiegoAndai DiegoAndai added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Sep 13, 2023
@DiegoAndai
Copy link
Member

I have yet to make time for this one. I added the ready to take label in case anyone can work on it before I do.

@gitstart
Copy link
Contributor

@DiegoAndai we would like to pick this up

@DiegoAndai
Copy link
Member

@gitstart please do 😊
Let me know if you need any help or guidance.

@danilo-leal danilo-leal changed the title [material] Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled [system] Typing changed between 5.14.0 and 5.14.1 and later, when overriding an overridable property using styled Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: system Specific to @mui/system ready to take Help wanted. Guidance available. There is a high chance the change will be accepted typescript
Projects
None yet
6 participants