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

[typescript] Fix conflicting types for onChange prop #8618

Merged
merged 6 commits into from
Oct 11, 2017
Merged

[typescript] Fix conflicting types for onChange prop #8618

merged 6 commits into from
Oct 11, 2017

Conversation

pelotom
Copy link
Member

@pelotom pelotom commented Oct 10, 2017 •
edited
Loading

Resolves #8617

pelotom
pelotom commented Oct 10, 2017
View reviewed changes
src/index.d.ts
@@ -46,7 +46,7 @@ export interface Color {
*/
export type Diff<T extends string, U extends string> = ({ [P in T]: P } &
{ [P in U]: never } & { [x: string]: never })[T];
export type Omit<T, K extends keyof T> = { [P in Diff<keyof T, K>]: T[P] };
export type Omit<T, K extends keyof T> = Pick<T, Diff<keyof T, K>>;
Copy link
Member Author

@pelotom pelotom Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the more correct typing; it makes it unnecessary to wrap Omit in Partial everywhere it's used. I went ahead and made that change as part of this PR.

@pelotom pelotom force-pushed the fix-textfield-onchange-type-collision branch from f9194b3 to 3ccaa41 Compare October 10, 2017 00:30
@pelotom pelotom changed the title Fix conflicting types for TextField.onChange (resolves #8617) Fix conflicting types for TextField.onChange Oct 10, 2017
@oliviertassinari oliviertassinari changed the title Fix conflicting types for TextField.onChange [typescript] Fix conflicting types for TextField.onChange Oct 10, 2017
@oliviertassinari
Copy link
Member

Should we add some tests for this PR?

@pelotom
Copy link
Member Author

pelotom commented Oct 10, 2017

@oliviertassinari I think the best way to test this is turn on noImplicitAny in the tsconfig.json and then remove type annotations from the existing functions being passed as onChange... I'll look into how painful that is, it's a good setting to have on anyway.

@pelotom
Copy link
Member Author

pelotom commented Oct 10, 2017 •
edited
Loading

When I turn on noImplicitAny it flags this component in components.spec.tsx:

const GridListTest = () =>
  <GridList cellHeight={160} cols={3}>
    <GridListTest cols={1}>
      <img src="img.png" alt="alt text" />
    </GridListTest>,
  </GridList>;

This seems like a bug; GridListTest is referencing itself. If you actually tried to instantiate it it would create a JSX tree of infinite size! What was intended here? I assume the inner element should be another GridList?

pelotom
pelotom commented Oct 10, 2017
View reviewed changes
test/typescript/components.spec.tsx Outdated
<img src="img.png" alt="alt text" />
</GridListTest>,
</GridList>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Turning on noImplicitAny caught this bug.

Copy link
Member

Choose a reason for hiding this comment

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

GridListTile instead

pelotom
pelotom commented Oct 10, 2017
View reviewed changes
src/BottomNavigation/BottomNavigation.d.ts

export type BottomNavigationProps = {
children: React.ReactNode;
onChange?: (event: React.ChangeEvent<{}>, value: any) => void;
showLabels?: boolean;
value?: any;
} & React.HTMLAttributes<HTMLDivElement>;
} & Omit<React.HTMLAttributes<HTMLDivElement>, 'onChange'>;
Copy link
Member Author

Choose a reason for hiding this comment

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

I went ahead and fixed this one too.

@pelotom pelotom changed the title [typescript] Fix conflicting types for TextField.onChange [typescript] Fix conflicting types for onChange prop Oct 10, 2017
sebald
sebald suggested changes Oct 11, 2017
View reviewed changes
src/BottomNavigation/BottomNavigation.d.ts Outdated
@@ -1,12 +1,13 @@
import * as React from 'react';
import { StyledComponent } from '..';
import { Omit } from '../index';
Copy link
Member

Choose a reason for hiding this comment

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

Could you merge this with the above import.

@sebald sebald merged commit 15b2767 into mui:v1-beta Oct 11, 2017
@pelotom
Copy link
Member Author

pelotom commented Oct 11, 2017

@oliviertassinari did you mean to remove the v1 tag from this? I'm not sure if you're using that to generate release notes or something, but this should probably have it.

@oliviertassinari
Copy link
Member

@pelotom I have deleted the v1 tag. Instead, I'm using the legacy tag on the v0.x issues. +80% of the activity is on the v1 version now.

@pelotom pelotom deleted the fix-textfield-onchange-type-collision branch October 11, 2017 18:21
@sebald sebald mentioned this pull request Oct 16, 2017
Closed
the-noob pushed a commit to the-noob/material-ui that referenced this pull request Oct 17, 2017
@pelotom @the-noob
[typescript] Fix conflicting types for onChange prop (mui#8618)
8f286e1 
* Fix conflicting types for TextField.onChange (resolves mui#8617)

* Get rid of Partial made unnecessary by better definition of Omit

* Enable noImplicitAny to test that event handlers infer correct type

* Test that BottomNavigation.onChange has correct type

* Use GridListTile

* Merge named imports
@pelotom pelotom mentioned this pull request Oct 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebald sebald sebald requested changes

@oliviertassinari oliviertassinari oliviertassinari approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@pelotom @oliviertassinari @sebald