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

[prop component] ButtonBaseProps not accepting a component property #15827

Closed
Slessi opened this issue May 24, 2019 · 76 comments · Fixed by #35924
Closed

[prop component] ButtonBaseProps not accepting a component property #15827

Slessi opened this issue May 24, 2019 · 76 comments · Fixed by #35924
Labels

Comments

@Slessi
Copy link
Contributor

Slessi commented May 24, 2019

Expected Behavior 🤔

component property should be accepted

Current Behavior 😯

component property is not accepted

Steps to Reproduce 🕹

const props: ButtonProps<'span'> = {
  component: 'span',
  ^^^^^^^^^^^^^^^^^ 'component' does not exist in type ...
  onClick(event: React.MouseEvent<HTMLSpanElement>) {},
};

Context 🔦

Need for integration with a third-party routing library

Your Environment 🌎

Tech Version
Material-UI v4.0.0
TypeScript v4.1.0

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

@oliviertassinari oliviertassinari added duplicate This issue or pull request already exists and removed duplicate This issue or pull request already exists labels May 24, 2019
@eps1lon

This comment has been minimized.

@eps1lon eps1lon added the status: waiting for author Issue with insufficient information label May 24, 2019
@piotros

This comment has been minimized.

@Slessi

This comment has been minimized.

@piotros

This comment has been minimized.

@eps1lon

This comment has been minimized.

@bengry
Copy link

bengry commented May 25, 2019

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:

Type '{ children: string; component: FunctionComponent<{}>; }' is not assignable to type 'IntrinsicAttributes & Pick<{ action?: ((actions: ButtonBaseActions) => void) | undefined; buttonRef?: ((instance: unknown) => void) | RefObject | null | undefined; centerRipple?: boolean | undefined; ... 6 more ...; TouchRippleProps?: Partial<...> | undefined; } & { ...; } & CommonProps<...> & Pick<...>, "c...'.
Property 'component' does not exist on type 'IntrinsicAttributes & Pick<{ action?: ((actions: ButtonBaseActions) => void) | undefined; buttonRef?: ((instance: unknown) => void) | RefObject | null | undefined; centerRipple?: boolean | undefined; ... 6 more ...; TouchRippleProps?: Partial<...> | undefined; } & { ...; } & CommonProps<...> & Pick<...>, "c...'.

I'm using TypeScript 3.3.3, but also checked 3.4.5, and getting the same error

@eps1lon

This comment has been minimized.

@eps1lon

This comment has been minimized.

@bengry

This comment has been minimized.

@eps1lon

This comment has been minimized.

@bengry

This comment has been minimized.

@eps1lon

This comment has been minimized.

@Slessi

This comment has been minimized.

@Slessi Slessi changed the title ButtonBase not accepting a component property ButtonBaseProps not accepting a component property May 26, 2019
@eps1lon

This comment has been minimized.

@Slessi

This comment has been minimized.

@eps1lon

This comment has been minimized.

@eps1lon eps1lon added component: ButtonBase The React component. and removed status: waiting for author Issue with insufficient information labels May 26, 2019
@Slessi

This comment has been minimized.

@bengry
Copy link

bengry commented May 26, 2019

@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 Link from @reach/router or react-router), but still have the same props passes, so you'd like to do:

<CancelButton component={Link} to="/" />

In this case the cancel button does a navigation back to the home page.
This does not currently compile in MUI v4, due to the aforementioned issue in this thread.

@gunn4r
Copy link

gunn4r commented May 31, 2019

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!

@eps1lon

This comment has been minimized.

@eps1lon
Copy link
Member

eps1lon commented Dec 9, 2020

Looks like we already document it in material-ui.com/guides/typescript/#usage-of-component-prop ok_hand.

I already forgot #19341. Thanks for noticing. I'll update my response above.

@rickstaa
Copy link
Contributor

rickstaa commented Mar 29, 2021

Extend the material-UI button component while also using React.ForwardRef

