[Slider] Narrow the type of value in callbacks

[seloner]

Trying to fix

Closes #1230

[seloner]

ReactForwardRef does not work well with generics.
So we need to create a wrapper function to apply the generic.

[mj12albert]

Instead of making fixedForwardRef, we could cast the result of React.forwardRef, e.g. something like this:

type SliderRootType = {
  <Value>(props: SliderRoot.Props<Value>): React.JSX.Element;
  propTypes?: any;
}

const SliderRoot = React.forwardRef(function SliderRoot<Value>(
  props: SliderRoot.Props<Value>,
  forwardedRef: React.ForwardedRef<HTMLDivElement>
) {
  //
}) as SliderRootType;

We did something similar in the legacy package: https://github.com/mui/material-ui/blob/master/packages/mui-base/src/Select/Select.tsx#L235

[seloner]

My first approach here was to try to carry this generic all the way down in the slider components but it resulted to just pollute the code with generics.
Instead I simple just try to modify the top level component that the consumer will use.

[good-jinu]

@seloner
I believe we can define Props without using generics. By utilizing intersection types, we can potentially make the necessary changes internally without affecting the external API. This approach could simplify the type definitions and improve readability.

Here’s an example of how we can implement this:

@@ -158,7 +158,10 @@ export namespace SliderRoot {
     values: ReadonlyArray<number>;
   }
 
-  export interface Props<TValue extends number | number[]>
+  export type Props = PropsTemplate<number> &
+    PropsTemplate<number[]> &
+    PropsTemplate<number | number[]>;
+  export interface PropsTemplate<TValue extends number | number[]>
     extends Pick<
         useSliderRoot.Parameters,
         | 'disabled'

Please refer to mui/material-ui#44777 (comment)

[mui-bot]

Netlify deploy preview

https://deploy-preview-1241--base-ui.netlify.app/

Generated by 🚫 dangerJS against 2bff7c2

[good-jinu]

I’d like to suggest the following tasks to address this issue:

1. Convert SliderRoot.Props to an Intersection Type: Change the Props definition to use an intersection type, eliminating the need for generics.

@@ -158,7 +158,10 @@ export namespace SliderRoot {
     values: ReadonlyArray<number>;
   }
 
-  export interface Props<TValue extends number | number[]>
+  export type Props = PropsTemplate<number> &
+    PropsTemplate<number[]> &
+    PropsTemplate<number | number[]>;
+  export interface PropsTemplate<TValue extends number | number[]>
     extends Pick<
         useSliderRoot.Parameters,
         | 'disabled'

2. Remove Generic Type from SliderRoot: Simplify SliderRoot by removing the generic type.
3. Replace fixedForwardRef with React.forwardRef: Update the code to use React.forwardRef directly, which will allow us to remove the // @ts-expect-error.

