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

[Slider] Narrow onChange value type #44777

Merged
merged 16 commits into from
Jan 23, 2025
Merged

Conversation

good-jinu
Copy link
Contributor

@good-jinu good-jinu commented Dec 15, 2024

  • Change value and onChange value param type from number | number[] to generic Value

Closes #37854
Inspired by #38753


Solved every typescript error (not really sure but as far as I tested, it worked)

- Change value and onChange value param type from `number | number[]` to generic `Value`
@mui-bot
Copy link

mui-bot commented Dec 15, 2024

Netlify deploy preview

https://deploy-preview-44777--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 8d09708

@zannager zannager added the component: slider This is the name of the generic UI component, not the React module! label Dec 16, 2024
@zannager zannager requested a review from DiegoAndai December 16, 2024 14:30
@mj12albert mj12albert added typescript package: material-ui Specific to @mui/material labels Dec 17, 2024
@mj12albert mj12albert changed the title [mui-material][Slider] Narrow onChange value type [Slider] Narrow onChange value type Dec 17, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@good-jinu Looks good. Can you add a type test in Slider.spec.tsx file?

No work was done on the code snippets in the docs.

We need to update the type in demos. For example in ContinuousSlider demo:

--- a/docs/data/material/components/slider/ContinuousSlider.tsx
+++ b/docs/data/material/components/slider/ContinuousSlider.tsx
@@ -8,8 +8,8 @@ import VolumeUp from '@mui/icons-material/VolumeUp';
 export default function ContinuousSlider() {
   const [value, setValue] = React.useState<number>(30);

-  const handleChange = (event: Event, newValue: number | number[]) => {
-    setValue(newValue as number);
+  const handleChange = (event: Event, newValue: number) => {
+    setValue(newValue);
   };

   return (

Same in other demos.

@good-jinu
Copy link
Contributor Author

Thanks for your specific comment! @ZeeshanTamboli
I'll ensure that all the necessary changes are applied to the demos. I'll go ahead and add the type test in the Slider.spec.tsx as well.

- Change value and onChange value param type from `number | number[]` to generic `Value`
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@good-jinu I have pushed few commits to cleanup the code. But there's an issue. The component prop is no longer available on Slider after removing OverridableComponent type from the Slider type. It should be available. Additionally, TypeScript doesn't catch arbitrary props, allowing any prop without error detection.

PR CodeSandbox: https://codesandbox.io/p/sandbox/material-ui-cra-ts-forked-zm74j4
Master: https://codesandbox.io/p/sandbox/material-ui-cra-ts-forked-4ch62x

@DiegoAndai
Copy link
Member

Thanks for working on this @good-jinu. This is a difficult problem, we've tried to solve it a couple of times in the past, so expect more back and forth than usual PRs.

As additional context, Base UI has not implemented this: https://github.com/mui/base-ui/blob/master/packages/react/src/slider/root/useSliderRoot.ts. It makes me wonder if we should do it, given that we're going to use Base UI soon. @aarongarciah what do you think?

@good-jinu
Copy link
Contributor Author

Thank you @DiegoAndai for providing the context!

This issue turned out to be more complex and challenging than I initially thought. As you mentioned, if we resolve this problem using Base UI, I hope a better solution will emerge in the future instead of this PR.

@aarongarciah
Copy link
Member

aarongarciah commented Dec 25, 2024

@DiegoAndai I've added the issue (#37854) to the Base UI milestone. I think we should wait to act on this since we'll adopt Base UI eventually.

@good-jinu I've created an issue on the Base UI repo: mui/base-ui#1230

@ZeeshanTamboli
Copy link
Member

@aarongarciah @DiegoAndai @good-jinu What do you think about the recent changes I pushed for v6? It seems to work well.

@aarongarciah
Copy link
Member

@ZeeshanTamboli seems to work fine. I'll let @DiegoAndai have a go.

@DiegoAndai
Copy link
Member

The solution seems to work.

Before we can approve it. I would like to understand the rationale behind using intersection for the SliderType. An intersection is supposed to represent a combination of types, so how and why combining SliderComponent<number> & SliderComponent<number[]> & SliderComponent<number | number[]>; work in this scenario? I would like to understand this before proceeding with the change @ZeeshanTamboli.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Dec 28, 2024

I would like to understand the rationale behind using intersection for the SliderType. An intersection is supposed to represent a combination of types, so how and why combining SliderComponent<number> & SliderComponent<number[]> & SliderComponent<number | number[]>; work in this scenario? I would like to understand this before proceeding with the change @ZeeshanTamboli.

@DiegoAndai
We use an intersection type here to simplify function type overloading. The SliderComponent is a React function type because it leverages OverridableComponent, which describes a callable structure representing a React component that can be invoked like a function.

We can also explicitly define overloading for each case like:

type SliderTypeMapNumber<
  RootComponent extends React.ElementType = 'span',
  AdditionalProps = {},
> = SliderTypeMap<RootComponent, AdditionalProps, number>;

type SliderTypeMapNumberArray<
  RootComponent extends React.ElementType = 'span',
  AdditionalProps = {},
> = SliderTypeMap<RootComponent, AdditionalProps, number[]>;

interface SliderType {
  <RootComponent extends React.ElementType>(
    props: {
      /**
       * The component used for the root node.
       * Either a string to use an HTML element or a component.
       */
      component: RootComponent;
    } & OverrideProps<SliderTypeMapNumber, RootComponent>,
  ): React.JSX.Element | null;
  (props: DefaultComponentProps<SliderTypeMapNumber>): React.JSX.Element | null;

  <RootComponent extends React.ElementType>(
    props: {
      /**
       * The component used for the root node.
       * Either a string to use an HTML element or a component.
       */
      component: RootComponent;
    } & OverrideProps<SliderTypeMapNumberArray, RootComponent>,
  ): React.JSX.Element | null;
  (props: DefaultComponentProps<SliderTypeMapNumberArray>): React.JSX.Element | null;

  <RootComponent extends React.ElementType>(
    props: {
      /**
       * The component used for the root node.
       * Either a string to use an HTML element or a component.
       */
      component: RootComponent;
    } & OverrideProps<SliderTypeMap, RootComponent>,
  ): React.JSX.Element | null;
  (props: DefaultComponentProps<SliderTypeMap>): React.JSX.Element | null;
}

While this works, it is verbose and requires repeating similar patterns for each case. Using intersection simplifies the type definition:

type SliderType = 
  SliderComponent<number> & 
  SliderComponent<number[]> & 
  SliderComponent<number | number[]>;

An intersection of function types is treated as overloaded signatures in TypeScript. For example:

  • A call with value as number resolves to the first signature (SliderComponent<number>).
  • A call with value as number[] resolves to the second signature (SliderComponent<number[]>).
  • TypeScript ensures the correct props and return types are enforced for each case.

Function overloading is particularly useful when:

  • A specific subset of inputs maps directly to a subset of outputs, ensuring type safety.
  • It improves code readability by clearly defining distinct cases.

While you are correct that intersection is supposed to represent a combination of types, but this case is for function type.

For more on function overloading, refer to https://www.typescriptlang.org/docs/handbook/2/functions.html#function-overloads

So in conclusion intersections for same function but different types are considered a form of overloading in TypeScript. I also found this microsoft/TypeScript#53777 (comment), if it helps for better understanding.

@good-jinu
Copy link
Contributor Author

good-jinu commented Dec 28, 2024

I attempted to resolve this issue without a deep understanding of how the changes to the Slider type would affect the entire repository, and I greatly appreciate your help. @ZeeshanTamboli


The component prop is no longer available on Slider after removing OverridableComponent type from the Slider type. It should be available.

One concern I have is that the official API documentation does not mention the component prop. Should we add it? I initially thought the component prop should not be used since it was not included in the official documentation.

Actually, component was about to be added to the docs in #38753.

@@ -12,6 +12,9 @@
     "color": {
       "description": "The color of the component. It supports both default and custom theme colors, which can be added as shown in the <a href=\"https://mui.com/material-ui/customization/palette/#adding-new-colors\">palette customization guide</a>."
     },
+    "component": {
+      "description": "The component used for the root node. Either a string to use a HTML element or a component."
+    },
     "components": {
       "description": "The components used for each slot inside.<br>This prop is an alias for the <code>slots</code> prop. It&#39;s recommended to use the <code>slots</code> prop instead."
     },

Copy link
Contributor Author

@good-jinu good-jinu left a comment

Choose a reason for hiding this comment

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

type SliderType = 
  SliderComponent<number> & 
  SliderComponent<number[]> & 
  SliderComponent<number | number[]>;

The clearer and more readable definition of SliderType is excellent. 👍

@ZeeshanTamboli
Copy link
Member

@DiegoAndai This is ready for review; I've addressed your query above.

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation @ZeeshanTamboli; it's clear now 👍🏼


I'm cautious with this kind of type improvements, as they've backfired in the past.

Coincidentally, we plan to split the v6 (v6.x) and v7 (master) branches this week. Would you be ok with having this improvement included in v7? @ZeeshanTamboli @good-jinu.

This is just to be extra cautious and to avoid regressions on the v6.x branch now that we start working on v7.


Besides that, a question:

@ZeeshanTamboli
Copy link
Member

I'm cautious with this kind of type improvements, as they've backfired in the past.

Coincidentally, we plan to split the v6 (v6.x) and v7 (master) branches this week. Would you be ok with having this improvement included in v7? @ZeeshanTamboli @good-jinu.

This is just to be extra cautious and to avoid regressions on the v6.x branch now that we start working on v7.

@DiegoAndai I am fine with it.

@ZeeshanTamboli ZeeshanTamboli added the enhancement This is not a bug, nor a new feature label Jan 22, 2025
@DiegoAndai
Copy link
Member

Thanks @good-jinu and @ZeeshanTamboli!

This will be available in the first v7 pre-release, which should be coming out next week.

@DiegoAndai DiegoAndai merged commit ad4825a into mui:master Jan 23, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: slider This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Slider] Allow generic value type to narrow onChange value type
7 participants