Based 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 notallowedvariable in the sandbox).

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 React.ForwardRef type constructor like this was done in the @bengry comment, but I did not find the right syntax to get this to work.

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 typecasting

The 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 property

Another 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 overload

Another 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 Sandbox

I 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

@paynecodes
Copy link

paynecodes commented May 27, 2021

@rickstaa Thank you for the awesome write up. This helps a ton. I checked your sandbox. I uncommented notallowedvariable="Typescript should complain when this is uncommented!" in several places, and TypeScript does complain with Property 'notallowedvariable' does not exist on type. Any ideas where this was fixed, or am I mistaken?

@rickstaa
Copy link
Contributor

@paynecodes No problem! That looks like the desired behaviour to me. The notallowedvariable was added to show that the type-checking is working. When this line is commented, typescript should throw a Property 'notallowedvariable' does not exist on type allowed error. You can, of course, also add other variables like typescriptwontlikethis to check that the type checking. Does that answer your question, or did I misinterpret it?

@oliviertassinari
Copy link
Member

@eps1lon We don't document how to solve the extension issue with forwardRef in the mix.
Should the guidance provided by @rickstaa in #15827 (comment) be added in the documentation?

@paynecodes
Copy link

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.

@eps1lon
Copy link
Member

eps1lon commented May 31, 2021

@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 const Component = React.forwardRef(<C>() => {}). I don't think the C generic still exists on Component.

@eps1lon eps1lon removed their assignment May 31, 2021
@rickstaa
Copy link
Contributor

rickstaa commented Jun 1, 2021

@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

@paynecodes
Copy link

paynecodes commented Jun 2, 2021

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 <Button /> component utilizes a custom forwardRef function that abstracts some of the difficulties. I'm not to the point where I know if a similar approach will work, but I hope this helps. Their approach to the component prop is a similar pattern using the as prop, so much of this looks similar but would need some adjusting.

It would be great is this project could support a similar first party solution.

@eps1lon
Copy link
Member

eps1lon commented Jun 2, 2021

For example the chakra-ui component utilizes a custom forwardRef function that abstracts some of the difficulties.

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 React.forwardRef and MaterialUI.forwardRef is confusing and might cause even more mistakes.

@rickstaa
Copy link
Contributor

rickstaa commented Jun 6, 2021

@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?

@eps1lon
Copy link
Member

eps1lon commented Jun 7, 2021

@rickstaa You can't import form @material-ui/core/* in the TypeScript playground (microsoft/TypeScript-Website#206). I'd recommend inlining all the relevant types that aren't importable from @material-ui/core i.e. copy&paste.

@rickstaa
Copy link
Contributor

rickstaa commented Jun 7, 2021

@eps1lon Thanks for the explanation. I was not aware of that. I will give it another try!

@rickstaa
Copy link
Contributor

rickstaa commented Jul 7, 2021

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

@rickstaa
Copy link
Contributor

rickstaa commented Jul 7, 2021

@eps1lon I added all playgrounds to my previous comment.

@ValentinH
Copy link
Contributor

@rickstaa thanks for the awesome explanation and detailed examples! 😍

I'm trying one more thing: override the default color prop of the Button with something like 'color1' | 'color2'.

The closest I got is using Omit<>. However, TS then accepts any other props like your "notallowedvariable" 😞

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 ?

@rickstaa
Copy link
Contributor

rickstaa commented Feb 9, 2022

@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).

@ValentinH
Copy link
Contributor

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 🤣

@rickstaa
Copy link
Contributor

rickstaa commented Feb 9, 2022

No worries, and thank you for taking the time to answer.

Indeed, with muiv5 adding the colours 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 rofl

Haha, good choice! A lot better than having to apply a workaround on a workaround just to make your compiler happy. 😆

@PauliCZ44
Copy link

PauliCZ44 commented Jun 2, 2023

Hi I have had a similar problem with extending the Button component, where I needed few things:
use forawrdRef, use Omit and also extend inteface with extra props. My solution is to create my component with ref as always and then create second component without ref where it is possible to correctly type the interface..

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:
See this thread for better solution:
#35026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.