@@ -18,8 +18,8 @@ import { fixedForwardRef } from '../utils';
  *
  * Documentation: [Base UI Slider](https://base-ui.com/react/components/slider)
  */
-const SliderRoot = fixedForwardRef(function SliderRoot<TValue extends number | number[]>(
-  props: SliderRoot.Props<TValue>,
+const SliderRoot = React.forwardRef(function SliderRoot(
+  props: SliderRoot.Props,
   forwardedRef: React.ForwardedRef<HTMLDivElement>,
 ) {
   const {

4. Remove // @ts-expect-error: With the changes above, the TypeScript error should be resolved, making the // @ts-expect-error unnecessary.

These changes should help resolve the type-related issues. Let me know your thoughts!

[good-jinu]

@seloner
I believe we can define Props without using generics. By utilizing intersection types, we can potentially make the necessary changes internally without affecting the external API. This approach could simplify the type definitions and improve readability.

Here’s an example of how we can implement this:

@@ -158,7 +158,10 @@ export namespace SliderRoot {
     values: ReadonlyArray<number>;
   }
 
-  export interface Props<TValue extends number | number[]>
+  export type Props = PropsTemplate<number> &
+    PropsTemplate<number[]> &
+    PropsTemplate<number | number[]>;
+  export interface PropsTemplate<TValue extends number | number[]>
     extends Pick<
         useSliderRoot.Parameters,
         | 'disabled'

Please refer to mui/material-ui#44777 (comment)

[seloner]

@good-jinu
I have applied your suggestions but I can't make this work 🤔
SlideRoot.test.tsx broke

[good-jinu]

@seloner My bad

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

So It seems like just a field like value doesn't work with it.
How about union type?

For example:

export type Props =  PropsTemplate<number> |
  PropsTemplate<number[]> |
  PropsTemplate<number | number[]>;

[seloner]

 PropsTemplate<number> |
  PropsTemplate<number[]> |
  PropsTemplate<number | number[]>;

It does not work
I think the generic is needed.
Another approach is to export two SliderRoots one SliderRoot and one RangeSliderRoot to support both types without generic?

[mj12albert]

Another approach is to export two SliderRoots one SliderRoot and one RangeSliderRoot to support both types without generic?

@seloner Thanks for working on this, I'm afraid this isn't an option though

So It seems like just a field like value doesn't work with it.

@good-jinu Not sure if I understand you correctly, by "field like value" did you mean a single non-array value?

[seloner]

Another approach is to export two SliderRoots one SliderRoot and one RangeSliderRoot to support both types without generic?

@seloner Thanks for working on this, I'm afraid this isn't an option though

So It seems like just a field like value doesn't work with it.

@good-jinu Not sure if I understand you correctly, by "field like value" did you mean a single non-array value?

We can still do the solution with the generics

[mj12albert]

We can still do the solution with the generics

I think this is ok as long as it doesn't affect the public API, just wonder why @good-jinu is against generics (I prefer to avoid them too, but I'm just curious here)

A while back we did decide that for components with values that could sometimes be an array (accordion, slider, toggle group), we'd make the value in callbacks always be an array so it's predictable, also curious to see what you all think about this!

[seloner]

We can still do the solution with the generics

I think this is ok as long as it doesn't affect the public API, just wonder why @good-jinu is against generics (I prefer to avoid them too, but I'm just curious here)

A while back we did decide that for components with values that could sometimes be an array (accordion, slider, toggle group), we'd make the value in callbacks always be an array so it's predictable, also curious to see what you all think about this!

We could use a default value so it does no really affect the public API compared to now .

type Props<T extends number | number[] = number | number[]> = {
  value: T;
  onValueChange: (value: T) => void;
};

const Component: <T extends number | number[]>(props: Props<T>) => {};

I like your suggestion to remove the number case and always go with an array.
It will work for this specific case.
The only issue I see that it tries to bypass the problem and not solve it.
In this case it will be fine but in general for a more complex case it will not work.
For example if there is a case with possible inputs string/number.

I am new to this project so I can't really answer though what is the best solution here.

[good-jinu]

I think this is ok as long as it doesn't affect the public API, just wonder why @good-jinu is against generics (I prefer to avoid them too, but I'm just curious here)

I actually agree with using generics; however, I wanted to avoid changing the public API. My main intention was to eliminate the @ts-expect-error from previous commits due to the TypeScript error.

I also think it's fine as long as it doesn't change the public API.

[seloner]

@good-jinu @mj12albert

Updated.
Used the defaults for generics.
The only issue that I can't solve is proptypes.

[mj12albert]

Suggested change

	export interface Props<TValue extends number[] | number = number | number[]>
	export interface Props<Value extends readonly number[] | number = number | readonly number[]>

Value is fine ~ we don't use the T* convention, also the array form of value/defaultValue should be readonly arrays

[mj12albert]

Instead of making fixedForwardRef, we could cast the result of React.forwardRef, e.g. something like this:

type SliderRootType = {
  <Value>(props: SliderRoot.Props<Value>): React.JSX.Element;
  propTypes?: any;
}

const SliderRoot = React.forwardRef(function SliderRoot<Value>(
  props: SliderRoot.Props<Value>,
  forwardedRef: React.ForwardedRef<HTMLDivElement>
) {
  //
}) as SliderRootType;

We did something similar in the legacy package: https://github.com/mui/material-ui/blob/master/packages/mui-base/src/Select/Select.tsx#L235

[mj12albert]

I like your suggestion to remove the number case and always go with an array.

@seloner Since this would be a breaking change, let's keep going with the solution using generics!

I've left some comments, you should also merge/rebase master first btw ~