[Slider][material-ui] Pass narrowed value type to onChange and onChangeCommitted

[sai6855]

closes: #37854

• I have followed (at least) the PR section of the contributing guide.

[mui-bot]

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 3c96556

[sai6855]

Not quite sure what to name the type, so went with a naive name. Feel free to suggest appropriate name

[michaldudak]

👍 it's consistent with

material-ui/packages/mui-base/src/Select/Select.types.ts

Line 177 in 6411821

export interface SelectType {

[DiegoAndai]

@sai6855 thanks for working on this 😊

[DiegoAndai]

Could you explain why it's necessary to add value explicitly here? Isn't it enough that it's in SliderTypeMap?

[sai6855]

Since both value and value arg in onChange is expected to have exact same type... we have to explicitly extract value type and pass it to onChange to make value types in both places to have same type.

To make it easy for you, i've made this ts playground with simplified version of this change. Please feel free to go through and let me know if anything is unclear for you.

https://www.typescriptlang.org/play?#code/PTAEDcFMCcGcEsD2A7U9UFsCGsAuNQAjaLZAYwAsAoAMwFdzclUBlAG3gBMYAKFAYQqkA5pABcyOhkIwAPpOkwA2gF0ANOCxs64hTOjyp+1QEoA3lSoBfS+y68ATGqUOVJ0CFCwKiOm06guBTQiADuoDAh0ER0uKDIiHFB0KGI6MJUdtzQPC7qDu6e3r7+gcFhEdBRMXEJScGp6ZZUnlBwzKCh8EG+cQByiACSyDQwtAxkTCigWTAA6t0UAPKxA8Oj0AA8AGpaOhEAHvjInLDxRnJ6yioAfHzIgiLiu9qQGnvPH+aWNpkc2QsgitcGsRo5nK5CmBko1kMJKtVpqEQnDQAAHLAkDB-ezQQHLVZDME5PJqAoeMCpaAAa3SoBo6DeZRSaVRkUQ0WmZA50Egk3RmKw2OauAAnmjIKBQRtNgAVG6gAC8oCUspUqsOx1OoFIotAAH5QAAGUBieKQNoqADcLTAbQQSMWUqJG3GjA6szxi2lMB2H01kBOZyuBhDqjuAiEcPEPq2Lx0N3erzE8cg32szU9+NjPCceShnQ5tNRDOQTJhrPh7M5qG5VT5cQxWJxAO9Lt4pPJnipxfhpfLDUrCI5oC5PIbAubQA

[DiegoAndai]

My question was, why is it necessary to extract the value if it's already in the SliderTypeMap 🤔

props:
  { value?: Value } // <--- why is this necessary
  & DefaultComponentProps<
    SliderTypeMap<SliderTypeMap['defaultComponent'], {}, Value> // <--- if it's already passed here
  >

I don't see that done in the Typescript playground you shared, but I may be missing something.

While reviewing the NoInfer issue, I found this comment. Would that work for us? I think it's better as it does not rely on the deferral of evaluation. I've tried it in this playground and seems to be working.

[sai6855]

value prop and value in onChange args should have exact same type. when i say same type what i mean is, if value is of number type and then value arg in onChange should also be number.

Since value in onChange arg is unaware of what value type is (as they are not linked), we have to extract value prop type and pass it to onChange to link both types and make both values exact same type.

Would that work for us?

I did tried your suggestion, it worked and all tests are passing. commit related to change f8374c1 , but there is one catch. If below prop description is not added, yarn proptypes script is removing onChange prop description in Slider.js proptypes. So i had to add it. I know there is duplication of prop description, but i couldn't think of any other way.

material-ui/packages/mui-material/src/Slider/Slider.d.ts

Lines 314 to 320 in f8374c1

	/**
	* Callback function that is fired when the slider's value changed.
	*
	* @param {Event} event The event source of the callback.
	* You can pull out the new value by accessing `event.target.value` (any).
	* **Warning**: This is a generic event not a change event.
	* @param {number | number[]} value The new value.

.

[DiegoAndai]

Regarding adding the onChange description twice, is that not happening for onChangeCommitted? Is that different somehow?

@michaldudak do you know why this might be happening?

[michaldudak]

I'm not so deep into the docs infra. Perhaps someone from the @mui/docs-infra could help here.

[alexfauquette]

Seems to not be the only issue we have with props documentation. It look similar to #38459 where some default value are ignored.

Could you open an issue with the diff to do in order to remove the hack such that we can try a deeper look later?

[DiegoAndai]

Left two minor comments and replied to the onChange description added twice comment above. I think this is really close to being ready 🎉

[michaldudak]

Why do we need both Value and OnChangeValue?

[DiegoAndai]

I proposed it instead of the NoInfer type here: #38753 (comment). Got it from here: microsoft/TypeScript#14829 (comment)

[sai6855]

@DiegoAndai i polished PR a bit, here are the summary of new changes.

1. 6f0f84b this commit has changes you suggested
2. 32eb5e0 this commit fixed a regression, if defaultValue is provided as explicit number, then previoulsy value in onChange is infered as explicit number instead of generic number type. This commit fixes the issue , it has both logic changes and tests
3. c9d8d9e this commit added a test when both value , defaultValue prop is not provided.

Also not sure why CI is failing. as error shown is not relavant to this PR. can you help Triggering CI worked

[DiegoAndai]

This looks ready to merge to me. @michaldudak, may I ask you for a final review? Specially regarding:

• This comment
• This comment
• The overall Typescript implementation

These are somewhat complex type structures that I need more experience with to feel comfortable approving on my own.

[michaldudak]

I must admit this isn't the most obvious piece of code, but if it gets the job done, I can accept it. I added some remarks.

[michaldudak]

Thinking out loud here: I'm not sure if it could work but have you tried having two overloads / union of props instead of generic functions?

{
    value?: number;
    onChange?: (event: Event, value: number, activeThumb: number) => void;
} |
{
    value?: number[],
    onChange?: (event: Event, value: number[], activeThumb: number) => void;
}

[sai6855]

Tried below code after your suggestion, but somehow it's now working. types of event and value in onChange handler is inferring as any.

export type SliderTypeMap<
  RootComponent extends React.ElementType = 'span',
  AdditionalProps = {},
> = {
  props:
    | (AdditionalProps &
        Omit<SliderOwnProps, 'defaultValue' | 'value' | 'onChange' | 'onChangeCommitted'> & {
          defaultValue?: number[];
          value?: number[];
          onChange?: (event: Event, value: number[], activeThumb: number) => void;
          onChangeCommitted?: (event: React.SyntheticEvent | Event, value: number[]) => void;
        })
    | (AdditionalProps &
        Omit<SliderOwnProps, 'defaultValue' | 'value' | 'onChange' | 'onChangeCommitted'> & {
          defaultValue?: number;
          value?: number;
          onChange?: (event: Event, value: number, activeThumb: number) => void;
          onChangeCommitted?: (event: React.SyntheticEvent | Event, value: number) => void;
        });
  defaultComponent: RootComponent;
};

[michaldudak]

This seems to be working:

export interface SliderTypeMap<
  RootComponent extends React.ElementType = 'span',
  AdditionalProps = {},
  Value extends number | number[] = number | number[],
> {
  props: AdditionalProps & SliderOwnProps<Value>;
  defaultComponent: RootComponent;
}

export interface SliderType {
  <RootComponent extends React.ElementType>(
    props: {
      component: RootComponent;
    } & OverrideProps<SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number>, RootComponent>,
  ): JSX.Element | null;
  (
    props: DefaultComponentProps<SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number>>,
  ): JSX.Element | null;
  <RootComponent extends React.ElementType>(
    props: {
      component: RootComponent;
    } & OverrideProps<
      SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number[]>,
      RootComponent
    >,
  ): JSX.Element | null;
  (
    props: DefaultComponentProps<SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number[]>>,
  ): JSX.Element | null;
}

...but a few tests with undefined value fail. Could you explore this direction a bit? I can help you if you hit any roadblocks. Having just one type parameter simplifies the code and (possibly?) allows to remove duplicate prop definitions (as noted in #38753 (comment))

[sai6855]

@michaldudak Tried your sugegstion in this commit (i tweaked a bit) 3c96556 it seems to be working, all tests are passing. But CI is not happy with implementation. More details can be found here. I didn't entirely understood the CI error, but from my understanding i tried below code locally, but now all tests are failing . I didn't pushed below code, just tried locally.

export interface SliderType {
  <RootComponent extends React.ElementType>(
    props:
      | ({
          /**
           * The component used for the root node.
           * Either a string to use a HTML element or a component.
           */
          component: RootComponent;
        } & (
          | {
              value: number;
              defaultValue?: number;
            }
          | {
              value?: number;
              defaultValue: number;
            }
        ) &
          OverrideProps<
            SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number>,
            RootComponent
          >)
      | ({
          /**
           * The component used for the root node.
           * Either a string to use a HTML element or a component.
           */
          component: RootComponent;
        } & (
          | {
              value: number[];
              defaultValue?: number[];
            }
          | {
              value?: number[];
              defaultValue: number[];
            }
        ) &
          OverrideProps<
            SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number[]>,
            RootComponent
          >)
      | ({
          /**
           * The component used for the root node.
           * Either a string to use a HTML element or a component.
           */
          component: RootComponent;
        } & OverrideProps<
          SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number | number[]>,
          RootComponent
        >),
  ): JSX.Element | null;
  (
    props:
      | (DefaultComponentProps<SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number>> &
          (
            | {
                value: number;
                defaultValue?: number;
              }
            | {
                value?: number;
                defaultValue: number;
              }
          ))
      | (DefaultComponentProps<SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number[]>> &
          (
            | {
                value: number[];
                defaultValue?: number[];
              }
            | {
                value?: number[];
                defaultValue: number[];
              }
          ))
      | DefaultComponentProps<
          SliderTypeMap<SliderTypeMap['defaultComponent'], {}, number | number[]>
        >,
  ): JSX.Element | null;
}

[DiegoAndai]

I'm closing this as we have yet to find a proper implementation, and I'm not keen on releasing this with the last v5 release.

I added the original issue to the v7 draft milestone to revisit later.

Thanks for working on this anyway, @sai6855. I hope we can implement it soon.