Material-ui: [Slider] onChange Typing is broken if wrapping Slider Component and reexporting Props

[riceboyler]

Since @material-ui/core v4.9.5, the Slider component appears to not be wrappable due to the change in the Typings, especially for the onChange prop.

• [x] The issue is present in the latest release.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When attempting to wrap and re-export a Slider component, the onChange prop now appears to be typed as ((event: React.ChangeEvent<{}>, value: number | number[]) => void) & ((event: React.FormEvent<HTMLSpanElement>) => void). The latter part of that Typing (the & ((event: React.FormEvent<HTMLSpanElement>) => void) effectively breaks any function being passed into the onChange prop, as it would seem that a function can't satisfy both types here. As a result, there are TypeScript errors with onChange.

Expected Behavior 🤔

I believe the typing for onChange should be just the part preceding the &, that is the (event: React.ChangeEvent<{}>, value: number | number[]) => void. That would allow for standard change handlers to be passed through.

Steps to Reproduce 🕹

https://codesandbox.io/s/goofy-chatterjee-1hz7t

Steps:

1. Create a new component that wraps and implements the Slider and SliderProps from Material-UI
2. Use that component and pass in an event handler function for onChange
3. See that the onChange event is marked as invalid. The full error text from TS is Type '(_: any, value: any) => void' is not assignable to type '((event: ChangeEvent<{}>, value: number | number[]) => void) & ((event: FormEvent<HTMLSpanElement>) => void)'. Type '(_: any, value: any) => void' is not assignable to type '(event: FormEvent<HTMLSpanElement>) => void'.ts(2322)

Context 🔦

I'm trying to upgrade our project from 4.8.3 to 4.9.7 to stay up to date, but this issue has caused me to be unable to move forward, as I can't seem to define onChange events that will pass TS scrutiny. (FWIW, everything works as it should as far as functionality, this is just some weird typing issue.)

Your Environment 🌎

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.9.7 |
| React | 16.12 |
| Browser | Chrome 80|
| TypeScript | 3.7.4 |
| MacOS | 10.15.3 |

[oliviertassinari]

@riceboyler The change you are referring to is #20110. I can reproduce the issue.

[oliviertassinari]

Same problem if we set defaultValue.

[oliviertassinari]

@eps1lon I would expect the Material-UI components to intercepts the incoming props and to spread props compatible with the default component. In this context, this diff seems appropriate:

diff --git a/packages/material-ui/src/OverridableComponent.d.ts b/packages/material-ui/src/OverridableComponent.d.ts
index 172d3572a..14f997b70 100644
--- a/packages/material-ui/src/OverridableComponent.d.ts
+++ b/packages/material-ui/src/OverridableComponent.d.ts
@@ -21,7 +21,7 @@ export type OverrideProps<
   C extends React.ElementType
 > = (
   & BaseProps<M>
-  & Omit<React.ComponentPropsWithRef<C>, keyof CommonProps<M>>
+  & Omit<React.ComponentPropsWithRef<C>, keyof BaseProps<M>>
 );

 /**

What do you think about it? It does break an existing test case, but looking closely at this test, I don't think that the test was correct:

https://github.com/mui-org/material-ui/blob/89687f38cae750650555772ba4d821c9084d8dfc/packages/material-ui/test/typescript/OverridableComponent.spec.tsx#L137

As far as I know, the component could very well transform the type of its input props before applying them to the root element. So, the incompatibility shouldn't be verified between the expected props and the component props, but between the default component props and the provided component props. I doubt it's something we can test, happy to ignore this case.

[eps1lon]

This is another reason why polymorphic components are so hard to reason about.

Imagine your MaterialUIComponent implements variant expecting a string. Now you pass in your CustomComponent that implements variant expecting a number like so <MaterialUIComponent component={CustomComponent} />.

What would be the correct type of variant? Does MaterialUIComponent only handle string variant and gracefully passes this along if the type didn't match? Does it crash otherwise? Is the behavior undefined? Do we even care at that point that CustomComponent never sees variant?

The test you linked tests exactly that: Foo stops working correctly if inconsistentProp isn't a string. At the same time MyIncompatibleComponent1 will never receive a number in inconsistentProp because Foo already crashed before.

The fix should tell typescript "I take any props that DefaultComponent takes but implement prop onChange differently. In fact you will never see onChange from DefaultComponent".

props compatible with the default component

What happens if another component is passed? We can't determine the expected type at runtime. And we don't want to always assume the type like we're using the DefaultComponent. We've seen in the past that users don't want this. It's the reason we have polymorphic components in the first place.

[oliviertassinari]

@eps1lon Definitely a hard problem 🤯, here is how far I went with the reflection:

• The primary use case for using OverridableComponent component seems to be about support new props, available on CustomComponent but not on MaterialUIComponent. Correct me if I'm wrong.
• We can assume that if a prop isn't available on MaterialUIComponent, it will be forwarded to CustomComponent as is.
• There is uncertainty around what happens for a prop available on MaterialUIComponent. The component might, 1. forward it or not, 2. keep the same type or not, 3. keep the same name or not.

What would be the correct type of variant?

The correct type is a string. MaterialUIComponent should set the constraint as it's the first render logic to handle the prop.

Does MaterialUIComponent only handle string variant and gracefully passes this along if the type didn't match?

Yes, we don't have enough information to conclude what MaterialUIComponent does with the prop, he might very well convert it to a number.

In fact you will never see onChange from DefaultComponent".

Well, it's the root of this problem (that impacts a couple of others), The DefaultComponent has an onChange type.

Do we even care at that point that CustomComponent never sees variant?

I'm not sure that we should care.

---

Does the proposed fix match your requirements? It basically says, if you provide a custom component, we only type check the new props.

[eps1lon]

The correct type is a string. MaterialUIComponent should set the constraint as it's the first render logic to handle the prop.

Which means unsound types in your custom component. If you expect a non-nullable variant of type number then you either (depending on the implementation): will always see undefined when you expect a number or see string when you expect a number.

So we definitely should have this test in place. The custom component has to expect the same type or none at all.

For the Slider case we simply need to tell the type checker that the underlying span will not see our onChange implementation.

[riceboyler]

This is another reason why polymorphic components are so hard to reason about.

Imagine your MaterialUIComponent implements variant expecting a string. Now you pass in your CustomComponent that implements variant expecting a number like so <MaterialUIComponent component={CustomComponent} />.

What would be the correct type of variant? Does MaterialUIComponent only handle string variant and gracefully passes this along if the type didn't match? Does it crash otherwise? Is the behavior undefined? Do we even care at that point that CustomComponent never sees variant?

The test you linked tests exactly that: Foo stops working correctly if inconsistentProp isn't a string. At the same time MyIncompatibleComponent1 will never receive a number in inconsistentProp because Foo already crashed before.

The fix should tell typescript "I take any props that DefaultComponent takes but implement prop onChange differently. In fact you will never see onChange from DefaultComponent".

props compatible with the default component

What happens if another component is passed? We can't determine the expected type at runtime. And we don't want to always assume the type like we're using the DefaultComponent. We've seen in the past that users don't want this. It's the reason we have polymorphic components in the first place.

We actually have this exact same issue with the Slider as well. Previously, the orientation and track props were set as strings in our local wrapper, but with the changes in #20110 we had to set them to explicitly match the props in the MaterialUI Slider Props.

Honestly, I'm glad that it was reproducible and I'm not crazy. I guess most people who use the library don't wrap the components, but we do that so we can easily pass through styling (or other props) for consistency.

[oliviertassinari]

For the Slider case we simply need to tell the type checker that the underlying span will not see our onChange implementation.

@eps1lon It's somewhat a larger issue. it impacts:

• Slider: onChange, defaultValue
• BottomNavigation: onChange
• CardHeader: title
• Tabs: onChange

We actually have this exact same issue with the Slider as well. Previously, the orientation and track props were set as strings in our local wrapper, but with the changes in #20110 we had to set them to explicitly match the props in the MaterialUI Slider Props.

@riceboyler Do you have an example we can look at?

[riceboyler]

I'm glad you asked. In looking back through our code, I realized that the problem was we weren't properly instantiating our useState calls, so the typing was off. Never mind this second one.

The first issue with the SliderProps still applies though.

[eps1lon]

Good news: The suggested type was definitely wrong on our part.
Bad news: React.ChangeEvent is not the (technically) correct type (Though React.ChangeEvent<{}> will work incidentally. It's only valid structurally but has implications that don't hold true such as event.target === event.currentTarget.)

We're not creating a custom, react change event in the Slider component. We're simply passing along whatever event triggered the change in value. I won't go into detail what specific type of events this could be so that we're not needlessly restricting our implementation.

It's a bit unfortunate since it's highly unlikely that you ever relied on specifics of ChangeEvent simply because they don't matter in our case (has something to do with target vs. currentTarget).

I suggest using function handleSliderChange(event: React.SyntheticEvent, tabsValue: unknown) {} since that is going to be the type in v5.

[ryancarville]

Hey all,
Has this new implementation of a React.SyntheticEvent been proven to be the correct approach. I am having the same issue but when I use the synthetic event I get the same error plus a few more!
Thanks in advance.

Error

Type '(e: React.SyntheticEvent, value: number) => void' is not assignable to type '((event: ChangeEvent<{}>, value: number | number[]) => void) & ((event: FormEvent<HTMLSpanElement>) => void)'.
  Type '(e: React.SyntheticEvent, value: number) => void' is not assignable to type '(event: ChangeEvent<{}>, value: number | number[]) => void'.
    Types of parameters 'e' and 'event' are incompatible.
      Type 'ChangeEvent<{}>' is not assignable to type 'SyntheticEvent<Element, Event>'.
        Types of property 'currentTarget' are incompatible.
          Type 'EventTarget' is not assignable to type 'EventTarget & Element'.
            Type 'EventTarget' is missing the following properties from type 'Element': assignedSlot, attributes, classList, className, and 120 more

[oliviertassinari]

It was solved in v5.0.0-alpha.1

[ryancarville]

AHHHH!!! We are still running v4.11. Will update and try again. Thanks!

[rmehlinger]

It was solved in v5.0.0-alpha.1

Is there any chance of getting this backported to a patch for 4.11?

[oliviertassinari]

@rmehlinger It's unlikely, I think that we should invest time in making the upgrade easier instead rather than making it less compelling.