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.

Sign up for GitHub

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] Fix with* injectors ignoring defaultProps #12673

Merged
merged 11 commits into from
Sep 11, 2018
Merged

[typescript] Fix with* injectors ignoring defaultProps #12673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5c36cb0
[core] Fix withStyles ignoring defaultProps
eps1lon Aug 27, 2018
d6d88bb
[typescript] Fix chore review issues
eps1lon Aug 30, 2018
25b333b
[typescript] Normalize order of InjectedProps and TargetProps
eps1lon Aug 31, 2018
a10f121
[typescript] Remove extraneous type constraint in Shared
eps1lon Aug 31, 2018
917f221
[typescript] Removed extraneous Shared type
eps1lon Aug 31, 2018
93da0df
[typescript] Add more test cases for stateless componets
eps1lon Sep 4, 2018
5da60a0
[typescript] Fix false positive consistency check for implicit SFCs
eps1lon Sep 6, 2018
8c83e14
[typescript] Removed unused prop
eps1lon Sep 6, 2018
eed5c48
[typescript] Cleanup component tests
eps1lon Sep 6, 2018
859d2d9
[typescript] Fix wrong injected theme type in withStyles
eps1lon Sep 8, 2018
a0ffe02
[typescript] Introduce generic prop injector HOC
eps1lon Sep 8, 2018
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
27 changes: 21 additions & 6 deletions packages/material-ui/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ import * as React from 'react';
import { StyledComponentProps } from './styles';
export { StyledComponentProps };

export type AnyComponent<P = any> =
| (new (props: P) => React.Component)
| ((props: P & { children?: React.ReactNode }) => React.ReactElement<any> | null);

export type PropsOf<C extends AnyComponent> = C extends new (props: infer P) => React.Component
export type PropsOf<C> = C extends new (props: infer P) => React.Component
? P
: C extends (props: infer P) => React.ReactElement<any> | null ? P : never;

Expand Down Expand Up @@ -55,7 +51,26 @@ export type Omit<T, K extends keyof any> = T extends any ? Pick<T, Exclude<keyof
*
* @internal
*/
export type ConsistentWith<T, U> = Pick<U, keyof T & keyof U>;
export type ConsistentWith<DecorationTargetProps, InjectedProps> = {
[P in keyof DecorationTargetProps]: P extends keyof InjectedProps
? InjectedProps[P] extends DecorationTargetProps[P]
? DecorationTargetProps[P]
: InjectedProps[P]
: DecorationTargetProps[P]
};

/**
* a function that takes {component} and returns a component that passes along
* all the props to {component} except the {InjectedProps} and will accept
* additional {AdditionalProps}
*/
export type PropInjector<InjectedProps, AdditionalProps = {}> = <
C extends React.ComponentType<ConsistentWith<PropsOf<C>, InjectedProps>>
>(
component: C,
) => React.ComponentType<
Omit<JSX.LibraryManagedAttributes<C, PropsOf<C>>, keyof InjectedProps> & AdditionalProps
>;

/**
* Like `T & U`, but using the value types from `U` where their properties overlap.
Expand Down
10 changes: 4 additions & 6 deletions packages/material-ui/src/styles/withStyles.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import * as React from 'react';
import { WithTheme } from '../styles/withTheme';
import { AnyComponent, ConsistentWith, Overwrite, Omit } from '..';
import { Omit, PropInjector, PropsOf } from '..';
import { Theme } from './createMuiTheme';
import * as CSS from 'csstype';
import * as JSS from 'jss';
Expand Down Expand Up @@ -41,12 +40,13 @@ export type ClassNameMap<ClassKey extends string = string> = Record<ClassKey, st
export type WithStyles<
T extends string | StyleRules | StyleRulesCallback = string,
IncludeTheme extends boolean | undefined = undefined
> = (IncludeTheme extends true ? WithTheme : Partial<WithTheme>) & {
> = (IncludeTheme extends true ? { theme: Theme } : {}) & {
classes: ClassNameMap<
T extends string
? T
: T extends StyleRulesCallback<infer K> ? K : T extends StyleRules<infer K> ? K : never
>;
innerRef?: React.Ref<any> | React.RefObject<any>;
};

export interface StyledComponentProps<ClassKey extends string = string> {
Expand All @@ -60,6 +60,4 @@ export default function withStyles<
>(
style: StyleRulesCallback<ClassKey> | StyleRules<ClassKey>,
options?: Options,
): <P extends ConsistentWith<P, StyledComponentProps<ClassKey> & Partial<WithTheme>>>(
component: AnyComponent<P & WithStyles<ClassKey, Options['withTheme']>>,
) => React.ComponentType<Overwrite<Omit<P, 'theme'>, StyledComponentProps<ClassKey>>>;
): PropInjector<WithStyles<ClassKey, Options['withTheme']>, StyledComponentProps<ClassKey>>;
6 changes: 2 additions & 4 deletions packages/material-ui/src/styles/withTheme.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import { Theme } from './createMuiTheme';
import { AnyComponent, ConsistentWith, Overwrite } from '..';
import { PropInjector } from '..';

export interface WithTheme {
theme: Theme;
innerRef?: React.Ref<any> | React.RefObject<any>;
}

export default function withTheme(): <P extends ConsistentWith<P, WithTheme>>(
component: AnyComponent<P & WithTheme>,
) => React.ComponentType<Overwrite<P, Partial<WithTheme>>>;
export default function withTheme(): PropInjector<WithTheme, Partial<WithTheme>>;
6 changes: 2 additions & 4 deletions packages/material-ui/src/withWidth/withWidth.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Breakpoint } from '../styles/createBreakpoints';
import { AnyComponent, ConsistentWith, Overwrite } from '..';
import { PropInjector } from '..';

export interface WithWidthOptions {
resizeInterval: number;
Expand All @@ -24,6 +24,4 @@ export function isWidthUp(

export default function withWidth(
options?: WithWidthOptions,
): <P extends ConsistentWith<P, WithWidth>>(
component: AnyComponent<P & WithWidth>,
) => React.ComponentType<Overwrite<P, Partial<WithWidth>>>;
): PropInjector<WithWidth, Partial<WithWidth>>;
8 changes: 5 additions & 3 deletions packages/material-ui/src/withWidth/withWidth.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ const Decorated = withWidth()(withStyles(styles)(Hello));

<Decorated name="Bob" />;

const WidthSFC = withWidth()<{
// shouldn't need to specify width here; it's a given
interface SFCProps extends WithWidth {
name: string;
}>(({ width, name }) => <div style={{ width }}>hello, {name}</div>);
}
const WidthSFC = withWidth()(({ width, name }: SFCProps) => (
<div style={{ width }}>hello, {name}</div>
));

<WidthSFC name="Hortense" />;
94 changes: 80 additions & 14 deletions packages/material-ui/test/typescript/styles.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ import {
import Button from '@material-ui/core/Button/Button';
import blue from '@material-ui/core/colors/blue';
import { WithTheme } from '@material-ui/core/styles/withTheme';
import { StandardProps } from '@material-ui/core';
import { PropsOf, StandardProps } from '@material-ui/core';
import { TypographyStyle } from '@material-ui/core/styles/createTypography';

// Shared types for examples
interface ComponentProps {
interface ComponentProps extends WithStyles<typeof styles> {
text: string;
}

Expand All @@ -31,7 +31,7 @@ const styles = ({ palette, spacing }: Theme) => ({
},
});

const StyledExampleOne = withStyles(styles)<ComponentProps>(({ classes, text }) => (
const StyledExampleOne = withStyles(styles)(({ classes, text }: ComponentProps) => (
<div className={classes.root}>{text}</div>
));
<StyledExampleOne text="I am styled!" />;
Expand Down Expand Up @@ -70,7 +70,7 @@ const stylesAsPojo = {

const AnotherStyledSFC = withStyles({
root: { backgroundColor: 'hotpink' },
})(({ classes }) => <div className={classes.root}>Stylish!</div>);
})(({ classes }: WithStyles<'root'>) => <div className={classes.root}>Stylish!</div>);

// Overriding styles
const theme = createMuiTheme({
Expand Down Expand Up @@ -162,18 +162,21 @@ function OverridesTheme() {
}

// withTheme
const ComponentWithTheme = withTheme()(({ theme }) => <div>{theme.spacing.unit}</div>);
const ComponentWithTheme = withTheme()(({ theme }: WithTheme) => <div>{theme.spacing.unit}</div>);

<ComponentWithTheme />;

// withStyles + withTheme
type AllTheProps = WithTheme & WithStyles<typeof styles>;

const AllTheComposition = withTheme()(
withStyles(styles)(({ theme, classes }: AllTheProps) => (
<div className={classes.root}>{theme.palette.text.primary}</div>
)),
);
const StyledComponent = withStyles(styles)(({ theme, classes }: AllTheProps) => (
<div className={classes.root}>{theme.palette.text.primary}</div>
));

// missing prop theme
<StyledComponent />; // $ExpectError

const AllTheComposition = withTheme()(StyledComponent);

<AllTheComposition />;

Expand Down Expand Up @@ -201,7 +204,7 @@ declare const themed: boolean;
);
<Foo />;

const Bar = withStyles({}, { withTheme: true })(({ theme }) => (
const Bar = withStyles({}, { withTheme: true })(({ theme }: WithStyles<string, true>) => (
<div style={{ margin: theme.spacing.unit }} />
));
<Bar />;
Expand Down Expand Up @@ -292,12 +295,13 @@ withStyles(theme =>
});

interface ListItemContentProps extends WithStyles<typeof styles> {
children?: React.ReactElement<any>;
inset?: boolean;
row?: boolean;
}

const ListItemContent = withStyles(styles, { name: 'ui-ListItemContent' })<ListItemContentProps>(
({ children, classes, inset, row }) => (
const ListItemContent = withStyles(styles, { name: 'ui-ListItemContent' })(
({ children, classes, inset, row }: ListItemContentProps) => (
<div className={classes.root} color="textSecondary">
{children}
</div>
Expand All @@ -311,7 +315,7 @@ withStyles(theme =>
b: boolean;
}

const ListItemContent = withStyles({ x: {}, y: {} })<FooProps>(props => <div />);
const ListItemContent = withStyles({ x: {}, y: {} })((props: FooProps) => <div />);
}

{
Expand Down Expand Up @@ -383,3 +387,65 @@ withStyles(theme =>
text: theme.typography.body2,
});
}

{
// can't provide own `classes` type
interface Props {
classes: number;
}

class Component extends React.Component<Props & WithStyles<typeof styles>> {}
// $ExpectError
const StyledComponent = withStyles(styles)(Component);
Copy link
Member

Choose a reason for hiding this comment

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

This consistency check isn't working when applied to an SFC:

// $ExpectError
withStyles(styles)((props: Props) => null);

Copy link
Member Author

Choose a reason for hiding this comment

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

@pelotom This is a limitation of the never type. I cannot assign T to never but a function that takes T to a function that takes never. typescript playground

If I type this function explicitly as a React.SFC I do get errors but the message is confusing:
Property 'children' is missing in type 'ValidationMap<Props>'.

I'm going to verify if this was working before. If we can't resolve this we have to make a decision whether we want to support defaultProps or the consistency check for stateless components.

Copy link
Member Author

Choose a reason for hiding this comment

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

It did indeed work.

I would prefer defaultProps support over consistency checks for implicit SFCs. We could always add a section to the typescript guide.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, are you 100% sure that we can't support both? I haven't looked at this in enough detail to know... but I agree that defaultProps takes precedence if it has to be one or the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not with the never approach. That's either an issue with ts (although I couldn't really find reports on this) or an actual limitation. I tried using the previous approach with ConsistentWith but either ts inferred {} as the prop type or I got errors when passing a styled component to withTheme

I probably revisit the react-redux typings since I used the Matching from there and see if I can connect inconsistent props with their typings too. That way we get more focus on the issue and maybe get some more help.

But as is the issue with #12697 this problem at least has a workaround with explicit typing.

Copy link
Member

@pelotom pelotom Sep 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If we can't do a complete job of enforcing consistency maybe we should just remove the constraints completely, as they significantly complicate the expression of the types, and arguably they're of fairly niche value.

Copy link

Choose a reason for hiding this comment

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

@eps1lon as the original author on the react-redux PR, happy to help out if I can. I haven't gotten any reviews in 20 days so it's honestly been a bit hard to iterate/improve on the definition. If there are edge cases it doesn't handle, I can certainly iterate.

I also have my email in my github profile so feel free to reach out that way.

Copy link

Choose a reason for hiding this comment

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

FWIW, is the issue you're dealing with related to this by any chance DefinitelyTyped/DefinitelyTyped#28249?

Copy link
Member Author

Choose a reason for hiding this comment

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

That issue is probably causing the fail with the explicitly typed SFC. So if that issue is resolved we might not even have a consistency check for explicitly typed SFCs.


// implicit SFC
withStyles(styles)((props: Props) => null); // $ExpectError
withStyles(styles)((props: Props & WithStyles<typeof styles>) => null); // $ExpectError
withStyles(styles)((props: Props & { children?: React.ReactNode }) => null); // $ExpectError
withStyles(styles)(
(props: Props & WithStyles<typeof styles> & { children?: React.ReactNode }) => null, // $ExpectError
);

// explicit not but with "Property 'children' is missing in type 'ValidationMap<Props>'".
// which is not helpful
const StatelessComponent: React.SFC<Props> = props => null;
const StatelessComponentWithStyles: React.SFC<Props & WithStyles<typeof styles>> = props => null;
withStyles(styles)(StatelessComponent); // $ExpectError
withStyles(styles)(StatelessComponentWithStyles); // $ExpectError
}

{
// https://github.com/mui-org/material-ui/issues/12670
interface Props {
nonDefaulted: string;
defaulted: number;
}

class MyButton extends React.Component<Props & WithStyles<typeof styles>> {
static defaultProps = {
defaulted: 0,
};

render() {
const { classes, nonDefaulted, defaulted } = this.props;
return (
<Button className={classes.btn}>
{defaulted}, {nonDefaulted}
</Button>
);
}
}

const styles = () =>
createStyles({
btn: {
color: 'red',
},
});

const StyledMyButton = withStyles(styles)(MyButton);

const CorrectUsage = () => <StyledMyButton nonDefaulted="2" />;
// Property 'nonDefaulted' is missing in type '{}'
const MissingPropUsage = () => <StyledMyButton />; // $ExpectError
}