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

[ListItem] Add type test for button prop #15049

Merged
merged 1 commit into from
Mar 27, 2019

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Mar 25, 2019

Since #13868 button is a discriminant for the props of ListItem. This means it can't be assigned boolean. Either false or true has to be used.

You can't pass the union of the possible types of discriminants in TypeScript. A simplified illustration of this problem:

interface BookProps {
  isBook: 'yes';
}

interface MagazineProps {
  isBook: 'no';
}

interface OverloadedComponent {
  (props: BookProps | MagazineProps): void;
}

declare const SomeComponent: OverloadedComponent;

function Library(props: { isBook: 'yes'} | {isBook: 'no'}) {
  const { isBook } = props;
  SomeComponent(props);
  // This fails because `isBook` is of type `'yes' | 'no'`
  SomeComponent({ isBook });
}

Might be a limitation of TypeScript, might a bug.
/cc @pelotom

@mui-pr-bot
Copy link

No bundle size changes comparing ae31efe...93fc476

Generated by 🚫 dangerJS against 93fc476

@pelotom
Copy link
Member

pelotom commented Mar 26, 2019

Might be a limitation of TypeScript, might a bug.
/cc @pelotom

Yeah, this is pretty unfortunate. Looks like it's an edge case which only comes up when a record type has a single property, and they consider it working as intended, per this comment.

I wonder if we can add another overload to handle the general boolean case or if that destroys all our hard-earned type safety?

@eps1lon
Copy link
Member Author

eps1lon commented Mar 27, 2019

I wonder if we can add another overload to handle the general boolean case or if that destroys all our hard-earned type safety?

I think we're back at the start by that point. With the general boolean case we would need to infer the component with a conditional at which point we might as well completely switch to it. But that approach had issues with type inference in callbacks which we tried to avoid if I recall correctly.

At least we have documented the issue. Maybe we can improve it later.

@eps1lon eps1lon merged commit 48d52e1 into mui:next Mar 27, 2019
@eps1lon eps1lon deleted the test/ListItem/typescript-union-tag-call branch March 27, 2019 08:11
@VincentLanglet
Copy link
Contributor

Might be a limitation of TypeScript, might a bug.

Is this issue reported in Typescript repository ?

@eps1lon
Copy link
Member Author

eps1lon commented Apr 1, 2019

Might be a limitation of TypeScript, might a bug.

Is this issue reported in Typescript repository ?

@VincentLanglet It looks like it.

It essentially comes down to { button: true | false } vs { button: true } | { button: false } as far as I can tell.

@franklixuefei
Copy link
Contributor

franklixuefei commented Oct 9, 2019

Why not just do

<ListItem button={(someBoolean == someOtherBoolean) as false}

:-D
On a side note, I know this looks weird, but at least we don't have to write the if else which could be cumbersome.

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.

6 participants