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

[TypeScript] Paper with component=Link #21154

Closed
2 tasks done
michalbundyra opened this issue May 23, 2020 · 18 comments · Fixed by #25059
Closed
2 tasks done

[TypeScript] Paper with component=Link #21154

michalbundyra opened this issue May 23, 2020 · 18 comments · Fixed by #25059
Labels
component: Paper 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

@michalbundyra
Copy link

michalbundyra commented May 23, 2020

I might be doing something wrong, but I cannot use component Link with Paper.

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

Getting the following exception:

Type '(props: LinkRouter) => JSX.Element' is not assignable to type '"object" | "meter" | "textarea" | "button" | "style" | "progress" | "ruby" | "table" | "small" | "sub" | "embed" | "pre" | "caption" | "menu" | "menuitem" | "a" | "abbr" | "address" | ... 101 more ... | undefined'.
  Type '(props: LinkRouter) => JSX.Element' is not assignable to type 'FunctionComponent<HTMLAttributes<HTMLElement>>'.
    Types of parameters 'props' and 'props' are incompatible.
      Type 'PropsWithChildren<HTMLAttributes<HTMLElement>>' is not assignable to type 'LinkRouter'.
        Property 'to' is missing in type 'HTMLAttributes<HTMLElement> & { children?: ReactNode; }' but required in type 'LinkProps<PoorMansUnknown>'.

Expected Behavior 🤔

I should be able to use any component with Paper.

Steps to Reproduce 🕹

Steps:

import React from "react";
import {Link as RouterLink, LinkProps as RouterLinkProps} from "react-router-dom";
import Link, {LinkProps} from "@material-ui/core/Link";
import Paper from "@material-ui/core/Paper";

type LinkRouter = RouterLinkProps & Omit<LinkProps, "component">

const LinkRouter = (props: LinkRouter) => <Link underline="none" component={RouterLink} {...props}/>

export default () => (
    <Paper component={LinkRouter} to="/page">Link</Paper>
)

It works as expected if I use Avatar instead.

Context 🔦

I'd like to keep classes and options from paper but use a link.
It works fine without TypeScript...

Your Environment 🌎

Tech Version
Material-UI v4.9.14
React v16.13.1
Browser Chrome
TypeScript 3.9.3
@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label May 24, 2020
@oliviertassinari

This comment has been minimized.

@michalbundyra
Copy link
Author

@oliviertassinari Thanks for response. I've created live example, as suggested:

https://codesandbox.io/s/mui-paper-link-ts-neio1

The problem is that it compiles on codesandbox, but not on local! But on the codesandbox at lest we are getting the same error - underline "component" with message:

(JSX attribute) PaperProps.component?: "object" | "a" | "abbr" | "address" | "area" | "article" | "aside" | "audio" | "b" | "base" | "bdi" | "bdo" | "big" | "blockquote" | "body" | "br" | "button" | "canvas" | "caption" | ... 100 more ... | undefined
The component used for the root node. Either a string to use a DOM element or a component.

Type '(props: OverrideProps<LinkTypeMap<RouterLinkProps<History.PoorMansUnknown>, RouterLink<History.PoorMansUnknown>>, RouterLink<History.PoorMansUnknown>>) => JSX.Element' is not assignable to type '"object" | "a" | "abbr" | "address" | "area" | "article" | "aside" | "audio" | "b" | "base" | "bdi" | "bdo" | "big" | "blockquote" | "body" | "br" | "button" | "canvas" | "caption" | ... 100 more ... | undefined'.
  Type '(props: OverrideProps<LinkTypeMap<RouterLinkProps<History.PoorMansUnknown>, RouterLink<History.PoorMansUnknown>>, RouterLink<History.PoorMansUnknown>>) => JSX.Element' is not assignable to type 'FunctionComponent<HTMLAttributes<HTMLElement>>'.
    Types of parameters 'props' and 'props' are incompatible.
      Type 'PropsWithChildren<HTMLAttributes<HTMLElement>>' is not assignable to type 'OverrideProps<LinkTypeMap<LinkProps<PoorMansUnknown>, Link<PoorMansUnknown>>, Link<PoorMansUnknown>>'.
        Property 'to' is missing in type 'HTMLAttributes<HTMLElement> & { children?: ReactNode; }' but required in type 'LinkProps<PoorMansUnknown>'.ts(2322)
