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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

TS2322 Spreading declared MenuItemProps on MenuItem fails type checking #16245

Closed
2 tasks done
SoulsD opened this issue Jun 15, 2019 · 8 comments
Closed
2 tasks done

TS2322 Spreading declared MenuItemProps on MenuItem fails type checking #16245

SoulsD opened this issue Jun 15, 2019 · 8 comments
Labels
bug 馃悰 Something doesn't work component: menu This is the name of the generic UI component, not the React module! typescript

Comments

@SoulsD
Copy link

SoulsD commented Jun 15, 2019

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 馃

MenuItemProps should be typed with correct 'button' override by default the when imported

Current Behavior 馃槸

from core/MenuItem/MenuItem.d.ts:

declare const MenuItem: OverridableComponent<ListItemTypeMap<{ button: false }, 'li'>> &
  ExtendButtonBase<ListItemTypeMap<{ button?: true }, 'li'>>;

export type MenuItemProps<D extends React.ElementType = 'li', P = {}> = OverrideProps<
  ListItemTypeMap<P, D>,
  D
>;

P parameter of menu Item props default to {} instead of { button?: true } resulting in an error when you want to use it directly:

    var a: MenuItemProps = {};
    <MenuItem {...a}/>

    // error: 
    // Types of property 'button' are incompatible.
    //     Type 'boolean | undefined' is not assignable to type 'true | undefined'.
    //         Type 'false' is not assignable to type 'true | undefined'

To dismiss the error, type MenuItemProps needs to be declared like this:

MenuItemProps<'li', { button?: true }>

Steps to Reproduce 馃暪

    import MenuItem, { MenuItemProps } from "@material-ui/core/MenuItem";
    /* ... */

    var a: MenuItemProps = {};
    <MenuItem {...a}/>

Link:

  1. https://codesandbox.io/s/create-react-app-with-typescript-3j44q

Context 馃敠

I am migrating to MUIv4 from v3. I had a component taking MenuItemProps in its own props to be passed to its children MenuItems. I am now forced to type this as MenuItemProps<'li', { button?: true }>.

Your Environment 馃寧

Tech Version
Material-UI v4.1.1
React v16.8
Browser Chrome 74
TypeScript 3.4.1
@oliviertassinari
Copy link
Member

This seems to be the same core issue than #16122.

@rosskevin
Copy link
Member

rosskevin commented Jun 20, 2019

Broken test pr in #16315, I don't think this is the same as #16122

@rosskevin rosskevin added the bug 馃悰 Something doesn't work label Jun 20, 2019
@rosskevin rosskevin changed the title [TS] type MenuItemProps doesn't correspond to actual MenuItem props by default TS2322 Spreading declared MenuItemProps on MenuItem fails type checking Jun 20, 2019
@Kyderman

This comment has been minimized.

@sirajalam049

This comment has been minimized.

@girardiricardo

This comment has been minimized.

@oliviertassinari oliviertassinari added the component: menu This is the name of the generic UI component, not the React module! label Oct 4, 2020
@oliviertassinari
Copy link
Member

How about we refactor MenuItem to be built on top of ButtonBase, not on top of ListItem? Would it help solve this TypeScript issue?

@DMR-coding
Copy link

DMR-coding commented Oct 30, 2020

How about we refactor MenuItem to be built on top of ButtonBase, not on top of ListItem? Would it help solve this TypeScript issue?

That would solve the issue for MenuItem specifically, but ListItem itself is broken identically. A solution which goes more to the root of the problem would be preferable.

@mnajdova
Copy link
Member

mnajdova commented Jun 28, 2021

The problem should be resolved now that #26591 is merged (The MenuItem no longer depends on the ListItem). Prove https://codesandbox.io/s/create-react-app-with-typescript-forked-9t9n3?file=/src/App.tsx (I've updated the original sandbox with the latest (next) release).

Regarding the ListItem, the problem seems to be solved with #26480 if I am not mistaken (sandbox - https://codesandbox.io/s/create-react-app-with-typescript-forked-zoiml?file=/src/App.tsx)

@SoulsD feel free to re-open if something was not addressed by the two changes mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 馃悰 Something doesn't work component: menu This is the name of the generic UI component, not the React module! typescript
Projects
None yet
8 participants