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 9 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
12 changes: 12 additions & 0 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 @@ -64,6 +64,18 @@ export type ConsistentWith<T, U> = Pick<U, keyof T & keyof U>;
*/
export type Overwrite<T, U> = Omit<T, keyof U> & U;

/**
* a property P will will have the the type of:
* - DecorationTargetProps if it is not present in InjectedProps
* - InjectedTargetProps if it is present in DecorationTargetProps
*
*/
export type Matching<InjectedProps, DecorationTargetProps> = {
[P in keyof DecorationTargetProps]: P extends keyof InjectedProps
? InjectedProps[P]
Copy link

@apapirovski apapirovski Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This no longer validates that the two interfaces passed in are matching. If InjectedProps[P] doesn't extend DecorationTargetProps[P] (assuming their keys are shared) then that means that the two definitions are incompatible and cannot be reconciled. E.g., this would accept: Matching<{ test: string; }, { test: boolean; }>

Edit: actually, you can ignore me, I think I see what you're doing? Maybe... Need to test it locally.
Edit2: Nope, this is definitely incorrect but not for the reason I mentioned. The problem is that if your component declares something as optional and the InjectedProps spec something as required then it won't match.

Copy link
Member Author

Choose a reason for hiding this comment

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

@apapirovski
Which is sufficient for usage in withStyles. The previous version allowed mismatch of props for functional components that did not have the React.SFC type or to be more specific only had a call signature. I'm going to checkout the react/redux typings with your PR and see if this happens there to.

Copy link

@apapirovski apapirovski Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I posted a new definition for Matching in my PR that is IMO slightly more correct, at least as far as connect goes. Not sure if you have different requirements here as I don't use material-ui. Thanks for the test case and the triage!

Copy link
Member Author

Choose a reason for hiding this comment

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

@apapirovski
I agree that approach is indeed better and I tried this before but this caused some regression. I realized that this was not an issue with your definition but with ours. Basically injecting T | undefined while we actually only inject T if it is defined which is different when using spread syntax.

: DecorationTargetProps[P]
};
Copy link
Member

Choose a reason for hiding this comment

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

It seems like Matching is meant to accomplish much the same as ConsistentWith, but maybe does a better job? Do we still need ConsistentWith or can Matching replace it everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so too but ConsistentWith did not help with this issue. Maybe replace ConsistentWith in a future PR with Matching or explore where they actually differ. Same goes for AnyComponent.


export namespace PropTypes {
type Alignment = 'inherit' | 'left' | 'center' | 'right' | 'justify';
type Color = 'inherit' | 'primary' | 'secondary' | 'default';
Expand Down
16 changes: 12 additions & 4 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,6 @@
import * as React from 'react';
import { WithTheme } from '../styles/withTheme';
import { AnyComponent, ConsistentWith, Overwrite, Omit } from '..';
import { Matching, Omit, PropsOf } from '..';
import { Theme } from './createMuiTheme';
import * as CSS from 'csstype';
import * as JSS from 'jss';
Expand Down Expand Up @@ -60,6 +60,14 @@ 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>>>;
): <
C extends React.ComponentType<Matching<WithStyles<ClassKey, Options['withTheme']>, PropsOf<C>>>
>(
component: C,
) => React.ComponentType<
Omit<
JSX.LibraryManagedAttributes<C, PropsOf<C>>,
keyof WithStyles<ClassKey, Options['withTheme']>
> &
Copy link
Member

Choose a reason for hiding this comment

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

We only ever use keyof Shared<...>? This makes me think we can get rid of Shared completely and just use

Omit<
  JSX.LibraryManagedAttributes<C, PropsOf<C>>,
  (keyof WithStyles<ClassKey, Options['withTheme']>) & (keyof PropsOf<C>)
>

here. Furthermore the & (keyof PropsOf<C>) seems redundant, and it can just be

Omit<
  JSX.LibraryManagedAttributes<C, PropsOf<C>>,
  keyof WithStyles<ClassKey, Options['withTheme']>
>

StyledComponentProps<ClassKey>
>;
81 changes: 72 additions & 9 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 { Matching, 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 @@ -170,7 +170,7 @@ const ComponentWithTheme = withTheme()(({ theme }) => <div>{theme.spacing.unit}<
type AllTheProps = WithTheme & WithStyles<typeof styles>;

const AllTheComposition = withTheme()(
withStyles(styles)(({ theme, classes }: AllTheProps) => (
withStyles(styles, { withTheme: true })(({ theme, classes }: AllTheProps) => (
<div className={classes.root}>{theme.palette.text.primary}</div>
)),
);
Expand Down Expand Up @@ -201,7 +201,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 +292,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 +312,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 +384,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
}