mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.59k stars 32.21k forks source link

[TypeScript] Paper with component=Link #21154

Closed michalbundyra closed 3 years ago

michalbundyra commented 4 years ago

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

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 commented 4 years ago

Please provide a full reproduction test case. This would help a lot 👷 . A live example would be perfect. This codesandbox.io template may be a good starting point. Thank you!

michalbundyra commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

@michalbundyra I think that we can consider this a bug or an enhancement, depends on the perspective. Using the new pattern look something like this:

diff --git a/packages/material-ui/src/Paper/Paper.d.ts b/packages/material-ui/src/Paper/Paper.d.ts
index f9bef6078..e76a0d362 100644
--- a/packages/material-ui/src/Paper/Paper.d.ts
+++ b/packages/material-ui/src/Paper/Paper.d.ts
@@ -1,32 +1,48 @@
 import * as React from 'react';
-import { StandardProps } from '..';
+import { OverridableComponent, OverrideProps } from '../OverridableComponent';

-export interface PaperProps
-  extends StandardProps<React.HTMLAttributes<HTMLDivElement>, PaperClassKey> {
-  /**
-   * The content of the component.
-   */
-  children?: React.ReactNode;
-  /**
-   * 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.
-   */
-  elevation?: number;
-  /**
-   * If `true`, rounded corners are disabled.
-   */
-  square?: boolean;
-  /**
-   * The variant to use.
-   */
-  variant?: 'elevation' | 'outlined';
+export interface PaperTypeMap<P = {}, D extends React.ElementType = 'div'> {
+  props: P & {
+    /**
+     * The content of the component.
+     */
+    children?: React.ReactNode;
+    /**
+     * 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.
+     */
+    elevation?: number;
+    /**
+     * If `true`, rounded corners are disabled.
+     */
+    square?: boolean;
+    /**
+     * The variant to use.
+     */
+    variant?: 'elevation' | 'outlined';
+  };
+  defaultComponent: D;
+  classKey: PaperClassKey;
 }

+/**
+ *
+ * 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 =
   | 'root'
   | 'rounded'
@@ -57,15 +73,9 @@ export type PaperClassKey =
   | 'elevation23'
   | 'elevation24';

-/**
- *
- * Demos:
- *
- * - [Cards](https://material-ui.com/components/cards/)
- * - [Paper](https://material-ui.com/components/paper/)
- *
- * API:
- *
- * - [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;

We track this migration effort in #19536. However, it comes with a number of other issues, it's not easy:

rpk-red commented 3 years ago

What is status of this issue? Is it solved?

oliviertassinari commented 3 years ago

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

cyrilchapon commented 3 years ago

You are awesome guys 🤩

cyrilchapon commented 3 years ago

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 commented 3 years ago

@cyrilchapon See #16843

saiki4116 commented 3 years ago

@oliviertassinari I am picking this up

saiki4116 commented 3 years ago

@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 commented 3 years ago

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

mngu commented 3 years ago

@oliviertassinari Hi ! Can I work on this ?

oliviertassinari commented 3 years ago

@mngu Yes, definitely :)

saiki4116 commented 3 years ago

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 commented 3 years ago

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

RemyMachado commented 2 years ago

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.