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][material-ui] Pass narrowed value type to onChange and onChangeCommitted #38753

Closed
wants to merge 36 commits into from

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Sep 1, 2023

closes: #37854

@sai6855 sai6855 marked this pull request as draft September 1, 2023 12:38
@sai6855 sai6855 added component: slider This is the name of the generic UI component, not the React module! typescript package: material-ui Specific to @mui/material enhancement This is not a bug, nor a new feature labels Sep 1, 2023
@mui-bot
Copy link

mui-bot commented Sep 1, 2023

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

@@ -306,7 +329,7 @@ export declare const SliderValueLabel: React.FC<SliderValueLabelProps>;
*
* - [Slider API](https://mui.com/material-ui/api/slider/)
*/
declare const Slider: OverridableComponent<SliderTypeMap>;
declare const Slider: SliderType;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

👍 it's consistent with

export interface SelectType {

@sai6855 sai6855 changed the title [Slider] Pass correct value type to onChange and onChangeCommitted [Slider][material-ui] Pass correct value type to onChange and onChangeCommitted Sep 1, 2023
@sai6855 sai6855 changed the title [Slider][material-ui] Pass correct value type to onChange and onChangeCommitted [Slider][material-ui] Pass narrowed value type to onChange and onChangeCommitted Sep 1, 2023
@sai6855 sai6855 requested review from ZeeshanTamboli and michaldudak and removed request for ZeeshanTamboli September 1, 2023 18:22
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.

@sai6855 thanks for working on this 😊

} & OverrideProps<SliderTypeMap<SliderTypeMap['defaultComponent'], {}, Value>, RootComponent>,
): JSX.Element | null;
<Value extends number | number[]>(
props: { value?: Value } & DefaultComponentProps<
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sai6855 sai6855 Sep 5, 2023

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

/**
* 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.
.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

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.

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

> {
props: AdditionalProps & SliderOwnProps;
props: AdditionalProps & SliderOwnProps<Value, OnChangeValue>;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need both Value and OnChangeValue?

Copy link
Member

Choose a reason for hiding this comment

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

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

@sai6855
Copy link
Contributor Author

sai6855 commented Sep 8, 2023

@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

@sai6855 sai6855 mentioned this pull request Sep 12, 2023
@DiegoAndai
Copy link
Member

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

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

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

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.

packages/mui-material/src/Slider/Slider.d.ts Outdated Show resolved Hide resolved
* Either a string to use a HTML element or a component.
*/
component: RootComponent;
value?: Value;
Copy link
Member

@michaldudak michaldudak Sep 12, 2023

Choose a reason for hiding this comment

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

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
};

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@sai6855 sai6855 Sep 18, 2023

Choose a reason for hiding this comment

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

@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;
}

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 2024
@DiegoAndai
Copy link
Member

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.

@DiegoAndai DiegoAndai closed this Mar 14, 2024
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 PR: out-of-date The pull request has merge conflicts and can't be merged typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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