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] Enable generic props for certain components #13868

Merged
merged 60 commits into from
Feb 10, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
3d67416
WIP
pelotom Aug 13, 2018
9ba3309
Merge branch 'master' into typescript-generics-new-approach
pelotom Aug 14, 2018
070df8f
WIP
pelotom Aug 22, 2018
dbf061b
Upgrade TypeScript
pelotom Dec 8, 2018
3e65e4e
Rename defaultRoot to defaultComponent
pelotom Dec 8, 2018
6a5f2a4
Fix InnerProps operator
pelotom Dec 9, 2018
66f1885
Add comments to type tests
pelotom Dec 9, 2018
bc82b64
Add another test
pelotom Dec 9, 2018
29c8771
Remove unnecessary generic
pelotom Dec 9, 2018
2f3d066
Format comment
pelotom Dec 9, 2018
d2f84ea
Tweak comment
pelotom Dec 9, 2018
a1de8af
Use PropsOf definition from master
pelotom Dec 9, 2018
95f1ff3
Move AnyReactType to muiComponent
pelotom Dec 9, 2018
a24d8af
Renames
pelotom Dec 9, 2018
8935391
Merge branch 'master' into typescript-generics-new-approach
pelotom Dec 9, 2018
e68aae8
Upgrade TypeScript
pelotom Dec 9, 2018
95ad1ad
Comment out test that doesn't work in TS 3.2
pelotom Dec 9, 2018
0ad90c1
Fix some errors
pelotom Dec 9, 2018
e2470e8
Merge branch 'upgrade-ts' into typescript-generics-new-approach
pelotom Dec 9, 2018
2dd8b56
Use SimplifiedPropsOf to obtain exported Props type for each component
pelotom Dec 9, 2018
e6acb1d
Add error expectations
pelotom Dec 10, 2018
fc402c4
Test callback arg types
pelotom Dec 10, 2018
4077fee
Fix derivatives of ButtonBase
pelotom Dec 10, 2018
cfe50c2
Make an ExtendButtonBase utility
pelotom Dec 10, 2018
b58e667
Merge branch 'master' into typescript-generics-new-approach
pelotom Dec 10, 2018
2664864
Run prettier
pelotom Dec 10, 2018
ebee33f
Fix error assertion
pelotom Dec 10, 2018
88af2dc
Make prettier happy
pelotom Dec 10, 2018
84f4391
Simplify ButtonBase
pelotom Dec 10, 2018
e78f507
Move className and styles into StyledComponentProps
pelotom Dec 10, 2018
a458d46
Rename outerProps to props
pelotom Dec 10, 2018
cc5c311
Get rid of innerProps concept which is too complex for the value it p…
pelotom Dec 10, 2018
fe20e86
Remove non-useful test
pelotom Dec 13, 2018
4ab8653
Merge branch 'next' into typescript-generics-new-approach
pelotom Jan 12, 2019
edd1c3c
Fix type expectations for new version of @types/react
pelotom Jan 12, 2019
45a2b20
Revert "Move className and styles into StyledComponentProps"
pelotom Jan 12, 2019
5d3e3fe
Refine types of ListItem and MenuItem
pelotom Jan 13, 2019
e978006
Run Prettier
pelotom Jan 13, 2019
367929e
Add ref type tests
pelotom Jan 16, 2019
9b7fc1e
Add ref tests to OverridableComponent.spec.tsx
pelotom Jan 16, 2019
54f82e6
Handle class component refs
pelotom Jan 17, 2019
065c79d
Remove useless shouldSucceed / shouldFail variable assignments
pelotom Jan 17, 2019
825ffe8
More ref testing
pelotom Jan 17, 2019
620e6ce
Prettier
pelotom Jan 17, 2019
b387eda
Replace AnyReactType with React.ReactType
pelotom Jan 17, 2019
32a11d0
Prettier
pelotom Jan 17, 2019
ff72ec3
Merge branch 'next' into typescript-generics-new-approach
pelotom Jan 20, 2019
35d63df
Merge branch 'master' into pr/pelotom/13868
eps1lon Feb 1, 2019
a7f6d4c
[lint] Re-enable no-unecessary-callbackwrapper
eps1lon Feb 5, 2019
4f6dafb
[typescript] document via test how implicit any can be resolved
eps1lon Feb 5, 2019
f5ccac1
[typescript] Drop PropsOf in favor of React.ComponentProps
eps1lon Feb 5, 2019
1a179f6
[test] Move var declaration closer to callsite
eps1lon Feb 5, 2019
c080e5f
Merge branch 'next' into pr/pelotom/13868
eps1lon Feb 5, 2019
b5edd2a
[chore] format
eps1lon Feb 5, 2019
752c8e6
[typescript] add some code docs to overrideable component types
eps1lon Feb 5, 2019
6b95cbf
Merge branch 'next' into pr/pelotom/13868
eps1lon Feb 8, 2019
06a7bf5
[typescript] explain ExtendButtonBase
eps1lon Feb 8, 2019
e5990ed
[core] hoist dev dependencies
eps1lon Feb 8, 2019
b02652f
Allow Simplify to distribute over unions
pelotom Feb 9, 2019
79c4872
Revert "[lint] Re-enable no-unecessary-callbackwrapper"
pelotom Feb 9, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 30 additions & 53 deletions packages/material-ui/test/typescript/components.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ const AvatarTest = () => (
<Avatar
component="button"
ref={(elem: HTMLButtonElement) => {}}
onClick={(e: React.MouseEvent<HTMLButtonElement>) => log(e)}
onClick={log}
alt="Image Alt"
src="example.jpg"
/>
Expand Down Expand Up @@ -163,7 +163,7 @@ const BottomNavigationTest = () => {
const value = 123;

return (
<BottomNavigation value={value} onChange={e => log(e)} showLabels>
Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of writing these as lambdas was to ensure that the argument type was being inferred...

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it complain that log is not assignable to (e: EventType) => ReturnType in those cases? What makes this different from the cases where you added an explicit ExpectType?

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't it complain that log is not assignable to (e: EventType) => ReturnType in those cases?

Perhaps it should, but it doesn't.

What makes this different from the cases where you added an explicit ExpectType?

It's a less rigorous test (just that something other than any was inferred for the argument, not making an assertion about what was inferred). There's no reason an ExpectType couldn't be added also.

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 quickly push a test that is not throwing when it should for my own peace of mind? Just want to make sure we're not disabling some lint rule that was removed because of something unrelated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I don't think I follow. Could you elaborate?

Copy link
Member Author

@pelotom pelotom Feb 9, 2019

Choose a reason for hiding this comment

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

That doesn’t test inference because you’re passing a function whose signature is already determined. The point is to pass an inline function with no parameter signature so that if it can’t infer the type there will be a noImplicitAny error.

Copy link
Member

Choose a reason for hiding this comment

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

The point is to pass an inline function with no parameter signature so that if it can’t infer the type there will be a no-implicit-any error.

Ah now I understand. Happy to revert a7f6d4c then.

Copy link
Member Author

@pelotom pelotom Feb 9, 2019

Choose a reason for hiding this comment

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

If we didn’t want to disable the lint rule we could just as well pass e => {} to test this, the log call is unimportant... but IIRC the only reason for it was to appease another lint rule for unused variables.

Probably should just selectively disable the lint rule for these lines.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with disabling the rule since our ts files never make it to runtime anyway. I would say that the only useful pattern the lint rule flags is in react props. A new callback could defeat memoized components.

The ts files in our docs are transpiled to JS at which point eslint would cover that case.

If end up disabling the rules for every instance of unnecessary callbacks then we might as well disable it globally (or maybe starting with per file disables)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I reverted 79c4872.

<BottomNavigation value={value} onChange={log} showLabels>
<BottomNavigationAction label="Recents" icon={<FakeIcon />} />
<BottomNavigationAction label="Favorites" />
<BottomNavigationAction label={<span>Nearby</span>} icon={<FakeIcon />} />
Expand Down Expand Up @@ -345,30 +345,30 @@ const CardMediaTest = () => (
const ChipsTest = () => (
<div>
<Chip label="Basic Chip" />
<Chip avatar={<Avatar>MB</Avatar>} label="Clickable Chip" onClick={e => log(e)} />
<Chip avatar={<Avatar src={'image.bmp'} />} label="Deletable Chip" onDelete={e => log(e)} />
<Chip avatar={<Avatar>MB</Avatar>} label="Clickable Chip" onClick={log} />
<Chip avatar={<Avatar src={'image.bmp'} />} label="Deletable Chip" onDelete={log} />
<Chip
avatar={
<Avatar>
<FakeIcon />
</Avatar>
}
label="Clickable Deletable Chip"
onClick={e => log(e)}
onDelete={e => log(e)}
onClick={log}
onDelete={log}
/>
</div>
);

const DialogTest = () => {
const emails = ['[email protected]', '[email protected]'];
return (
<Dialog onClose={e => log(e)} open>
<Dialog onClose={log} open>
<DialogTitle>Set backup account</DialogTitle>
<div>
<List>
{emails.map(email => (
<ListItem button onClick={e => log(e)} key={email}>
<ListItem button onClick={log} key={email}>
<ListItemAvatar>
<Avatar>
<FakeIcon />
Expand Down Expand Up @@ -457,37 +457,25 @@ const DrawerTest = () => {
};
return (
<div>
<Drawer variant="persistent" open={open.left} onClose={e => log(e)} onClick={e => log(e)}>
<Drawer variant="persistent" open={open.left} onClose={log} onClick={log}>
List
</Drawer>
<Drawer
variant="temporary"
anchor="top"
open={open.top}
onClose={e => log(e)}
onClick={e => log(e)}
onClose={log}
onClick={log}
ModalProps={{
hideBackdrop: true,
}}
>
List
</Drawer>
<Drawer
anchor="bottom"
variant="temporary"
open={open.bottom}
onClose={e => log(e)}
onClick={e => log(e)}
>
<Drawer anchor="bottom" variant="temporary" open={open.bottom} onClose={log} onClick={log}>
List
</Drawer>
<Drawer
variant="persistent"
anchor="right"
open={open.right}
onClose={e => log(e)}
onClick={e => log(e)}
>
<Drawer variant="persistent" anchor="right" open={open.right} onClose={log} onClick={log}>
List
</Drawer>
</div>
Expand All @@ -503,42 +491,31 @@ const SwipeableDrawerTest = () => {
};
return (
<div>
<SwipeableDrawer
open={open.left}
onClose={e => log(e)}
onClick={e => log(e)}
onOpen={e => log(e)}
>
<SwipeableDrawer open={open.left} onClose={log} onClick={log} onOpen={log}>
List
</SwipeableDrawer>
<SwipeableDrawer
anchor="top"
open={open.top}
onClose={e => log(e)}
onClick={e => log(e)}
onOpen={e => log(e)}
onClose={log}
onClick={log}
onOpen={log}
ModalProps={{
hideBackdrop: true,
}}
>
List
</SwipeableDrawer>
<SwipeableDrawer
anchor="bottom"
open={open.bottom}
onClose={e => log(e)}
onClick={e => log(e)}
onOpen={e => log(e)}
>
<SwipeableDrawer anchor="bottom" open={open.bottom} onClose={log} onClick={log} onOpen={log}>
List
</SwipeableDrawer>
<SwipeableDrawer
variant="temporary"
anchor="right"
open={open.right}
onClose={e => log(e)}
onClick={e => log(e)}
onOpen={e => log(e)}
onClose={log}
onClick={log}
onOpen={log}
>
List
</SwipeableDrawer>
Expand All @@ -548,7 +525,7 @@ const SwipeableDrawerTest = () => {

const ExpansionPanelTest = () => (
<div>
<ExpansionPanel onChange={e => log(e)} expanded disabled>
<ExpansionPanel onChange={log} expanded disabled>
<ExpansionPanelSummary />
<ExpansionPanelDetails />
</ExpansionPanel>
Expand Down Expand Up @@ -584,8 +561,8 @@ const GridTest = () => (
);

const GridListTest = () => (
<GridList cellHeight={160} cols={3} onClick={e => log(e)}>
<GridListTile cols={1} rows={4} onClick={e => log(e)}>
<GridList cellHeight={160} cols={3} onClick={log}>
<GridListTile cols={1} rows={4} onClick={log}>
<img src="img.png" alt="alt text" />
</GridListTile>
,
Expand All @@ -595,7 +572,7 @@ const GridListTest = () => (
const ListTest = () => (
<List>
{[0, 1, 2, 3].map(value => (
<ListItem dense button selected={false} key={value} onClick={e => log(e)}>
<ListItem dense button selected={false} key={value} onClick={log}>
<Checkbox checked={true} tabIndex={-1} disableRipple />
<ListItemText primary={`Line item ${value + 1}`} />
<ListItemSecondaryAction>
Expand Down Expand Up @@ -624,7 +601,7 @@ const MenuTest = () => {
id="lock-menu"
anchorEl={anchorEl}
open={true}
onClose={e => log(e)}
onClose={log}
PopoverClasses={{ paper: 'foo' }}
>
{options.map((option, index) => (
Expand Down Expand Up @@ -747,15 +724,15 @@ const SwitchTest = () => {

const SnackbarTest = () => (
<div>
<Button onClick={e => log(e)}>Open simple snackbar</Button>
<Button onClick={log}>Open simple snackbar</Button>
<Snackbar
anchorOrigin={{
vertical: 'bottom',
horizontal: 'left',
}}
open={true}
autoHideDuration={6e3}
onClose={e => log(e)}
onClose={log}
ContentProps={
{
// 'aria-describedby': 'message-id',
Expand All @@ -764,10 +741,10 @@ const SnackbarTest = () => (
}
message={<span id="message-id">Note archived</span>}
action={[
<Button key="undo" color="secondary" size="small" onClick={e => log(e)}>
<Button key="undo" color="secondary" size="small" onClick={log}>
UNDO
</Button>,
<IconButton key="close" aria-label="Close" color="inherit" onClick={e => log(e)}>
<IconButton key="close" aria-label="Close" color="inherit" onClick={log}>
<FakeIcon />
</IconButton>,
]}
Expand Down
1 change: 0 additions & 1 deletion tslint.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
"jsRules": {},
"rules": {
"file-name-casing": false,
"no-unnecessary-callback-wrapper": false,
"no-unnecessary-generics": false,
"semicolon": [true, "always", "ignore-bound-class-methods"]
}
Expand Down