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

Jump to bottom

Broken test: declare props separate from usage for Button, MenuItem, ListItem #16315

Closed
wants to merge 2 commits into from
Closed

Broken test: declare props separate from usage for Button, MenuItem, ListItem #16315

wants to merge 2 commits into from

Conversation

rosskevin
Copy link
Member

@rosskevin rosskevin commented Jun 20, 2019 •
edited
Loading

I've discovered a typescript issue when trying to migrate a 3.x codebase to 4.x

On the 4.x master, it seems that declaring/constructing props separate from usage, while it may be valid, fails to typecheck. A representative error is:

  Types of property 'button' are incompatible.
    Type 'boolean | undefined' is not assignable to type 'true | undefined'.
      Type 'false' is not assignable to type 'true | undefined'.

for

const MenuItemTest = () => {
  const p: MenuItemProps = {};
  return <MenuItem {...p} />;
};

@eps1lon @pelotom thoughts?

/issue MenuItem #16245
/issue ListItem #14971
/issue Button #15827

@rosskevin rosskevin requested a review from eps1lon June 20, 2019 16:35
eps1lon
eps1lon reviewed Jun 20, 2019
View reviewed changes
packages/material-ui/test/typescript/components.spec.tsx
@@ -170,6 +172,20 @@ const BottomNavigationTest = () => {
);
};

const ButtonTest = () => {
const p: ButtonProps = {
Copy link
Member

Choose a reason for hiding this comment

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

Does

Suggested change
const p: ButtonProps = {
const p: ButtonProps<'span'> = {

work?

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 doesn't.

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 20, 2019 •
edited
Loading

No bundle size changes comparing 0d4f1b1...29c47d1

Generated by 🚫 dangerJS against 29c47d1

@rosskevin rosskevin mentioned this pull request Jun 20, 2019
Closed
2 tasks
@rosskevin
Copy link
Member Author

I found the MenuItem issue was already reported in #16245

@rosskevin rosskevin mentioned this pull request Jun 20, 2019
Closed
2 tasks
@rosskevin rosskevin changed the title Broken test: declare props separate from usage for Button and MenuItem Broken test: declare props separate from usage for Button, MenuItem, ListItem Jun 20, 2019
@rosskevin
Copy link
Member Author

I have added a test for ListItem - this was reported in #14971

@rosskevin rosskevin mentioned this pull request Jun 21, 2019
Closed
2 tasks
@eps1lon
Copy link
Member

eps1lon commented Jul 10, 2019

Closing in favor of the existing issues.

@eps1lon eps1lon closed this Jul 10, 2019
@rosskevin rosskevin mentioned this pull request Dec 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eps1lon eps1lon eps1lon left review comments

Assignees
No one assigned
Labels
typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rosskevin @mui-pr-bot @eps1lon