[Slider] restrict slider type #1241

seloner · 2024-12-28T23:55:03Z

Trying to fix

seloner · 2024-12-28T23:55:41Z

packages/react/src/slider/root/SliderRoot.tsx


 /**
 * Groups all parts of the slider.
 * Renders a `<div>` element.
 *
 * Documentation: [Base UI Slider](https://base-ui.com/react/components/slider)
 */
-const SliderRoot = React.forwardRef(function SliderRoot(
-  props: SliderRoot.Props,
+const SliderRoot = fixedForwardRef(function SliderRoot<TValue extends number | number[]>(


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

@seloner I think we can remove generic SliderRoot component by removing generic in Props interfacce.

Check https://github.com/mui/base-ui/pull/1241/files#r1899037802, please

seloner · 2024-12-28T23:57:43Z

packages/react/src/slider/root/SliderRoot.tsx

@@ -157,28 +158,25 @@ export namespace SliderRoot {
    values: ReadonlyArray<number>;
  }

-  export interface Props
+  export interface Props<TValue extends number | number[]>


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.

@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 · 2024-12-28T23:58:01Z

Netlify deploy preview

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

Generated by 🚫 dangerJS against 11792ab

seloner · 2024-12-28T23:58:30Z

packages/react/src/slider/root/SliderRoot.tsx

  }
 }

 export { SliderRoot };

+// @ts-expect-error


I don't know how to handle this I am getting ts error for proptypes.
Tried the proptypes script but no luck.

if you remove fixedForwardRef and replace it with React.forwardRef, then // @ts-expect-error can be removed.

@@ -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 {

good-jinu · 2024-12-29T01:31:10Z

Nice work! @seloner

This issue originated from mui/material-ui#44777. Therefore, referring to mui/material-ui#44777 might be helpful.

IMHO, it would be better to run pnpm proptypes and fix the ts errors since the CI: test_static check has failed.

It has failed CI check

good-jinu

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

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'

Remove Generic Type from SliderRoot: Simplify SliderRoot by removing the generic type.
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 {

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 · 2024-12-29T07:08:29Z

packages/react/src/slider/root/SliderRoot.tsx

+    /**
+     * The component orientation.
+     * @default 'horizontal'
+     */


@seloner
It seems like this should be removed.

good-jinu · 2024-12-29T07:19:03Z

packages/react/src/slider/root/SliderRoot.tsx

@@ -157,28 +158,25 @@ export namespace SliderRoot {
    values: ReadonlyArray<number>;
  }

-  export interface Props
+  export interface Props<TValue extends number | number[]>


@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)

good-jinu · 2024-12-29T07:20:26Z

packages/react/src/slider/root/SliderRoot.test.tsx

@@ -49,7 +49,7 @@ function TestSlider(props: SliderRoot.Props) {
  );
 }

-function TestRangeSlider(props: SliderRoot.Props) {
+function TestRangeSlider(props: SliderRoot.Props<number[]>) {


@seloner I think we can define Props without using generics.

Check https://github.com/mui/base-ui/pull/1241/files#r1899037802, please

good-jinu · 2024-12-29T07:20:56Z

packages/react/src/slider/root/SliderRoot.tsx


 /**
 * Groups all parts of the slider.
 * Renders a `<div>` element.
 *
 * Documentation: [Base UI Slider](https://base-ui.com/react/components/slider)
 */
-const SliderRoot = React.forwardRef(function SliderRoot(
-  props: SliderRoot.Props,
+const SliderRoot = fixedForwardRef(function SliderRoot<TValue extends number | number[]>(


@seloner I think we can remove generic SliderRoot component by removing generic in Props interfacce.

Check https://github.com/mui/base-ui/pull/1241/files#r1899037802, please

good-jinu · 2024-12-29T07:29:02Z

packages/react/src/slider/root/SliderRoot.tsx

  }
 }

 export { SliderRoot };

+// @ts-expect-error


if you remove fixedForwardRef and replace it with React.forwardRef, then // @ts-expect-error can be removed.

@@ -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 {

seloner · 2024-12-29T10:40:11Z

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

good-jinu · 2024-12-29T15:09:54Z

@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 · 2024-12-29T17:12:11Z

 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 · 2024-12-30T12:28:27Z

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 · 2024-12-30T14:45:10Z

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 · 2024-12-30T15:36:42Z

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 · 2024-12-30T16:29:52Z

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 · 2024-12-31T18:27:15Z

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 external API.

seloner commented Dec 28, 2024

View reviewed changes

seloner changed the title ~~[Slider]: restrict slider type~~ [Slider] restrict slider type Dec 29, 2024

seloner force-pushed the fix/slider-types branch from 7f7d189 to 186397e Compare December 29, 2024 00:04

seloner marked this pull request as ready for review December 29, 2024 00:04

seloner requested review from atomiks, colmtuite and michaldudak as code owners December 29, 2024 00:04

oliviertassinari added the component: slider This is the name of the generic UI component, not the React module! label Dec 29, 2024

good-jinu suggested changes Dec 29, 2024

View reviewed changes

seloner added 2 commits December 29, 2024 11:31

restrict slider type

ecd1b5b

generate proptypes

11792ab

seloner force-pushed the fix/slider-types branch from 186397e to 11792ab Compare December 29, 2024 10:37

mj12albert added the typescript label Dec 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Slider] restrict slider type #1241

[Slider] restrict slider type #1241

seloner commented Dec 28, 2024 •

edited

Loading

seloner Dec 28, 2024

good-jinu Dec 29, 2024

seloner Dec 28, 2024

good-jinu Dec 29, 2024

mui-bot commented Dec 28, 2024 •

edited

Loading

seloner Dec 28, 2024

good-jinu Dec 29, 2024

good-jinu commented Dec 29, 2024 •

edited

Loading

good-jinu left a comment •

edited

Loading

good-jinu Dec 29, 2024

good-jinu Dec 29, 2024

good-jinu Dec 29, 2024

good-jinu Dec 29, 2024

good-jinu Dec 29, 2024

seloner commented Dec 29, 2024 •

edited

Loading

good-jinu commented Dec 29, 2024

seloner commented Dec 29, 2024

mj12albert commented Dec 30, 2024

seloner commented Dec 30, 2024

mj12albert commented Dec 30, 2024

seloner commented Dec 30, 2024

good-jinu commented Dec 31, 2024

[Slider] restrict slider type #1241

Are you sure you want to change the base?

[Slider] restrict slider type #1241

Conversation

seloner commented Dec 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mui-bot commented Dec 28, 2024 • edited Loading

Netlify deploy preview

Choose a reason for hiding this comment

Choose a reason for hiding this comment

good-jinu commented Dec 29, 2024 • edited Loading

good-jinu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seloner commented Dec 29, 2024 • edited Loading

good-jinu commented Dec 29, 2024

seloner commented Dec 29, 2024

mj12albert commented Dec 30, 2024

seloner commented Dec 30, 2024

mj12albert commented Dec 30, 2024

seloner commented Dec 30, 2024

good-jinu commented Dec 31, 2024

seloner commented Dec 28, 2024 •

edited

Loading

mui-bot commented Dec 28, 2024 •

edited

Loading

good-jinu commented Dec 29, 2024 •

edited

Loading

good-jinu left a comment •

edited

Loading

seloner commented Dec 29, 2024 •

edited

Loading