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] Fix withStyles edge cases, require TS 2.8 #11280

Merged
merged 9 commits into from
May 8, 2018

Conversation

pelotom
Copy link
Member

@pelotom pelotom commented May 8, 2018

Fixes #11109.
Fixes #11191.

This employs conditional types to define an Overwrite type operator which resolves the edge cases around withStyles, without destroying the integrity of props that are specified as a union. This means requiring a minimum TypeScript version of 2.8.

@pelotom pelotom changed the title Fix with styles Fix withStyles edge cases May 8, 2018
@pelotom pelotom changed the title Fix withStyles edge cases [typescript] Fix withStyles edge cases May 8, 2018
@estaub
Copy link
Contributor

estaub commented May 8, 2018

I took it around the block with all of my own code. All looks good!

You might want to document Overwrite a bit; it takes some staring at it and examples to understand. Something like:

Overwrite takes a type T and a "mixin" type U, removes any properties from T that would conflict with properties in U, and returns the combined type T&U. When T is equivalent to U, it just returns T.

@pelotom
Copy link
Member Author

pelotom commented May 8, 2018

It seems like the stakeholders from the issues that this resolves are all in favor of it, so I'll merge. We'll want to mention in the release notes that TypeScript 2.8 is required now.

@pelotom pelotom changed the title [typescript] Fix withStyles edge cases [typescript] Fix withStyles edge cases, require TS 2.8 May 8, 2018
@pelotom pelotom merged commit e824afe into mui:v1-beta May 8, 2018
@oliviertassinari
Copy link
Member

@pelotom Oops, just after the release train 🚂.

@pelotom pelotom deleted the fix-withStyles branch May 8, 2018 22:04
@pelotom
Copy link
Member Author

pelotom commented May 8, 2018

@oliviertassinari 😩

@oliviertassinari
Copy link
Member

The next one will be this weekend :)

@semiadam
Copy link

semiadam commented May 8, 2018

@pelotom will this fix all the compile errors introduced around withStyle after beta.43? My code had a comfortable life before 43 :)

@pelotom
Copy link
Member Author

pelotom commented May 8, 2018

@semiadam with any luck 🙂

@olee
Copy link
Contributor

olee commented May 10, 2018

@semiadam you can import the following fix instead of material-ui/styles to get around the compile errors:

import withStylesOld from 'material-ui/styles/withStyles';

export * from 'material-ui/styles';

import { WithStyles, StyleRules, StyleRulesCallback, StyledComponentProps } from 'material-ui/styles';
import { WithStylesOptions } from 'material-ui/styles/withStyles';

type ConsistentWith<O> = Partial<O> & Record<string, any>;
declare function WithStylesFixed<ClassKey extends string>(
    style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
    options?: WithStylesOptions,
): {
    <P extends ConsistentWith<StyledComponentProps<ClassKey>>>
    (component: React.ComponentType<P & WithStyles<ClassKey>>): React.ComponentType<P & StyledComponentProps<ClassKey>>;
    (component: React.ComponentType<WithStyles<ClassKey>>): React.ComponentType<StyledComponentProps<ClassKey>>;
};

export const withStyles =  withStylesOld as typeof WithStylesFixed;

@estaub
Copy link
Contributor

estaub commented May 10, 2018

@olee Do you know of some problem with @pelotom's fix for this? IIt might be more productive if folks tried that. Flipping the overloads just causes problems in other cases. If you want to see, try patching a copy of material-ui with your declaration, npm run typescript, and see what breaks.

@estaub
Copy link
Contributor

estaub commented May 10, 2018

@pelotom In your copious spare time, you might want to consider adding test cases for the issues that were posted due to my PR, either in this PR or another.

@pelotom
Copy link
Member Author

pelotom commented May 10, 2018

@pelotom In your copious spare time, you might want to consider adding test cases for the issues that were posted due to my PR, either in this PR or another.

I did, in this PR... were there others that I missed?

@estaub
Copy link
Contributor

estaub commented May 10, 2018

@pelotom Sorry, no need for more AFAIK. I looked for them in the diffs, don't understand how I missed them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants