-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[prop component] ButtonBaseProps not accepting a component property #15827
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
To add to this, the following doesn't compile in TypeScript in MUIv4, but should: // MyButton.ts
import { Button } from '@material-ui/core'
import { ButtonProps } from '@material-ui/core/Button'
import React from 'react'
export const MyButton = React.forwardRef<HTMLButtonElement, ButtonProps>((btnProps, ref) => (
<Button
ref={ref}
variant="contained"
color="primary"
{...btnProps}
/>
))
// MyPage.ts
function MyPage() {
return (
<MyButton component={Link}> // <-- error on this
Click me
</MyButton>
)
} error message is:
I'm using TypeScript 3.3.3, but also checked 3.4.5, and getting the same error |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@eps1lon here's a more realistic example: const CancelButton = React.forwardRef<
HTMLButtonElement,
Omit<ButtonProps, 'component'> & ComponentWorkaroundProps<LinkProps<void>>
>(({ children, ...props }, ref) => {
return (
<Button variant="contained" {...props} ref={ref} color="secondary">
{children || 'Cancel'}
</Button>
);
});
const ConfirmButton = React.forwardRef<
HTMLButtonElement,
Omit<ButtonProps, 'component'> & ComponentWorkaroundProps<LinkProps<void>>
>(({ children, ...props }, ref) => {
return (
<Button variant="contained" {...props} ref={ref} color="primary">
{children || 'Confirm'}
</Button>
);
}); These two buttons should be used throughout the app in cases where you want a Confirm or a Cancel button (think inside Dialogs for example, but could go inside other components as well). However, sometimes you want these to be links (using the <CancelButton component={Link} to="/" /> In this case the cancel button does a navigation back to the home page. |
Just want to chime in and am having this same issue. Trying to get our codebase migrated to v4 and this issue has been a real headache! I created a stackoverflow issue regarding it with a code sandbox and such. My use case is basically identical to @bengry's. We have various wrapped buttons for things like Cancel, Confirm, etc with some custom styling / props and what not. Here's the stackoverflow if interested: https://stackoverflow.com/questions/56300136/when-extending-button-component-property-not-found-what-is-the-correct-way-to Would love to get past this issue! Thanks for all the awesome work on v4! |
This comment has been minimized.
This comment has been minimized.
I already forgot #19341. Thanks for noticing. I'll update my response above. |
Extend the material-UI button component while also using React.ForwardRefBased on @oliviertassinari's answer, the Material-ui documentation, @bengry answer, this StackOverflow question and this pullrequest I found a solution that gets rid of any typescript errors in my project. import React from "react";
import Button, { ButtonProps } from "@material-ui/core/Button";
import { BrowserRouter, Link } from "react-router-dom";
/** Generic forwarded component typing (with forwardRef) + extra prop
*
* The extra prop can now be added in the type definition directly or extending
* the material-ui component props with declaration merging (https://www.typescriptlang.org/docs/handbook/declaration-merging.html)
*/
const CustomRefButtonExtraProp = React.forwardRef(
<C extends React.ElementType>(
props: ButtonProps<C, { component?: C; fun?: boolean }>,
ref?: React.Ref<HTMLButtonElement>
) => {
const { children, fun, ...rest } = props;
if (fun) {
console.log("This is fun!!!");
}
return (
<Button ref={ref} {...rest}>
{children}
</Button>
);
}
); This allows me to keep Typescript happy while extending the material-ui button while using the ForwardRef higher-order function. This code can be seen in action in this sandbox. I, however, quickly realized that although this solution doesn't throw any errors, it also does not work as it disables type-checking all together (this can be seen when uncommenting the The type inference seems to work when we use the example given in the documentation. However, when we wrap it with the ForwardRef higher-order function, the current syntax breaks the type-checking. This is because the generic type is not defined in the ForwardRef type constructor. I tried solving this by placing the generic type definition inside the export const MyButton = React.forwardRef<HTMLButtonElement, ButtonProps<C, { component?: C; fun?: boolean }>(
(props, ref) => {
const { children, fun, ...rest } = props;
if (fun) {
console.log("This is fun!!!");
}
return (
<Button ref={ref} {...rest}>
{children}
</Button>
);
}
); Judging from this StackOverflow issue, this is not yet possible. An explanation on why this does not work is given in this issue. A lot of different solutions to this problem can be found online (see references below). I tried all these solutions out in this code sandbox. From all these solutions, I think the typecasting or custom reference props solutions are the cleanest. If your using TS3.4 or higher you can also use a function overload to leverage TS 3.4 higher order function type inference: Use typecastingThe first solution is to use type casting to make the Typescript compiler aware of the generic types: type CastedForwardRefButtonType = <C extends React.ElementType>(
props: ButtonProps<C, { component?: C }>,
ref?: React.Ref<HTMLButtonElement>
) => React.ReactElement;
const CastedForwardRefButtonFnc: CastedForwardRefButtonType = (props, ref) => {
const { children, ...rest } = props;
return (
<Button ref={ref} {...rest}>
{children}
</Button>
);
};
const CastedForwardRefButton = React.forwardRef(
CastedForwardRefButtonFnc
) as CastedForwardRefButtonType;
export default function App() {
render(
<CastedForwardRefButton
component={Link}
ref={buttonRef}
to="/test"
// notallowedvariable="Typescript does complain here!"
/>
)
}
... Use a custom reference propertyAnother solution is not to use the ForwardRef function and pass the reference via the props. const CustomArrowButtonCustomRef = <C extends React.ElementType>(
props: ButtonProps<C, { component?: C; buttonRef?: React.Ref<HTMLButtonElement> }>
) => {
const { children, buttonRef, ...rest } = props;
return (
<Button ref={buttonRef} {...rest}>
{children}
</Button>
);
};
export default function App() {
render(
<CustomArrowButtonCustomRef
component={Link}
buttonRef={buttonRef}
to="/test"
// notallowedvariable="Typescript does complain here!"
/>
)
} Use a function overloadAnother solution is not to use the ForwardRef function and pass the reference via the props. declare module "react" {
function forwardRef<T, P = {}>(
render: (props: P, ref: React.Ref<T>) => React.ReactElement | null
): (props: P & React.RefAttributes<T>) => React.ReactElement | null;
}
const DeclaredCustomRefButtonExtraProp = React.forwardRef(
<C extends React.ElementType>(
props: ButtonProps<C, { component?: C; fun?: boolean; chicken?: string }>,
ref?: React.Ref<HTMLButtonElement>
) => {
const { children, fun, ...rest } = props;
if (fun) {
console.log("This is fun!!!");
}
return (
<Button ref={ref} {...rest}>
{children}
</Button>
);
}
);
export default function App() {
render(
<DeclaredCustomRefButtonExtraProp
component={Link}
ref={buttonRef}
to="/test"
// notallowedvariable="Typescript does complain here!"
/>
)
} Code SandboxI documented all the solutions to my knowledge currently available for this problem in this code sandbox. I also tried overloading the function declarations of React.ForwardRef as was explained in this issue but I was not able to find the right syntax to do this. If somebody has found the right function overload syntax or another solution, please let me know, and I will update the CodeSandbox. Possible solutions
Upstream issues |
@rickstaa Thank you for the awesome write up. This helps a ton. I checked your sandbox. I uncommented |
@paynecodes No problem! That looks like the desired behaviour to me. The |
@eps1lon We don't document how to solve the extension issue with |
I do think these workarounds should be documented, but I also think a better solution should be considered. These types are quite mind-bending, error-prone, and not fully functional. I have no clue what would make this situation better at the moment though. |
@rickstaa Thanks for this work. That's a great starting point. Could you create TypeScript playgrounds for each solution so that we can verify that the components are in fact still polymorphic? I'm a bit sceptical about |
@eps1lon I'm happy to break down all the solutions in their own playground. I will probably take a look at it this weekend. I will send you the updated playgrounds when they are finished. TODOS
|
Perhaps the patterns implemented in chakra-ui will be useful for someone here. I'm looking more deeply into them now. For example the chakra-ui It would be great is this project could support a similar first party solution. |
I do have this in mind as a fall back solution. It would be unfortunate if that would be the best solution since fragmenting between |
@eps1lon I tried creating a playground for the first todo but since I have never used the typescript playground I did not find a way to install the required npm modules. I currently added them under the plugins tab but I keep receiving an error about the types not being available (see this playground). My assumption is that there are some settings or pre-installed plugins in the codesandbox which are not enabled by default in the playground. Maybe you can nudge me in the right direction? |
@rickstaa You can't import form |
@eps1lon Thanks for the explanation. I was not aware of that. I will give it another try! |
@eps1lon I moved the first example from the code sandbox to the typescript playground. I put the type definitions in the tail of the document. Was this what you had in mind? In the current form it can not be run. |
@eps1lon I added all playgrounds to my previous comment. |
@rickstaa thanks for the awesome explanation and detailed examples! 😍 I'm trying one more thing: override the default The closest I got is using type ExtraProps = {
color: 'color1' | 'color2'
}
type CastedForwardRefButtonType = <C extends React.ElementType>(
props: Omit<ButtonProps<C, { component?: C; }>, 'color'> & ExtraProps ,
ref?: React.Ref<HTMLButtonElement>
) => React.ReactElement | null;
const CastedForwardRefButtonFnc: CastedForwardRefButtonType = (props, ref) => {
const { children, ...rest } = props;
return (
<Button ref={ref} {...rest}>
{children}
</Button>
);
};
const CastedForwardRefButton = React.forwardRef(
CastedForwardRefButtonFnc
) as CastedForwardRefButtonType; If I don't use Omit and simply do: type CastedForwardRefButtonType = <C extends React.ElementType>(
props: ButtonProps<C, { component?: C; }> & ExtraProps ,
ref?: React.Ref<HTMLButtonElement>
) => React.ReactElement | null; the Button only accepts the original colors (primary and secondary). I'm aware that you wrote these types a while ago but by any chance do you see a quick win ? |
@ValentinH Sorry for the very late response. I hope you found your answer. I wrote those types a long time ago, so I didn't yet have the time to dive into them again. However, I quickly checked to find the correct syntax for the casting method but failed. I could also not find a way to do this with the module declaration method I use in my projects. It might be a limitation of the typescript compiler, which requires a workaround similar to the following stack overflow questions:
The easiest way I can think is is to add your colours to the material UI theme (see https://mui.com/customization/palette/#customization). |
No worries and thank you for taking the time to answer. Indeed, with muiv5 adding the colors to the theme is the way to go. I was stuck on V4 when writing this comment. However, it's still a limitation for supporting any other extra prop... For now, I simply relaxed the types and moved on with my life 🤣 |
Haha, good choice! A lot better than having to apply a workaround on a workaround just to make your compiler happy. 😆 |
Hi I have had a similar problem with extending the Button component, where I needed few things: import { Button as MuiButton, ButtonProps as MuiButtonProps, styled } from '@mui/material'
interface ButtonProps extends Omit<MuiButtonProps, 'color'> {
color?: 'primary' | 'secondary' | 'tertiary' | 'inherit'
danger?: boolean
}
type ExtraProps = Pick<ButtonProps, 'color' | 'danger' >
const SMuiButton = styled(MuiButton, {
...someStyles
})
const Button = forwardRef<HTMLButtonElement, ButtonProps>(
({ color = 'primary', danger, children, ...props }, ref) => {
const resolveColor = () => (color === 'tertiary' ? 'primary' : color)
return (
<SMuiButton
color={resolveColor()}
{...{ icon, danger, ...props }}
ref={ref}
>
{resolveLabel()}
</SMuiButton>
)
}
) Then I create custom component only for its type, and cast its type to forwarded button: // So we have to create a component tha is typed correctly and then use its type as typeof for our Button with forwardRef.
// ExtraProps will merge with MuiButtonProps and then we will omit color from it.
// Add ref as a prop so TS wont complain about it. But in fact it will be forwarded to MuiButton because of forwardRef.
const CorrectlyTypedButton= <C extends any>(
props: ExtraProps &
// @ts-ignore - TS doesn't allow to use C extends any as a type for component prop. But it works only with any or {}
Omit<MuiButtonProps<C, { component?: C; ref?: React.Ref<HTMLButtonElement> }>, 'color'>
) => {
const { children, ...rest } = props
return <Button {...rest}>{children}</Button>
}
Button.displayName = 'Button'
export default Button as typeof CorrectlyTypedButton Then typescript will correctly handle custom component props, unkown props and intelisense is also there (both for extra props and original props) EDIT: |
Expected Behavior 🤔
component
property should be acceptedCurrent Behavior 😯
component
property is not acceptedSteps to Reproduce 🕹
Context 🔦
Need for integration with a third-party routing library
Your Environment 🌎
Edit @eps1lon: Heavily editorialized the original issue which was inaccurate and misleading.
Most of the issue has been solved, see https://next.material-ui.com/guides/typescript/#usage-of-component-prop
The text was updated successfully, but these errors were encountered: