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

[Card] Fix TypeScript not recognizing "component" prop #20179

Merged
merged 4 commits into from
Mar 30, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 50 additions & 9 deletions packages/material-ui/src/CardHeader/CardHeader.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,29 @@ import * as React from 'react';
import { TypographyProps } from '../Typography';
import { OverridableComponent, OverrideProps } from '../OverridableComponent';

export interface CardHeaderTypeMap<P = {}, D extends React.ElementType = 'div'> {
props: P & {
export interface CardHeaderTypeMap<
export interface CardHeaderTypeMap<
Props = {},
DefaultComponent extends React.ElementType = 'div',
TitleTypographyComponent extends React.ElementType = 'span',
SubheaderTypographyComponent extends React.ElementType = 'span'
> {
props: Props & {
action?: React.ReactNode;
avatar?: React.ReactNode;
disableTypography?: boolean;
subheader?: React.ReactNode;
subheaderTypographyProps?: Partial<TypographyProps>;
subheaderTypographyProps?: TypographyProps<
SubheaderTypographyComponent,
{ component?: SubheaderTypographyComponent }
>;
title?: React.ReactNode;
titleTypographyProps?: Partial<TypographyProps>;
titleTypographyProps?: TypographyProps<
TitleTypographyComponent,
{ component?: TitleTypographyComponent }
>;
};
defaultComponent: D;
defaultComponent: DefaultComponent;
classKey: CardHeaderClassKey;
}
/**
Expand All @@ -25,13 +37,42 @@ export interface CardHeaderTypeMap<P = {}, D extends React.ElementType = 'div'>
*
* - [CardHeader API](https://material-ui.com/api/card-header/)
*/
declare const CardHeader: OverridableComponent<CardHeaderTypeMap>;
declare const CardHeader: OverridableCardHeader;

export interface OverridableCardHeader extends OverridableComponent<CardHeaderTypeMap> {
<
DefaultComponent extends React.ElementType = CardHeaderTypeMap['defaultComponent'],
Props = {},
TitleTypographyComponent extends React.ElementType = 'span',
SubheaderTypographyComponent extends React.ElementType = 'span'
>(
props: { component?: DefaultComponent } & OverrideProps<
CardHeaderTypeMap<
Props,
DefaultComponent,
TitleTypographyComponent,
SubheaderTypographyComponent
>,
DefaultComponent
>,
): JSX.Element;
}

export type CardHeaderClassKey = 'root' | 'avatar' | 'action' | 'content' | 'title' | 'subheader';

export type CardHeaderProps<
D extends React.ElementType = CardHeaderTypeMap['defaultComponent'],
P = {}
> = OverrideProps<CardHeaderTypeMap<P, D>, D>;
DefaultComponent extends React.ElementType = CardHeaderTypeMap['defaultComponent'],
Props = {},
TitleTypographyComponent extends React.ElementType = 'span',
SubheaderTypographyComponent extends React.ElementType = 'span'
> = OverrideProps<
CardHeaderTypeMap<
Props,
DefaultComponent,
TitleTypographyComponent,
SubheaderTypographyComponent
>,
DefaultComponent
>;

export default CardHeader;
180 changes: 180 additions & 0 deletions packages/material-ui/src/CardHeader/CardHeader.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
import * as React from 'react';
import CardHeader from '@material-ui/core/CardHeader';

const CustomComponent: React.FC<{ stringProp: string; numberProp: number }> = () => <div />;

function componentPropTest() {
<CardHeader component="div" />;
<CardHeader component={CustomComponent} stringProp="string" numberProp={1} />;
// $ExpectError
<CardHeader component="incorrectComponent" />;
// $ExpectError
<CardHeader component={CustomComponent} />;
}

function mixedCardHeaderComponentAndTypographyTest() {
<CardHeader component="div" titleTypographyProps={{ component: 'a', href: 'href' }} />;
<CardHeader component="div" subheaderTypographyProps={{ component: 'a', href: 'href' }} />;
<CardHeader
component={CustomComponent}
stringProp="string"
numberProp={1}
titleTypographyProps={{ component: CustomComponent, stringProp: 'stringProp', numberProp: 2 }}
/>;
<CardHeader
component={CustomComponent}
stringProp="string"
numberProp={1}
titleTypographyProps={{ component: CustomComponent, stringProp: 'stringProp', numberProp: 2 }}
subheaderTypographyProps={{
component: CustomComponent,
stringProp: 'stringProp',
numberProp: 2,
}}
/>;
// $ExpectError
<CardHeader component="incorrectComponent" />;
// $ExpectError
<CardHeader component={CustomComponent} />;
<CardHeader
component={CustomComponent}
stringProp="string"
numberProp={1}
// $ExpectError
titleTypographyProps={{ component: CustomComponent, stringProp: 'stringProp' }}
subheaderTypographyProps={{
component: CustomComponent,
stringProp: 'stringProp',
numberProp: 2,
}}
/>;
// $ExpectError
<CardHeader
component={CustomComponent}
stringProp="string"
numberProp={1}
titleTypographyProps={{ component: CustomComponent, stringProp: 'stringProp' }}
subheaderTypographyProps={{ component: CustomComponent, stringProp: 'stringProp' }}
/>;
<CardHeader
// $ExpectError
component="incorrectComponent"
stringProp="string"
numberProp={1}
titleTypographyProps={{ component: CustomComponent, stringProp: 'stringProp', numberProp: 2 }}
subheaderTypographyProps={{
component: CustomComponent,
stringProp: 'stringProp',
numberProp: 2,
}}
/>;
}

function titleTypographyPropsTest() {
// $ExpectError
<CardHeader titleTypographyProps={{ component: 'incorrectComponent' }} />;
<CardHeader titleTypographyProps={{ component: 'a', href: 'href' }} />;
<CardHeader
titleTypographyProps={{ component: CustomComponent, stringProp: 'stringProp', numberProp: 2 }}
/>;
<CardHeader titleTypographyProps={{ variant: 'h1' }} />;
<CardHeader titleTypographyProps={{ align: 'left' }} />;
<CardHeader
titleTypographyProps={{
color: 'primary',
display: 'block',
gutterBottom: true,
noWrap: true,
variantMapping: { h1: 'h1' },
}}
/>;
// $ExpectError
<CardHeader titleTypographyProps={{ component: CustomComponent, numberProp: 2 }} />;
<CardHeader
titleTypographyProps={{
component: 'a',
// $ExpectError
propThatDoesntExist: 'shouldNotWork',
}}
/>;
Copy link
Contributor Author

@rart rart Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eps1lon @fyodore82 need some advice...

CardHeader has an additional level of complexity compared to ListItemText in that it's a type map.

The component prop on both titleTypographyProps & subheaderTypographyProps now works as expected. However I can't seem to find the way to get these particular set of tests to fail — the tests that look into unknown props on typography object. The nested typography prop objects accept any number of unknown props. The correct typography props work as expected and the optional component prop too but the typing system just let's any other unknown props go through as well.

  1. Is this an issue? ListItemText rejects unknown props.
  2. If it is an issue, any ideas on how to address?

FYI: At the moment, the test_types build on the CI is broken for this reason.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the bandwidth to deep-dive into this issue. Maybe some issue with to many free type parameters or some quirk in JSX.

For now just document that this is undesired behavior in code comments. In addition it might be nice to extend these undesired behavior with tests using createElement directly (instead of JSX) and if wrong value types are rejected e.g. { variant: 123213 } or { component: SomeComponentImplementingFooNumber, foo: 'a-string-which-should-be-a-number' }

Copy link
Contributor Author

@rart rart Mar 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Documented the unmet expectations via comments
  • Added createElement tests
    • Behaviour is pretty consistent with JSX. Only exception I found was the type system not demanding the required props from the component prop type like it does in JSX — both for CardHeader and it's nested typography items.
    • Wrong value types are indeed rejected (e.g. { variant: 123213 }, etc)

}

function subheaderTypographyPropsTest() {
<CardHeader subheaderTypographyProps={{ component: 'a', href: 'href' }} />;
<CardHeader
subheaderTypographyProps={{
component: CustomComponent,
stringProp: 'stringProp',
numberProp: 2,
}}
/>;
<CardHeader subheaderTypographyProps={{ variant: 'h1' }} />;
<CardHeader subheaderTypographyProps={{ align: 'left' }} />;
<CardHeader
subheaderTypographyProps={{
color: 'primary',
display: 'block',
gutterBottom: true,
noWrap: true,
variantMapping: { h1: 'h1' },
}}
/>;
<CardHeader
subheaderTypographyProps={{
component: 'a',
// $ExpectError
propThatDoesntExist: 'shouldNotWork',
}}
/>;
// $ExpectError
<CardHeader subheaderTypographyProps={{ component: 'incorrectComponent' }} />;
// $ExpectError
<CardHeader subheaderTypographyProps={{ component: CustomComponent, numberProp: 2 }} />;
}

function mixedTypographyPropsTest() {
<CardHeader
titleTypographyProps={{ component: 'a', href: 'href' }}
subheaderTypographyProps={{ component: 'a', href: 'href' }}
/>;
<CardHeader
titleTypographyProps={{ component: CustomComponent, stringProp: 'stringProp', numberProp: 2 }}
subheaderTypographyProps={{
component: CustomComponent,
stringProp: 'stringProp',
numberProp: 2,
}}
/>;
// $ExpectError
<CardHeader
titleTypographyProps={{ component: 'incorrectComponent' }}
subheaderTypographyProps={{ component: 'incorrectComponent' }}
/>;
<CardHeader
titleTypographyProps={{
component: 'a',
// $ExpectError
propThatDoesntExist: 'shouldNotWork',
}}
subheaderTypographyProps={{
component: 'a',
// $ExpectError
propThatDoesntExist: 'shouldNotWork',
}}
/>;
// $ExpectError
<CardHeader
titleTypographyProps={{ component: CustomComponent, numberProp: 2 }}
subheaderTypographyProps={{ component: CustomComponent, numberProp: 2 }}
/>;
<CardHeader
// $ExpectError
titleTypographyProps={{ component: CustomComponent, numberProp: 2 }}
subheaderTypographyProps={{ component: CustomComponent, numberProp: 2, stringProp: 'yada' }}
/>;
<CardHeader
titleTypographyProps={{ component: CustomComponent, numberProp: 2, stringProp: 'yada' }}
// $ExpectError
subheaderTypographyProps={{ component: CustomComponent, numberProp: 2 }}
/>;
}