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] Constrain props type param appropriately in withStyles, withTheme, withWidth HOCs #11003

Merged
merged 17 commits into from
Apr 17, 2018
Merged
Show file tree
Hide file tree
Changes from 9 commits
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
50 changes: 6 additions & 44 deletions docs/src/pages/guides/typescript/typescript.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Have a look at the [Create React App with TypeScript](https://github.com/mui-org
The usage of `withStyles` in TypeScript can be a little tricky, so it's worth showing some examples. You can first call `withStyles()` to create a decorator function, like so:

```js
const decorate = withStyles(({ palette, spacing }) => ({
const withMyStyles = withStyles(({ palette, spacing }) => ({
Copy link
Member

Choose a reason for hiding this comment

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

Please don't rename decorate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

root: {
padding: spacing.unit,
backgroundColor: palette.background.default,
Expand All @@ -30,7 +30,7 @@ interface Props {
Functional components are straightforward:

```jsx
const DecoratedSFC = decorate<Props>(({ text, type, color, classes }) => (
const DecoratedSFC = withMyStyles<Props>(({ text, type, color, classes }) => (
<Typography variant={type} color={color} classes={classes}>
{text}
</Typography>
Expand All @@ -42,7 +42,7 @@ Class components are a little more cumbersome. Due to a [current limitation in T
```jsx
import { WithStyles } from 'material-ui/styles';

const DecoratedClass = decorate(
const DecoratedClass = withMyStyles(
class extends React.Component<Props & WithStyles<'root'>> {
render() {
const { text, type, color, classes } = this.props
Expand All @@ -56,45 +56,8 @@ const DecoratedClass = decorate(
);
```

Note that in the class example you didn't need to annotate `<Props>` in the call to `decorate`; type inference took care of everything. However, there are 2 scenarios where you _do_ need to provide an explicit type argument to `decorate`.

Scenario 1: your styled component takes _no_ additional props in addition to `classes`. The natural thing would be to write:

```jsx
import { WithStyles } from 'material-ui/styles';

const DecoratedNoProps = decorate(
class extends React.Component<WithStyles<'root'>> {
render() {
return (
<Typography classes={this.props.classes}>
Hello, World!
</Typography>
);
}
}
);
```

Unfortunately, TypeScript infers the wrong type in this case and you'll have trouble when you go to make an element of this component. In this case, you'll need to provide an explicit `{}` type argument, like so:

```jsx
import { WithStyles } from 'material-ui/styles';

const DecoratedNoProps = decorate<{}>( // <-- note the type argument!
class extends React.Component<WithStyles<'root'>> {
render() {
return (
<Typography classes={this.props.classes}>
Hello, World!
</Typography>
);
}
}
);
```

Scenario 2: `Props` is a union type. Again, to avoid getting a compiler error, you'll need to provide an explict type argument:
When your `props` are a union, Typescript needs you to explicitly tell it the type,
by providing an explicit generic `<Props>` parameter to `withMyStyles`:

```jsx
import { WithStyles } from 'material-ui/styles';
Expand All @@ -111,7 +74,7 @@ interface Painting {

type Props = Book | Painting;

const DecoratedUnionProps = decorate<Props>( // <-- without the type argument, we'd get a compiler error!
const DecoratedUnionProps = withMyStyles<Props>( // <-- without the type argument, we'd get a compiler error!
class extends React.Component<Props & WithStyles<'root'>> {
render() {
const props = this.props;
Expand All @@ -125,7 +88,6 @@ const DecoratedUnionProps = decorate<Props>( // <-- without the type argument, w
);
```

To avoid worrying about these 2 edge cases, it may be a good habit to always provide an explicit type argument to `decorate`.

Copy link
Member

@pelotom pelotom Apr 16, 2018

Choose a reason for hiding this comment

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

Discussion of the second (now only) scenario needs to be restored here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think not, if the compiler version can be moved forward. See below.

### Injecting Multiple Classes

Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"test:coverage:html": "cross-env NODE_ENV=test BABEL_ENV=coverage nyc mocha test/**/*.test.js src/{,**/}*.test.js && nyc report --reporter=html",
"test:karma": "cross-env NODE_ENV=test karma start test/karma.conf.js --single-run",
"test:regressions": "webpack --config test/regressions/webpack.config.js && rimraf test/regressions/screenshots/chrome/* && vrtest run --config test/vrtest.config.js --record",
"typescript": "tsc -p tsconfig.json",
"typescript": "tsc -v && tsc -p tsconfig.json",
Copy link
Member

Choose a reason for hiding this comment

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

Please revert.

"flow": "flow --show-all-errors",
"argos": "argos upload test/regressions/screenshots/chrome --token $ARGOS_TOKEN || true",
"prebuild": "rimraf build",
Expand Down Expand Up @@ -187,7 +187,7 @@
"rimraf": "^2.6.2",
"sinon": "^4.1.2",
"size-limit": "0.14.1",
"typescript": "^2.6.1",
"typescript": "2.6.2",
Copy link
Member

Choose a reason for hiding this comment

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

Please revert.

Copy link
Contributor Author

@estaub estaub Apr 16, 2018

Choose a reason for hiding this comment

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

Ok, but again, it doesn't compile in 2.6.1. Do you want me to remove the fragment from the test?

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't matter, the yarn.lock is what controls the actual version this project depends on, and it did point to 2.8.1 until you changed it. There should be no changes to either package.json or yarn.lock in this PR.

"url-loader": "^1.0.1",
"vrtest": "^0.2.0",
"webfontloader": "^1.6.28",
Expand Down
3 changes: 3 additions & 0 deletions src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ type Diff<T extends string, U extends string> = ({ [P in T]: P } &
/** @internal */
export type Omit<T, K extends keyof T> = Pick<T, Diff<keyof T, K>>;

/** @internal */
export type ConsistentWith<O> = Partial<O> & Record<string, any>;

export namespace PropTypes {
type Alignment = 'inherit' | 'left' | 'center' | 'right' | 'justify';
type Color = 'inherit' | 'primary' | 'secondary' | 'default';
Expand Down
16 changes: 11 additions & 5 deletions src/styles/withStyles.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import * as React from 'react';
import { WithTheme } from '../styles/withTheme';
import { Omit, ConsistentWith } from '..';
Copy link
Member

Choose a reason for hiding this comment

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

The Omit import is no longer necessary so it should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

import { Theme } from './createMuiTheme';

/**
Expand Down Expand Up @@ -28,9 +30,8 @@ export interface WithStylesOptions {

export type ClassNameMap<ClassKey extends string = string> = Record<ClassKey, string>;

export interface WithStyles<ClassKey extends string = string> {
export interface WithStyles<ClassKey extends string = string> extends Partial<WithTheme> {
classes: ClassNameMap<ClassKey>;
theme?: Theme;
}

export interface StyledComponentProps<ClassKey extends string = string> {
Expand All @@ -41,6 +42,11 @@ export interface StyledComponentProps<ClassKey extends string = string> {
export default function withStyles<ClassKey extends string>(
style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
options?: WithStylesOptions,
): <P>(
component: React.ComponentType<P & WithStyles<ClassKey>>,
) => React.ComponentType<P & StyledComponentProps<ClassKey>>;
): {
(
component: React.ComponentType<WithStyles<ClassKey>>,
): React.ComponentType<StyledComponentProps<ClassKey>>;
<P extends ConsistentWith<WithStyles<ClassKey>>>(
component: React.ComponentType<P & WithStyles<ClassKey>>,
): React.ComponentType<P & StyledComponentProps<ClassKey>>;
}
Copy link
Member

@pelotom pelotom Apr 14, 2018

Choose a reason for hiding this comment

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

How about making a reusable version of this,

type ConsistentWith<O> = Partial<O> & Record<string, any>;

which can then be used for withStyles, withWidth and withTheme:

P extends ConsistentWith<WithStyles<ClassKey>>
P extends ConsistentWith<WithWidthProps>
P extends ConsistentWith<WithTheme>

Copy link
Member

Choose a reason for hiding this comment

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

Missing semicolon

3 changes: 2 additions & 1 deletion src/styles/withTheme.d.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { Theme } from './createMuiTheme';
import { Omit, ConsistentWith } from '..';

export interface WithTheme {
theme: Theme;
}

declare const withTheme: () => <P>(
declare const withTheme: () => <P extends ConsistentWith<WithTheme>>(
component: React.ComponentType<P & WithTheme>,
) => React.ComponentClass<P>;

Expand Down
3 changes: 2 additions & 1 deletion src/utils/withWidth.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Breakpoint } from '../styles/createBreakpoints';
import { Omit, ConsistentWith } from '..';

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

export default function withWidth(
options?: WithWidthOptions,
): <P>(
): <P extends ConsistentWith<WithWidthProps>>(
component: React.ComponentType<P & WithWidthProps>,
) => React.ComponentClass<P & Partial<WithWidthProps>>;
7 changes: 7 additions & 0 deletions src/utils/withWidth.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,10 @@ export class Hello extends React.Component<IHelloProps & WithWidthProps & WithSt
const Decorated = withWidth()(withStyles(styles)(Hello));

<Decorated name="Bob" />;

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

<WidthSFC name="Hortense" />;
2 changes: 1 addition & 1 deletion test/typescript/components.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ const TableTest = () => {
);
}

return withStyles(styles)<{}>(BasicTable);
return withStyles(styles)(BasicTable);
};

const TabsTest = () => {
Expand Down
2 changes: 1 addition & 1 deletion test/typescript/styles.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const ComponentWithChildren: React.SFC<WithStyles<ComponentClassNames>> = ({
children,
}) => <div className={classes.root}>{children}</div>;

const StyledExampleThree = withStyles(styleRule)<{}>(ComponentWithChildren);
const StyledExampleThree = withStyles(styleRule)(ComponentWithChildren);
<StyledExampleThree />;

// Also works with a plain object
Expand Down
33 changes: 32 additions & 1 deletion test/typescript/styling-comparison.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as React from 'react';
import { Omit } from '../../src';
import Typography, { TypographyProps } from '../../src/Typography/Typography';
import { withStyles, WithStyles } from '../../src/styles';

Expand Down Expand Up @@ -35,7 +36,7 @@ const DecoratedClass = decorate(
},
);

const DecoratedNoProps = decorate<{}>(
const DecoratedNoProps = decorate(
class extends React.Component<WithStyles<'root'>> {
render() {
return <Typography classes={this.props.classes}>Hello, World!</Typography>;
Expand All @@ -46,3 +47,33 @@ const DecoratedNoProps = decorate<{}>(
const sfcElem = <DecoratedSFC text="Hello, World!" variant="title" color="secondary" />;
const classElem = <DecoratedClass text="Hello, World!" variant="title" color="secondary" />;
const noPropsElem = <DecoratedNoProps />;

// This is the "scenario 2" example straight from the doc, then invoked:
Copy link
Member

Choose a reason for hiding this comment

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

There is no scenario 2 now, maybe just provide a link or else delete the comment.


interface Book {
category: 'book';
author: string;
}

interface Painting {
category: 'painting';
artist: string;
}

type ArtProps = Book | Painting;

const DecoratedUnionProps = decorate<ArtProps>( // <-- without the type argument, we'd get a compiler error!
class extends React.Component<ArtProps & WithStyles<'root'>> {
render() {
const props = this.props;
return (
<Typography classes={props.classes}>
{props.category === "book" ? props.author : props.artist}
Copy link
Member

Choose a reason for hiding this comment

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

Please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done, but what is the intended preference?
Both are used in that file, roughly 50/50

Copy link
Member

Choose a reason for hiding this comment

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

My immediate preference is to keep irrelevant changes out of this PR 🙂

</Typography>
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

);

const unionPropElem = <DecoratedUnionProps category="book" author="Twain, Mark" />;

6 changes: 3 additions & 3 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -10173,9 +10173,9 @@ typedarray@^0.0.6, typedarray@~0.0.5:
version "0.0.6"
resolved "https://registry.yarnpkg.com/typedarray/-/typedarray-0.0.6.tgz#867ac74e3864187b1d3d47d996a78ec5c8830777"

typescript@^2.6.1:
version "2.8.1"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.8.1.tgz#6160e4f8f195d5ba81d4876f9c0cc1fbc0820624"
typescript@2.6.2:
version "2.6.2"
resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.6.2.tgz#3c5b6fd7f6de0914269027f03c0946758f7673a4"
Copy link
Member

Choose a reason for hiding this comment

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

Please revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


ua-parser-js@^0.7.9:
version "0.7.17"
Expand Down