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

[styles] Fix typescript throwing in makeStyles with no props required #14019

Merged
merged 2 commits into from
Dec 30, 2018

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Dec 28, 2018

Right now typescript infers Props to any in StyleRulesCallback but to {} in StyleRules if props are not used. I'm not sure if this is actually a typescript bug but it's fixable in our codebase.

Closes #14018

@eps1lon eps1lon added bug 🐛 Something doesn't work typescript package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Dec 28, 2018
@eps1lon eps1lon requested a review from pelotom December 28, 2018 16:28
? string extends T
? true
: false
: false;
Copy link
Member

@pelotom pelotom Dec 28, 2018

Choose a reason for hiding this comment

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

A simpler way to accomplish the same?

export type IsEmptyInterface<T> = {} extends T ? true : false;

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 was my first approach too but that would evaluate to true for object or Record<"a" | "b", string>. The first one isn't that problematic since it has no members either but the second one does.

Copy link
Member

Choose a reason for hiding this comment

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

That was my first approach too but that would evaluate to true for object or Record<"a" | "b", string>. The first one isn't that problematic since it has no members either but the second one does.

Strange, the second one shouldn't be true, and it doesn't seem to be in the latest TS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Funny I did the same but somehow confused the evaluated type. Still true for object though. In that case I would prefer keyof T extends never ? true : false since it communicates intent better than {} extends T IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me.

Copy link
Member

@pelotom pelotom Dec 28, 2018

Choose a reason for hiding this comment

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

Hmm, but that definition would allow undefined, null, void and never...

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. Going to improve coverage and then stick to your approach most likely. object should be fine in most cases.

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 I extended coverage for IsEmptyInterface and tried improving readability. I tried your approach in eps1lon@60007f3 but this doesn't work in its current form since it completely disables type checking for props.

: false
: false;

type Or<A, B> = A extends true ? true : B extends true ? true : false;
Copy link
Member

Choose a reason for hiding this comment

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

We might want to use a library like typelevel-ts instead of reinventing all our own combinators.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely. I would defer that to a different PR though since we have plenty of unrelated candidates for this (Pick, IsAny?) and I want to to have a look at different options. I for one only used utility-types.

@oliviertassinari
Copy link
Member

I would like to do a release tomorrow. Is this pull-request ready to be merged?

@pelotom
Copy link
Member

pelotom commented Dec 29, 2018

Good from my perspective.

@eps1lon eps1lon merged commit f12459e into mui:master Dec 30, 2018
@eps1lon eps1lon deleted the fix/styles/makeStyles/optional-props branch December 30, 2018 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants