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 expects ref of type HTMLSpanElement #24901

Closed
2 tasks done
qlonik opened this issue Feb 13, 2021 · 6 comments · Fixed by #28101
Closed
2 tasks done

[Link] component expects ref of type HTMLSpanElement #24901

qlonik opened this issue Feb 13, 2021 · 6 comments · Fixed by #28101
Labels
bug 🐛 Something doesn't work component: link This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. typescript

Comments

@qlonik
Copy link

qlonik commented Feb 13, 2021

  • The issue is present in the latest release.
  • 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/[email protected]/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/[email protected]/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
{
  "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"
  ]
}
@qlonik qlonik added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 13, 2021
@oliviertassinari
Copy link
Member

@qlonik Do you have a reproduction?

@oliviertassinari oliviertassinari added component: link This is the name of the generic UI component, not the React module! status: waiting for author Issue with insufficient information typescript and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 13, 2021
@qlonik
Copy link
Author

qlonik commented Feb 13, 2021

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

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work and removed status: waiting for author Issue with insufficient information labels Feb 13, 2021
@oliviertassinari oliviertassinari changed the title Link component expects ref of type HTMLSpanElement [Link] component expects ref of type HTMLSpanElement Feb 13, 2021
@oliviertassinari
Copy link
Member

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);

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Feb 13, 2021
@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. and removed ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Feb 18, 2021
@mnajdova mnajdova added OCD21 and removed good first issue Great for first contributions. Enable to learn the contribution process. labels Jun 30, 2021
@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Jul 16, 2021
@eric-burel
Copy link
Contributor

eric-burel commented Jul 16, 2021

We have a non-minimal repro here: VulcanJS/vulcan-next#114 (exact commit: VulcanJS/vulcan-next@3de85da)
You can check "packages/@vulcanjs/next-material-ui/components/Link.tsx", it's based on the example quoted by @qlonik I am currently investigating a quickfix.

Update: first, current demo implementation (here) seems to pass prefetch props to the anchor when defined.
I am refactoring the code and adding typings, a few things might be improved in the first place I think.

Update 2: gave it another shot https://github.com/VulcanJS/vulcan-next/blob/d2fb4ac5206d8076f64db51472325799bc6d7b37/packages/%40vulcanjs/next-material-ui/components/Link.tsx
I indeed end up with a cast to fix the ref typing issue, other than that it works well. I've isolated the props that should be provided to the a link from those provided to the NextLink as well for better consistency.

@thinceller
Copy link

Is there any update on this?
Can I create a pull request using the modification approach in this comment?

@mnajdova
Copy link
Member

Can I create a pull request using the modification approach in this comment?

Sure, looks like no one is working on it.

@mnajdova mnajdova added good first issue Great for first contributions. Enable to learn the contribution process. and removed ready to take Help wanted. Guidance available. There is a high chance the change will be accepted OCD21 labels Aug 24, 2021
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! good first issue Great for first contributions. Enable to learn the contribution process. typescript
Projects
None yet
5 participants