index.d.ts(57, 5): 'to' is declared here.
Paper.d.ts(10, 3): The expected type comes from property 'component' which is declared here on type 'IntrinsicAttributes & PaperProps'

When I run it on local I am getting the same error but also: "Failed to compile.".

Can you help me, please? Thanks!

@oliviertassinari oliviertassinari added support: Stack Overflow Please ask the community on Stack Overflow and removed status: waiting for author Issue with insufficient information labels May 24, 2020
@support support bot closed this as completed May 24, 2020
@oliviertassinari oliviertassinari added typescript and removed support: Stack Overflow Please ask the community on Stack Overflow labels May 24, 2020
@support support bot reopened this May 24, 2020
@mui mui deleted a comment from support bot May 24, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented May 24, 2020

@michalbundyra Thanks for the reproduction, I have put the Paper component side by side with the Link one: https://codesandbox.io/s/material-demo-qrsnl. It seems to originate from the different structures of the types. Paper uses StandardProps while Link uses OverridableComponent. Only the latter work.

@michalbundyra
Copy link
Author

@oliviertassinari thanks. What does it mean? Is it a bug or desired behaviour in that case?

There is no any issue with JSX, just I've noted the problem when changed the project to TypeScript.

@oliviertassinari

This comment has been minimized.

@rpk-red

This comment has been minimized.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 9, 2020

@cyrilchapon Thanks for shared this simpler reproduction in #23449: https://codesandbox.io/s/basicalerts-material-demo-forked-qgvhc. Looking back at the thread, it's awesome to see that the issues we were depending on got solved (#21002, #20191). What do you think about this fix?

diff --git a/packages/material-ui/src/Paper/Paper.d.ts b/packages/material-ui/src/Paper/Paper.d.ts
index 010593284c..7afecf7d03 100644
--- a/packages/material-ui/src/Paper/Paper.d.ts
+++ b/packages/material-ui/src/Paper/Paper.d.ts
@@ -1,77 +1,85 @@
 import * as React from 'react';
 import { OverridableStringUnion } from '@material-ui/types';
-import { InternalStandardProps as StandardProps } from '..';
+import { OverridableComponent, OverrideProps } from '../OverridableComponent';

 export interface PaperPropsVariantOverrides {}
 export type PaperVariantDefaults = Record<'elevation' | 'outlined', true>;

-export interface PaperProps extends StandardProps<React.HTMLAttributes<HTMLDivElement>> {
-  /**
-   * 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 unless `square={true}`. */
-    rounded?: string;
-    /** Styles applied to the root element if `variant="outlined"`. */
-    outlined?: string;
-    /** Styles applied to the root element if `variant="elevation"`. */
-    elevation?: string;
-    elevation0?: string;
-    elevation1?: string;
-    elevation2?: string;
-    elevation3?: string;
-    elevation4?: string;
-    elevation5?: string;
-    elevation6?: string;
-    elevation7?: string;
-    elevation8?: string;
-    elevation9?: string;
-    elevation10?: string;
-    elevation11?: string;
-    elevation12?: string;
-    elevation13?: string;
-    elevation14?: string;
-    elevation15?: string;
-    elevation16?: string;
-    elevation17?: string;
-    elevation18?: string;
-    elevation19?: string;
-    elevation20?: string;
-    elevation21?: string;
-    elevation22?: string;
-    elevation23?: string;
-    elevation24?: string;
+export interface PaperTypeMap<P = {}, D extends React.ElementType = 'div'> {
+  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 unless `square={true}`. */
+      rounded?: string;
+      /** Styles applied to the root element if `variant="outlined"`. */
+      outlined?: string;
+      /** Styles applied to the root element if `variant="elevation"`. */
+      elevation?: string;
+      elevation0?: string;
+      elevation1?: string;
+      elevation2?: string;
+      elevation3?: string;
+      elevation4?: string;
+      elevation5?: string;
+      elevation6?: string;
+      elevation7?: string;
+      elevation8?: string;
+      elevation9?: string;
+      elevation10?: string;
+      elevation11?: string;
+      elevation12?: string;
+      elevation13?: string;
+      elevation14?: string;
+      elevation15?: string;
+      elevation16?: string;
+      elevation17?: string;
+      elevation18?: string;
+      elevation19?: string;
+      elevation20?: string;
+      elevation21?: string;
+      elevation22?: string;
+      elevation23?: string;
+      elevation24?: string;
+    };
+    /**
+     * Shadow depth, corresponds to `dp` in the spec.
+     * It accepts values between 0 and 24 inclusive.
+     */
+    elevation?: number;
+    /**
+     * If `true`, rounded corners are disabled.
+     */
+    square?: boolean;
+    /**
+     * The variant to use.
+     */
+    variant?: OverridableStringUnion<PaperVariantDefaults, PaperPropsVariantOverrides>;
   };
