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

Link Component has bad typings in V4 #15299

Closed
2 tasks done
nmain opened this issue Apr 10, 2019 · 4 comments · Fixed by #15412
Closed
2 tasks done

Link Component has bad typings in V4 #15299

nmain opened this issue Apr 10, 2019 · 4 comments · Fixed by #15412
Labels
bug 🐛 Something doesn't work component: link This is the name of the generic UI component, not the React module! typescript

Comments

@nmain
Copy link

nmain commented Apr 10, 2019

When using "@material-ui/core": "^4.0.0-alpha.7" and typescript, I can't use a custom component in <Link>s.

  • 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 🤔

I've created a code sample: https://github.com/nmain/link-ts-bug
It is a barebones create-react-app --typescript with the minimal changes to illustrate my situation. In it, I have a MyLink component that simply creates an <a> and spreads the provided props to it:

function MyLink(props: React.DetailedHTMLProps<React.AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, ref: React.Ref<HTMLAnchorElement>) {
    // the implementation just rendors an anchor, but imagine this as the custom link of a router solution or similar
    return <a {...props} ref={ref} />;
}

I want to use that as the component for a MUI <Link>. In MUI v4, I have to use React.forwardRef to do that, but it won't pass typechecking:

      {/* Works, but fails typecheck when the `as any` is removed */}
      <Link component={MyLink as any} href="//example.com">Clack me!</Link>
      {/* Passes typecheck, but fails at runtime -- can't ref a functional component */}
      <Link component={MyLinkWithoutRefForwarding} href="//example.com">Clack me!</Link>

Current Behavior 😯

MyLinkWithoutRefForwarding causes an error message at runtime as MUI v4 is no longer using findDOMNode.
MyLink, when the as any typecast is removed, causes a typechecking error:

TypeScript error: Type 'ForwardRefExoticComponent<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "key" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | ... 257 more ... | "referrerPolicy"> & RefAttributes<...>>' is not assignable to type '"object" | "style" | "title" | "time" | "link" | "menu" | "dialog" | "a" | "abbr" | "address" | "area" | "article" | "aside" | "audio" | "b" | "base" | "bdi" | "bdo" | "big" | "blockquote" | ... 93 more ... | undefined'.
  Type 'ForwardRefExoticComponent<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "key" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | ... 257 more ... | "referrerPolicy"> & RefAttributes<...>>' is not assignable to type 'FunctionComponent<LinkProps>'.
    Types of property 'defaultProps' are incompatible.
      Type 'Partial<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "key" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | ... 257 more ... |
"referrerPolicy"> & RefAttributes<...>> | undefined' is not assignable to type 'Partial<LinkProps> |
undefined'.
        Type 'Partial<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "key" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | ... 257 more ...
| "referrerPolicy"> & RefAttributes<...>>' is not assignable to type 'Partial<LinkProps>'.
          Type 'Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "key" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | ... 257 more ... | "referrerPolicy"> & RefAttributes<...>' is not assignable to type 'LinkProps'.
            Types of property 'color' are incompatible.
              Type 'string | undefined' is not assignable to type '"initial" | "inherit" | "primary"
| "secondary" | "textPrimary" | "textSecondary" | "error" | undefined'.
                Type 'string' is not assignable to type '"initial" | "inherit" | "primary" | "secondary" | "textPrimary" | "textSecondary" | "error" | undefined'.  TS2322

Steps to Reproduce 🕹

https://github.com/nmain/link-ts-bug was created as follows:

create-react-app link-ts-bug --typescript
cd link-ts-bug
npm i --save @material-ui/core@next
(create MyLink.tsx)
(Edit App.tsx)
npm run start

Context 🔦

In the real use case that I have extracted this example from, <MyLink> does some custom modifications to href to work with some legacy ASP.NET routing. As I continue to evolve this project, I hope to have <MyLink> do some client-side routing instead at some point in the future. Either way, it should be a fully functional stand-in for <a>.

Your Environment 🌎

Package.json deps:

  "dependencies": {
    "@material-ui/core": "^4.0.0-alpha.7",
    "@types/jest": "24.0.11",
    "@types/node": "11.13.2",
    "@types/react": "16.8.13",
    "@types/react-dom": "16.8.3",
    "react": "^16.8.6",
    "react-dom": "^16.8.6",
    "react-scripts": "2.1.8",
    "typescript": "3.4.3"
  },
@eps1lon eps1lon added component: link This is the name of the generic UI component, not the React module! typescript labels Apr 10, 2019
@eps1lon
Copy link
Member

eps1lon commented Apr 10, 2019

  1. You can't use a function both as a component and as an argument to React.forwardRef. This is why the second one fails.

Warning: Unexpected ref object provided for *. Use either a ref-setter function or React.createRef().

  1. Your specific example (the one requiring the any cast) might've been fixed with [core] Type ref for components #15199
  2. The accepted component is not fully generic. We would need apply [typescript] Generic props for FormControl, FormLabel, List #15292 to Link. Happy to review a PR with that change.

@eps1lon eps1lon added bug 🐛 Something doesn't work help wanted labels Apr 10, 2019
@jeremy-coleman
Copy link

All of the forward refd types are wrong bc they use the wrong withStyles types AND they type it as a regular component. You can fix it by creatinf your own version where its like React.forwardRef(function Link(props: LinkProps, ref){ etc

@eps1lon
Copy link
Member

eps1lon commented Apr 17, 2019

@jeremy-coleman

All of the forward refd types are wrong bc they use the wrong withStyles types [...]

What would be the correct type? Could you add some examples that should fail/pass in the type checker but are actually sound/unsound?

[...] AND they type it as a regular component

The type of the component is not part of the public API. Only the props. I'm not sure what you mean by "regular component"?

@pachuka
Copy link
Contributor

pachuka commented Apr 19, 2019

@eps1lon - I took a pass at fixing the types, it resolved about 15 TSC errors I was getting for using <Link> with the component prop in one of my projects so I think it's working correctly (both Link from react-router-dom, as well as rendering as a 'button', etc).

I'm not entirely sure where to add tests for this, didnt see a .spec.tsx in the Link component (or when those runs), and was unclear about the typescript docs as well. If anyone has thoughts on that or could point me to an example of what to do there, I'm happy to add that.

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: link This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants