mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
92.44k stars 31.84k forks source link

[Select] Prop component + value breaks compilation for MenuItem #21210

Open ImADrafter opened 4 years ago

ImADrafter commented 4 years ago

Combination of both prop component + prop value seems incompatible.

Current Behavior 😯

value, a valid prop for MenuItem (inherit from li native element) seems to break compilation, if provided along with component prop. No matter if component is a native HTML element, a custom component or MUI component.

Expected Behavior 🤔

You can use both props w/o typescript compilation errors.

Steps to Reproduce 🕹

As you can see in the demo, every combination should be acceptable. As you can see in the second case, component="Typography" lets you use any Typography prop along with MenuItem props. You can comment line 18 to check this out. But when it comes to value, Typescript complains for line 15

Property 'component' does not exist on type 'IntrinsicAttributes & { action?: Ref<ButtonBaseActions>; buttonRef?: Ref<unknown>; centerRipple?: boolean; disabled?: boolean; disableRipple?: boolean; ... 4 more ...; TouchRippleProps?: Partial<...>; } & { ...; } & { ...; } & CommonProps<...> & Pick<...>'

I've check out how MenuItem is basically a ListItem with aditional props, and the problem persists as well using this component. Probably is something with OverridableComponent interface or BaseButton, im not sure.

Context 🔦

This "value" prop, not only an acceptable prop for native element, but it is indirectly being supported as MUI documentation for Select allows you to use MenuItem as children, but I couldn't find a demo with, also, a prop component.

Your Environment 🌎

Tech Version
Material-UI v4.10.0 (latest)
React v16.13.1
Browser 83.0.4103.61
TypeScript 3.9.3
eps1lon commented 4 years ago

This is unfortunately expected since this only works because you've wrapped the MenuItems in Select. That component removes the value via cloneElement.

We'll fix this in v5 where we will have a dedicated component for Select options.

ImADrafter commented 4 years ago

This is unfortunately expected since this only works because you've wrapped the MenuItems in Select. That component removes the value via cloneElement.

We'll fix this in v5 where we will have a dedicated component for Select options.

First of all, thank you for taking the time to answer, and adding this issue to the milestone. I really appreciate it.

Your thought makes sense, but I've tried replacing the select with a fragment and the compilation still broken for the same cases. Is this still related to the underneath cloneElement ?

miguelCT commented 4 years ago

I'm not sure if what @eps1lon mentions is exactly the reason. As @ImADrafter says, it seems more a Typescript compile error than an implementation bug per se. In the JS version of the same code, there is not even an execution error or warning.

Basically, I've followed the Material-UI docs and examples to use the "component" prop in Typescript with the MenuItem + Link (react-router), and the result is that same compile error. So, I guess this could be one of the cases stated in the TS guide, where it says:

Not every component fully supports any component type you pass in. If you encounter a component that rejects its component props in TypeScript please open an issue. There is an ongoing effort to fix this by making component props generic.

What do you think?

MonstraG commented 3 years ago

I think I'm hitting the same issue with NextJs's Link:

import { forwardRef, Ref } from "react";
import Link, { LinkProps } from "next/link";
import { MenuItem, MenuItemProps } from "@material-ui/core";

type Props = MenuItemProps & Pick<LinkProps, "href" | "as" | "prefetch" | "locale">;

const LinkMenuItem = ({ href, as, prefetch, locale, ...props }: Props, ref: Ref<HTMLLIElement>) => (
    <Link href={href} as={as} prefetch={prefetch} locale={locale} passHref>
        {/* @ts-ignore https://github.com/mui-org/material-ui/issues/21210 probably going to be fixed in mui v5 */}
        <MenuItem component="a" ref={ref} {...props} />
    </Link>
);

export default forwardRef<HTMLLIElement, Props>(LinkMenuItem);

This code gives me Property 'component' does not exist on type...

Although it does render and work correctly, and doesnt throw anything in the console.

ariesshrimp commented 2 years ago

here we are in v5.2.4 and it doesn't seem like Select's have their own option components. docs still using <MenuItem/>'s: https://mui.com/components/selects/

MenuItem docs make it look like <MenuItem component="" /> should be possible: https://mui.com/api/menu-item/