-  /**
-   * The component used for the root node.
-   * Either a string to use a HTML element or a component.
-   */
-  component?: React.ElementType<React.HTMLAttributes<HTMLElement>>;
-  /**
-   * Shadow depth, corresponds to `dp` in the spec.
-   * It accepts values between 0 and 24 inclusive.
-   * @default 1
-   */
-  elevation?: number;
-  /**
-   * If `true`, rounded corners are disabled.
-   * @default false
-   */
-  square?: boolean;
-  /**
-   * The variant to use.
-   * @default 'elevation'
-   */
-  variant?: OverridableStringUnion<PaperVariantDefaults, PaperPropsVariantOverrides>;
+  defaultComponent: D;
 }

-export type PaperClassKey = keyof NonNullable<PaperProps['classes']>;
+/**
+ *
+ * Demos:
+ *
+ * - [Cards](https://material-ui.com/components/cards/)
+ * - [Paper](https://material-ui.com/components/paper/)
+ *
+ * API:
+ *
+ * - [Paper API](https://material-ui.com/api/paper/)
+ */
+declare const Paper: OverridableComponent<PaperTypeMap>;
+
+export type PaperClassKey = keyof NonNullable<PaperTypeMap['props']['classes']>;

 /**
  *
@@ -84,4 +92,9 @@ export type PaperClassKey = keyof NonNullable<PaperProps['classes']>;
  *
  * - [Paper API](https://material-ui.com/api/paper/)
  */
-export default function Paper(props: PaperProps): JSX.Element;
+export type PaperProps<
+  D extends React.ElementType = PaperTypeMap['defaultComponent'],
+  P = {}
+> = OverrideProps<PaperTypeMap<P, D>, D>;
+
+export default Paper;

Do you want to work on a pull request? :) This is one chunk of #19536.

@oliviertassinari oliviertassinari added component: Paper 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. labels Nov 9, 2020
@cyrilchapon
Copy link

You are awesome guys 🤩

@cyrilchapon
Copy link

cyrilchapon commented Nov 21, 2020

I'm encountering

Property 'noValidate' does not exist on type 'IntrinsicAttributes & BoxProps & { children?: ReactNode; }'.

With a <Box component='form' noValidate />

Is that same underlying issue ? Or should I reopen separately ?
@eps1lon @oliviertassinari

EDIT:
For now, using kinda same hack

import { FunctionComponent, FormHTMLAttributes } from 'react'
import { Box, BoxProps } from '@material-ui/core'

type FormBoxProps = BoxProps & FormHTMLAttributes<HTMLFormElement>

export const FormBox: FunctionComponent<FormBoxProps> = (props) => (
  <Box {...props} component='form' />
)

@oliviertassinari
Copy link
Member

@cyrilchapon See #16843

@saiki4116
Copy link

@oliviertassinari I am picking this up

@saiki4116
Copy link

saiki4116 commented Nov 30, 2020

@oliviertassinari ,

I can add more tests in PaperTest like the ones present for AvatarTest using TestOverride component.

Changes would break this test

const refTest = () => {
  // for a detailed explanation of refs in react see https://github.com/mui-org/material-ui/pull/15199
  const genericRef = React.createRef<Element>();
  const divRef = React.createRef<HTMLDivElement>();
  const inputRef = React.createRef<HTMLInputElement>();

  <Paper ref={genericRef} />;
  <Paper ref={divRef} />;
  // undesired: throws when assuming inputRef.current.value !== undefined
  <Paper ref={inputRef} />;
  // recommended: soundness is the responsibility of the dev
  // alternatively use React.useRef<unknown>()  or React.createRef<unknown>()
  <Paper
    ref={(ref) => {
      // with runtime overhead, sound usage
      if (ref instanceof HTMLInputElement) {
        const i: number = ref.valueAsNumber;
      }
      // unsafe casts, sound usage, no runtime overhead
      const j: number = (ref as HTMLInputElement).valueAsNumber;
      // unsafe casts, unsound usage, no runtime overhead
      const k: number = (ref as any).valueAsNumber;
      // @ts-expect-error unsound usage, no runtime overhead, least syntax
      const n: number = ref.valueAsNumber;
    }}
  />;
};

