Skip to content

[core] Type custom onChange implementations with a generic react event#21552

Merged
eps1lon merged 10 commits intomui:nextfrom
eps1lon:test/Tabs/onChange
Jun 24, 2020
Merged

[core] Type custom onChange implementations with a generic react event#21552
eps1lon merged 10 commits intomui:nextfrom
eps1lon:test/Tabs/onChange

Conversation

@eps1lon
Copy link
Member

@eps1lon eps1lon commented Jun 23, 2020

Closes #17454
Closes #20191

-<Component onChange={(event: React.ChangEvent<{}>) => {}} />
+<Component onChange={(event: React.SyntheticEvent) => {}} />

for:

  • Accordion (former ExpansionPanel)
  • BottomNavigation
  • Slider
  • Tabs

These components do not necessarily dispatch a react change event. They only passes whatever event triggered the change in value. I don't want to type it as exact as possible (FocusEvent | ClickEvent) because that restricts our implementation. We simply say that it's any react event at this point. You would have to narrow it later anyway which we can always do later if there are legitimate use cases.

The takeaway from this typing is that event.target is not interchangeable with a ref on the component. event.target could point to any element that the component rendered. It's a bit unfortunate that we overload what is usually considered a change event. We have to make the trade-off at some point: either when passing props (Slider behaves just like any other input: I pass a value and onChange) or when typing the onChange (event.target === event.currentTarget).

Removes onChange types from BottomNavigationAction and Tab. These are overridden anyway and considered private.

About OverrideableComponent changes

Let's say we have a component that implements the component like

function Button({ component: Component, ...other }) {
  return <Component {...other} />;
}

Previously we assumed that any prop that is implemented by Button is also forwarded to the passed component.

However, this is almost never the case e.g. in Tab we expect that the given component either doesn't implement onChange or as the same type.

The reality is that Tab does not forward onChange and therefore component never "sees" onChange.

There are some exceptions e.g. ButtonBase intercepts href but also passes it along. We currently expect the href to be a string but this isn't required if a component is passed.

Instead of expecting the passed component to extend the props of Button we should expect component to implement a separate interface e.g. in

function Button({ component: Component, external, target, ...other }) {
  return <Component target={external ? '_blank' : target} {...other} />;
}

the passed component needs to implement the target prop. It doesn't need to implement external nor should it ever expect to receive an external prop if it is used in <Button />.

This change is a bit scary but safe in next. I'll follow-up in a later PR with more test and experimentation if it makes sense to require component to implement a certain interface.

Future work

  • remove problematic usage of React.ChangeEvent<{}>. It works structurally right now but has some implications that can't be expressed via types yet so we need to be careful with using it.

@eps1lon eps1lon added breaking change Introduces changes that are not backward compatible. scope: tabs Changes related to the tabs. typescript labels Jun 23, 2020
fullWidth?: boolean;
icon?: string | React.ReactElement;
label?: React.ReactNode;
onChange?: (event: React.ChangeEvent<{ checked: boolean }>, value: any) => void;
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 don't know why the target should implement checked. Probably some wrong c&p while refactoring.

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 23, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 4977b6f

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Awesome, I was leaning in the same direction. I think that it could be interesting to add a test case for #20191 too (don't we solve this issue too?).

@eps1lon
Copy link
Member Author

eps1lon commented Jun 24, 2020

don't we solve this issue too?

Yes! Thanks for linking it. It's the same underlying issue.

Will add the same test. I'm also checking out again why our docs were working fine

@eps1lon eps1lon force-pushed the test/Tabs/onChange branch from 5e423d2 to 31f261c Compare June 24, 2020 09:14
@eps1lon eps1lon marked this pull request as draft June 24, 2020 09:17
@eps1lon eps1lon changed the title [Tabs] Type onChange as a generic react event [core] Type custom onChange implementations with a generic react event Jun 24, 2020
@eps1lon eps1lon force-pushed the test/Tabs/onChange branch from 3211703 to 4977b6f Compare June 24, 2020 10:06
@eps1lon eps1lon requested a review from oliviertassinari June 24, 2020 10:54
@eps1lon eps1lon marked this pull request as ready for review June 24, 2020 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Introduces changes that are not backward compatible. scope: tabs Changes related to the tabs. typescript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Slider] onChange Typing is broken if wrapping Slider Component and reexporting Props [Tabs] Issues with onChange TypeScript types

3 participants