[Link] component expects ref of type HTMLSpanElement

qlonik commented 3 years ago

[x] The issue is present in the latest release.
[x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The property ref passed to Link component is expected to be of type HTMLSpanElement instead of HTMLAnchorElement

import MuiLink from '@material-ui/core/Link'

const x = (
  <MuiLink
    ref={(x) => {
      // x has type `HTMLSpanElement | null`
    }}
  >
    Test
  </MuiLink>
)

Expected Behavior 🤔

Not sure. Shouldn't it be HTMLAnchorElement?

Reproduction

The reproduction code is based on the example 'nextjs-with-typescript'. In that example, there is Link component wrapping around Next Link and available as NextLinkComposed. When I define another component using forwardRef and use either a or NextLinkComposed and pass-through properties from MuiLink, then I get the type error on ref property of both a and NextLinkComposed.

/* eslint-disable jsx-a11y/anchor-has-content */
import type { LinkProps as MuiLinkProps } from '@material-ui/core/Link'
import type { LinkProps as NextLinkProps } from 'next/link'
import NextLink from 'next/link'
import * as React from 'react'

interface NextLinkComposedProps
  extends Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, 'href'>,
    Omit<NextLinkProps, 'href' | 'as'> {
  to: NextLinkProps['href']
  linkAs?: NextLinkProps['as']
  href?: NextLinkProps['href']
}

export const NextLinkComposed = React.forwardRef<
  HTMLAnchorElement,
  NextLinkComposedProps
>(function NextLinkComposed(props, ref) {
  const {
    to,
    linkAs,
    href,
    replace,
    scroll,
    passHref,
    shallow,
    prefetch,
    locale,
    ...other
  } = props

  return (
    <NextLink
      href={to}
      prefetch={prefetch}
      as={linkAs}
      replace={replace}
      scroll={scroll}
      shallow={shallow}
      passHref={passHref}
      locale={locale}
    >
      <a ref={ref} {...other} />
    </NextLink>
  )
})

const Component = React.forwardRef<HTMLAnchorElement, MuiLinkProps>(
  (props, ref) => {
    const first = <NextLinkComposed ref={ref} to="/test" {...props} />
    const second = (
      <a ref={ref} href="/test" {...props}>
        Test
      </a>
    )

    return null
  },
)

Type error:

src/file.tsx:50:37 - error TS2322: Type '((instance: HTMLAnchorElement | null) => void) | MutableRefObject<HTMLAnchorElement | null> | ((instance: HTMLSpanElement | null) => void) | RefObject<...> | null' is not assignable to type '((instance: HTMLAnchorElement | null) => void) | RefObject<HTMLAnchorElement> | null | undefined'.
  Type 'RefObject<HTMLSpanElement>' is not assignable to type '((instance: HTMLAnchorElement | null) => void) | RefObject<HTMLAnchorElement> | null | undefined'.
    Type 'RefObject<HTMLSpanElement>' is not assignable to type 'RefObject<HTMLAnchorElement>'.
      Type 'HTMLSpanElement' is missing the following properties from type 'HTMLAnchorElement': charset, coords, download, hreflang, and 21 more.

50     const first = <NextLinkComposed ref={ref} to="/test" {...props} />
                                       ~~~

  node_modules/.pnpm/@types/react@17.0.2/node_modules/@types/react/index.d.ts:144:9
    144         ref?: Ref<T>;
                ~~~
    The expected type comes from property 'ref' which is declared here on type 'IntrinsicAttributes & NextLinkComposedProps & RefAttributes<HTMLAnchorElement>'

src/file.tsx:52:10 - error TS2322: Type '((instance: HTMLAnchorElement | null) => void) | MutableRefObject<HTMLAnchorElement | null> | ((instance: HTMLSpanElement | null) => void) | RefObject<...> | null' is not assignable to type 'string | ((instance: HTMLAnchorElement | null) => void) | RefObject<HTMLAnchorElement> | null | undefined'.
  Type 'RefObject<HTMLSpanElement>' is not assignable to type 'string | ((instance: HTMLAnchorElement | null) => void) | RefObject<HTMLAnchorElement> | null | undefined'.
    Type 'RefObject<HTMLSpanElement>' is not assignable to type 'RefObject<HTMLAnchorElement>'.

52       <a ref={ref} href="/test" {...props}>
            ~~~

  node_modules/.pnpm/@types/react@17.0.2/node_modules/@types/react/index.d.ts:147:9
    147         ref?: LegacyRef<T>;
                ~~~
    The expected type comes from property 'ref' which is declared here on type 'DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>'

Found 2 errors.

Context 🔦

I was digging through the code and it appears that it is coming from where LinkBaseProps uses TypographyProps here. Typography props use default element in this case, which is span. It should get fixed if HTMLAnchorElement is explicitly passed to TypographyProps with TypographyProps<HTMLAnchorElement>.

Additionally, it seems that this fix would remove the need to pass any type in these spots:

Your Environment 🌎

`npx @material-ui/envinfo`

``` System: OS: Linux 5.10 Arch Linux Binaries: Node: 15.8.0 - /usr/bin/node Yarn: 1.22.10 - /usr/bin/yarn npm: 7.5.4 - /usr/bin/npm Browsers: Chrome: Not Found Firefox: Not Found npmPackages: @emotion/react: ^11.1.5 => 11.1.5 @emotion/styled: ^11.1.5 => 11.1.5 @material-ui/core: ^5.0.0-alpha.25 => 5.0.0-alpha.25 @material-ui/types: 5.1.7 @types/react: ^17.0.2 => 17.0.2 react: 17.0.1 => 17.0.1 react-dom: 17.0.1 => 17.0.1 typescript: ^4.1.5 => 4.1.5 ```

tsconfig.json

```json { "compilerOptions": { "target": "ESNext", "module": "ESNext", "moduleResolution": "Node", "lib": [ "dom", "dom.iterable", "esnext" ], "strict": true, "skipLibCheck": true, "allowJs": true, "resolveJsonModule": true, "esModuleInterop": true, "forceConsistentCasingInFileNames": true, "isolatedModules": true, "noEmit": true, "jsx": "preserve" }, "include": [ "next-env.d.ts", "**/*.ts", "**/*.tsx" ], "exclude": [ "node_modules" ] } ```

oliviertassinari commented 3 years ago

@qlonik Do you have a reproduction?

qlonik commented 3 years ago

edited the original comment to add the example with the type error

oliviertassinari commented 3 years ago

I meant something we could run locally. Here we go: https://codesandbox.io/s/strange-wright-083td?file=/src/App.tsx.

Maybe a fix like this?

diff --git a/packages/material-ui/src/Link/Link.d.ts b/packages/material-ui/src/Link/Link.d.ts
index a05b381d63..e933974967 100644
--- a/packages/material-ui/src/Link/Link.d.ts
+++ b/packages/material-ui/src/Link/Link.d.ts
@@ -73,7 +73,7 @@ declare const Link: OverridableComponent<LinkTypeMap>;
 export type LinkClassKey = keyof NonNullable<LinkTypeMap['props']['classes']>;

 export type LinkBaseProps = Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, 'color'> &
-  DistributiveOmit<TypographyProps, 'children' | 'component' | 'color' | 'variant'>;
+  DistributiveOmit<TypographyProps<'a'>, 'children' | 'component' | 'color' | 'variant'>;

 export type LinkProps<
   D extends React.ElementType = LinkTypeMap['defaultComponent'],

or we can reproduce the ButtonBase approach:

diff --git a/packages/material-ui/src/Link/Link.d.ts b/packages/material-ui/src/Link/Link.d.ts
index a05b381d63..88636b9315 100644
--- a/packages/material-ui/src/Link/Link.d.ts
+++ b/packages/material-ui/src/Link/Link.d.ts
@@ -1,60 +1,66 @@
 import * as React from 'react';
-import { DistributiveOmit } from '@material-ui/types';
 import { SxProps } from '@material-ui/system';
-import { OverridableComponent, OverrideProps } from '../OverridableComponent';
+import { OverridableComponent, OverrideProps, OverridableTypeMap } from '../OverridableComponent';
 import { Theme } from '../styles';
-import { TypographyProps } from '../Typography';
+import { TypographyProps, TypographyTypeMap } from '../Typography';

-export interface LinkTypeMap<P = {}, D extends React.ElementType = 'a'> {
-  props: P &
-    LinkBaseProps & {
-      /**
-       * The content of the component.
-       */
-      children?: React.ReactNode;
-      /**
-       * Override or extend the styles applied to the component.
-       */
-      classes?: {
-        /** Styles applied to the root element. */
-        root?: string;
-        /** Styles applied to the root element if `underline="none"`. */
-        underlineNone?: string;
-        /** Styles applied to the root element if `underline="hover"`. */
-        underlineHover?: string;
-        /** Styles applied to the root element if `underline="always"`. */
-        underlineAlways?: string;
-        /** Styles applied to the root element if `component="button"`. */
-        button?: string;
-        /** Pseudo-class applied to the root element if the link is keyboard focused. */
-        focusVisible?: string;
-      };
-      /**
-       * The color of the link.
-       * @default 'primary'
-       */
-      color?: TypographyProps['color'];
-      /**
-       * The system prop that allows defining system overrides as well as additional CSS styles.
-       */
-      sx?: SxProps<Theme>;
-      /**
-       * `classes` prop applied to the [`Typography`](/api/typography/) element.
-       */
-      TypographyClasses?: TypographyProps['classes'];
-      /**
-       * Controls when the link should have an underline.
-       * @default 'hover'
-       */
-      underline?: 'none' | 'hover' | 'always';
-      /**
-       * Applies the theme typography styles.
-       * @default 'inherit'
-       */
-      variant?: TypographyProps['variant'];
+
+export interface ExtendTypographyTypeMap<M extends OverridableTypeMap> {
+  props: M['props'] & Omit<TypographyTypeMap['props'], 'classes'>;
+  defaultComponent: M['defaultComponent'];
+}
+
+export type LinkTypeMap<P = {}, D extends React.ElementType = 'a'> = ExtendTypographyTypeMap<{
+  props: P & {
+    /**
+     * The content of the component.
+     */
+    children?: React.ReactNode;
+    /**
+     * Override or extend the styles applied to the component.
+     */
+    classes?: {
+      /** Styles applied to the root element. */
+      root?: string;
+      /** Styles applied to the root element if `underline="none"`. */
+      underlineNone?: string;
+      /** Styles applied to the root element if `underline="hover"`. */
+      underlineHover?: string;
+      /** Styles applied to the root element if `underline="always"`. */
+      underlineAlways?: string;
+      /** Styles applied to the root element if `component="button"`. */
+      button?: string;
+      /** Pseudo-class applied to the root element if the link is keyboard focused. */
+      focusVisible?: string;
     };
+    /**
+     * The color of the link.
+     * @default 'primary'
+     */
+    color?: TypographyProps['color'];
+    /**
+     * The system prop that allows defining system overrides as well as additional CSS styles.
+     */
+    sx?: SxProps<Theme>;
+    /**
+     * `classes` prop applied to the [`Typography`](/api/typography/) element.
+     */
+    TypographyClasses?: TypographyProps['classes'];
+    /**
+     * Controls when the link should have an underline.
+     * @default 'hover'
+     */
+    underline?: 'none' | 'hover' | 'always';
+    /**
+     * Applies the theme typography styles.
+     * @default 'inherit'
+     */
+    variant?: TypographyProps['variant'];
+  };
   defaultComponent: D;
-}
+}>;

 /**
  *
@@ -72,9 +78,6 @@ declare const Link: OverridableComponent<LinkTypeMap>;

 export type LinkClassKey = keyof NonNullable<LinkTypeMap['props']['classes']>;

-export type LinkBaseProps = Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, 'color'> &
-  DistributiveOmit<TypographyProps, 'children' | 'component' | 'color' | 'variant'>;
-
 export type LinkProps<
   D extends React.ElementType = LinkTypeMap['defaultComponent'],
   P = {}

@eps1lon A preference? Both will allow removing the any in the Next.js Link integration:

diff --git a/docs/src/modules/components/Link.tsx b/docs/src/modules/components/Link.tsx
index 7ec2d025e2..3f1b732f7c 100644
--- a/docs/src/modules/components/Link.tsx
+++ b/docs/src/modules/components/Link.tsx
@@ -79,14 +79,14 @@ const Link = React.forwardRef<HTMLAnchorElement, LinkProps>(function Link(props,

   if (isExternal) {
     if (noLinkStyle) {
-      return <a className={className} href={href as string} ref={ref as any} {...other} />;
+      return <a className={className} href={href as string} ref={ref} {...other} />;
     }

     return <MuiLink className={className} href={href as string} ref={ref} {...other} />;
   }

   if (noLinkStyle) {
-    return <NextLinkComposed className={className} ref={ref as any} to={href} {...other} />;
+    return <NextLinkComposed className={className} ref={ref} to={href} {...other} />;
   }

   let linkAs = linkAsProp || (href as string);