is there any other component that can be used for this test, instead of Paper

@oliviertassinari
Copy link
Member

@saiki4116 I wouldn't worry about this test case as the component approach is the one we used everywhere.

@mngu
Copy link
Contributor

mngu commented Feb 22, 2021

@oliviertassinari Hi ! Can I work on this ?

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 22, 2021

@mngu Yes, definitely :)

@saiki4116
Copy link

Thanks @mngu for picking this up, Just a heads-up.
I did changes, but the build was failing for a couple of Typescript bindings(alert.d.ts and dialog.d.ts) as they were using StandardProps and I didn't get time to proceed after that.
I believe it might not be an case now, as more components are being moved to Overridable component pattern.
Here is my feature branch, could be a of help in closing this issue.

@oliviertassinari
Copy link
Member

@saiki4116 Strange, the error didn't reproduce in the CI.

@RemyMachado
Copy link

I'm still having this issue with

<Paper
        {...props}
        component="a"
>
TS2769: No overload matches this call.
Overload 1 of 2, '(props: { component: "a"; } & { children?: ReactNode; classes?: Partial<PaperClasses> | undefined; elevation?: number | undefined; square?: boolean | undefined; sx?: SxProps<...> | undefined; variant?: "elevation" | ... 1 more ... | undefined; } & CommonProps & Omit<...>): Element', gave the following error.
Type '{ children: Element[]; className: string; component: "a"; classes?: (Partial<PaperClasses> & Partial<ClassNameMap<never>>) | undefined; ... 257 more ...; onTransitionEndCapture?: TransitionEventHandler<...> | undefined; }' is not assignable to type 'Omit<Pick<DetailedHTMLProps<AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>, "key" | keyof AnchorHTMLAttributes<...>> & { ...; }, keyof CommonProps | ... 4 more ... | "variant">'.
Types of property 'ref' are incompatible.
Type '((instance: HTMLDivElement | null) => void) | RefObject<HTMLDivElement> | null | undefined' is not assignable to type '((instance: HTMLAnchorElement | null) => void) | RefObject<HTMLAnchorElement> | null | undefined'.
Type '(instance: HTMLDivElement | null) => void' is not assignable to type '((instance: HTMLAnchorElement | null) => void) | RefObject<HTMLAnchorElement> | null | undefined'. 
Type '(instance: HTMLDivElement | null) => void' is not assignable to type '(instance: HTMLAnchorElement | null) => void'.
Types of parameters 'instance' and 'instance' are incompatible.
Type 'HTMLAnchorElement | null' is not assignable to type 'HTMLDivElement | null'.
Property 'align' is missing in type 'HTMLAnchorElement' but required in type 'HTMLDivElement'. 
Overload 2 of 2, '(props: DefaultComponentProps<PaperTypeMap<{}, "div">>): Element', gave the following error.
Type '{ children: Element[]; className: string; component: string; classes?: (Partial<PaperClasses> & Partial<ClassNameMap<never>>) | undefined; ... 257 more ...; onTransitionEndCapture?: TransitionEventHandler<...> | undefined; }' is not assignable to type 'IntrinsicAttributes & { children?: ReactNode; classes?: Partial<PaperClasses> | undefined; elevation?: number | undefined; square?: boolean | undefined; sx?: SxProps<...> | undefined; variant?: "elevation" | ... 1 more ... | undefined; } & CommonProps & Omit<...>'.
Property 'component' does not exist on type 'IntrinsicAttributes & { children?: ReactNode; classes?: Partial<PaperClasses> | undefined; elevation?: number | undefined; square?: boolean | undefined; sx?: SxProps<...> | undefined; variant?: "elevation" | ... 1 more ... | undefined; } & CommonProps & Omit<...>'.

I tried:

props?: PaperProps & AnchorHTMLAttributes<HTMLAnchorElement>>

But it doesn't work.
Any help would be greatly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Paper 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
Development

Successfully merging a pull request may close this issue.

7 participants