[material-ui][TablePagination] Fix type error in Select props

[PaulKristoffersson]

Closes #38487

I based this solution on the changes made in #36737

Compared to #36737 I assumed we provide the TablePaginationOwnProps no matter which variant is used, since no other variant of props was defined beforehand.

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

[mui-bot]

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a8b36c2

[michaldudak]

Thanks for the PR. Please take a look at the failing CI checks. Also, please rebase on the latest master (this should help with the failing Argos check).

[PaulKristoffersson]

@michaldudak Thanks! I made some more changes that I think solves the issue well. Should I create some tests for PR?

After my rebase I get this error however, which I find confusing since I have not altered any files in either mui-base or the corresponding component. Do you have any idea how I can solve it?

\GolandProjects\material-ui\packages\mui-base\src\useTabPanel\useTabPanel.test.js
4:51 error Unable to resolve path to module 'test/utils' import/no-unresolved

[ZeeshanTamboli]

@PaulKristoffersson The type changes based on the variant should be made within the Select component types themselves, specifically in Select.d.ts file. This issue pertains to the SelectProps.

The type tests in the CI are still failing.

---

After my rebase I get this error however, which I find confusing since I have not altered any files in either mui-base or the corresponding component. Do you have any idea how I can solve it?
\GolandProjects\material-ui\packages\mui-base\src\useTabPanel\useTabPanel.test.js
4:51 error Unable to resolve path to module 'test/utils' import/no-unresolved

This is because test utilities are no longer imported from test/utils. To resolve this locally, please run yarn install from the project's root directory.

Let me know if you need any help.

[PaulKristoffersson]

@ZeeshanTamboli Thanks for the answer! Are you sure that I am supposed to the values inside the Select.d.ts file? Based on the reffered pull request (#36737) in the issue (#38487) I assumed that we wanted to assert our types based on the given Select variant inside of the component. By doing this we can pass the correct type of InputProps to the Select component and avoid the typeError.

Could you provide some additional information on how you would want that implemented inside of the Select component itself?

[ZeeshanTamboli]

@PaulKristoffersson Since TablePagination uses SelectProps of the Select component, I think it makes sense to fix it in the Select component types itself. As an example, I think when the Select component has filled variant, hiddenLabel prop should be available:

<Select variant="filled" hiddenLabel /> ---> Type error for hiddenLabel

I assumed that we wanted to assert our types based on the given Select variant inside of the component. By doing this we can pass the correct type of InputProps to the Select component and avoid the typeError.

We want the type to be based on the select variant. I didn't understand what you mean by the InputProps for TablePagination SelectProps.

Can you confirm the fix by adding TypeScript tests?

[PaulKristoffersson]

@ZeeshanTamboli Hey, I just came back to this issue after having worked on some other ones, sorry for the delay. In response to:

"I think it makes sense to fix it in the Select component types itself. As an example, I think when the Select component has filled variant, hiddenLabel prop should be available:"

As far as I can tell Select does not contain the hiddenLabel prop, instead the hiddenLabel prop is contained in one of the variations of the inputProps prop, namely FilledInput.

Do you want me to add hiddenLabel as a prop? Otherwise I can try to add some tests to show how my solution works.

[PaulKristoffersson]

@ZeeshanTamboli Just noticed that the SelectProps prop is deprecated and we slotProps.select instead, so I assume that there is not point on working with this issue anymore?

[ZeeshanTamboli]

As far as I can tell Select does not contain the hiddenLabel prop. Do you want me to add hiddenLabel as a prop? Otherwise I can try to add some tests to show how my solution works.

It doesn't have the hiddenLabel prop on its own, but when the variant="filled" is applied, the Select component renders the FilledInput component, which can be seen here. Therefore, the TypeScript definitions for Select should include the props of FilledInput when variant is filled, which includes the hiddenLabel prop. Feel free to add tests.

Just noticed that the SelectProps prop is deprecated and we slotProps.select instead, so I assume that there is not point on working with this issue anymore?

But the slotProps.select has the same type - Partial<SelectProps>. So the fix should be applied for it instead.

[PaulKristoffersson]

@ZeeshanTamboli Thank you for the answer! I moved my changes to slotProps, and instead of focusing on InputProps I now extend the SelectProps with the different props based on the Select variant. I get no type errors with the component in my local playground with my component as such:

<TablePagination size="small" component="div" count={10} page={page} rowsPerPage={10} onPageChange={handlePageChange} slotProps={{ select: { variant: "filled", size: "small", hiddenLabel: true, disableUnderline: true } }} />

Do you have any suggestion on what test I should make to confirm the behaviour?

[PaulKristoffersson]

@ZeeshanTamboli I have added a test now as well. If you have any inputs on it they are very welcome :)

[ZeeshanTamboli]

This solution functions correctly, but these changes should be made in the Select component's type definitions instead (file Select.d.ts). This is where SelectProps is defined, addressing the root issue. Both TablePagination and TextField utilize SelectProps. Importing input-related components (FilledInput, OutlinedInput) in TablePagination doesn't align logically.

[ZeeshanTamboli]

Can you include the test for issue #38487 in the TablePagination.spec.tsx file?

[PaulKristoffersson]

Can you include the test for issue #38487 in the TablePagination.spec.tsx file?

Added the test 👍

[ZeeshanTamboli]

Looks good to me! Great contribution. However, I'll let @michaldudak, @DiegoAndai, or @mj12albert provide the final review before merging.

[siriwatknp]

Great first PR! 👏

[Methuselah96]

FYI, this is breaking type change for people using SelectProps directly because the variant prop is required now when it wasn't before. I guess BaseSelectProps is a suitable replacement for most use cases?

[ErikBrendel]

Thanks! Using BaseSelectProps instead of SelectProps fixed the build in our component library after updating mui to 5.15.11

[maapteh]

Fixed it in #41359

[smmccabe]

The switch from interface to type also breaks for anyone extending it.

interface MyCustomSelectProps extends SelectProps {} no longer works as types can't be extended but have to use an intersection or a union

[NicerDicerPro]

Also, the variant "standard" broke and is now recognized as a error by typescript:

[damisparks]

I started having issues here.

Type '{ autoWidth?: boolean | undefined; children?: ReactNode; classes?: (Partial<SelectClasses> & Partial<FilledInputClasses>) | undefined; ... 309 more ...; hiddenLabel?: boolean | undefined; } | { ...; } | { ...; }' is not assignable to type 'IntrinsicAttributes & { variant?: "outlined" | undefined; } & Omit<SelectProps<Value, "outlined">, "variant">'.
  Type '{ autoWidth?: boolean | undefined; children?: ReactNode; classes?: (Partial<SelectClasses> & Partial<FilledInputClasses>) | undefined; ... 309 more ...; hiddenLabel?: boolean | undefined; }' is not assignable to type '{ variant?: "outlined" | undefined; }'.
    Types of property 'variant' are incompatible.
      Type '"filled"' is not assignable to type '"outlined"'.

Do you have any suggestions on how to resolve this? It is breaking our CI.

[DiegoAndai]

Hey everyone! I'm sorry for any trouble this might have caused.

As an immediate workaround, if you were using SelectProps directly, please try using BaseSelectProps instead.

Unfortunately, I don't think that would work for @damisparks case. I'll take a look into that. I'll get back to you as soon as possible.

[DiegoAndai]

@damisparks in the meantime, this workaround might help

import {
  Select as MaterialSelect,
  SelectProps as MaterialSelectProps,
  SelectVariants as MaterialSelectVariants,
} from '@mui/material';

type SelectProps<Value, Variant extends MaterialSelectVariants> = { variant?: Variant } & Omit<
  MaterialSelectProps<Value, Variant>,
  'variant'
>;

export default function Select<Value, Variant extends MaterialSelectVariants>(
  props: SelectProps<Value, Variant>,
) {
  return <MaterialSelect<Value, Variant> {...props} />;
}

[damisparks]

Hey everyone! I'm sorry for any trouble this might have caused.

As an immediate workaround, if you were using SelectProps directly, please try using BaseSelectProps instead.

Unfortunately, I don't think that would work for @damisparks case. I'll take a look into that. I'll get back to you as soon as possible.

@DiegoAndai I tried the BaseSelectProps, but it did not work as you suspected. I already tried that before commenting here.

[damisparks]

@damisparks in the meantime, this workaround might help

import {
  Select as MaterialSelect,
  SelectProps as MaterialSelectProps,
  SelectVariants as MaterialSelectVariants,
} from '@mui/material';

type SelectProps<Value, Variant extends MaterialSelectVariants> = { variant?: Variant } & Omit<
  MaterialSelectProps<Value, Variant>,
  'variant'
>;

export default function Select<Value, Variant extends MaterialSelectVariants>(
  props: SelectProps<Value, Variant>,
) {
  return <MaterialSelect<Value, Variant> {...props} />;
}

@DiegoAndai, I appreciate the workaround. It helped to resolve the TS errors.

The final implementation looks like this.

import MuiSelect, {
  type SelectProps as MuiSelectProps,
  type SelectVariants as MuiSelectVariants,
} from "@mui/material/Select";

import copyComponentStaticProps from "../copyComponentStaticProps.js";

// The function `wrapComponent` cannot be used because this component has a
// generic type parameter.

/**
 * A [Material UI](https://mui.com/material-ui) React component for a select
 * input.
 *
 * - [Material UI guide](https://mui.com/material-ui/react-select)
 * - [Material UI API docs](https://mui.com/material-ui/api/select)
 * @param props Props.
 * @returns Select JSX.
 */
function Select<Value, Variant extends MuiSelectVariants>(
  props: {
    variant?: Variant;
  } & Omit<MuiSelectProps<Value, Variant>, "variant">,
) {
  return <MuiSelect<Value, Variant> {...props} />;
}

copyComponentStaticProps(MuiSelect, Select);

export default Select;

[DiegoAndai]

I would expect this to work:

import { Select as MaterialSelect, SelectProps, SelectVariants } from '@mui/material';

export function MySelect<Value, Variant extends SelectVariants>(
  props: SelectProps<Value, Variant>,
) {
  return <MaterialSelect<Value, Variant> {...props} />;
}

But it isn't. Maybe I'm misunderstanding something. Or perhaps it is related to microsoft/TypeScript#10571 / microsoft/TypeScript#23696 / microsoft/TypeScript#26242. If someone figures out why it isn't working, please reach out 😊.

I'll bring this topic to the MUI Core team to gather ideas.

[damisparks]

@DiegoAndai, Should I raise an issue for this topic? Otherwise, I am afraid the conversation will get lost under this PR.

[damisparks]

@DiegoAndai Even though the code below fits the TS errors.

import MuiSelect, {
  type SelectProps as MuiSelectProps,
  type SelectVariants as MuiSelectVariants,
} from "@mui/material/Select";

function Select<Value, Variant extends MuiSelectVariants>(
  props: {
    variant?: Variant;
  } & Omit<MuiSelectProps<Value, Variant>, "variant">,
) {
  return <MuiSelect<Value, Variant> {...props} />;
}

copyComponentStaticProps(MuiSelect, Select);

It does not address choosing a concrete variant for the second generic argument. At the same time, the story (i.e.) Select on the storybook is supposed to be a component with multiple variants and ensure the props table is accurate when the selected variant changes. We are using react-docgen-typescript to generate docs for component props.

Conceptually, MUI has combined multiple components with different props, HTML elements and styles into one Select component. The allowed props are different depending on the variant prop.

It makes sense that the storybook cannot generate the correct props documentation and controls for the MUI Select because it can only generate one set of props to display statically. Still, this component has more than one set of props. It has one set per variant.

[DiegoAndai]

Should I raise an issue for this topic?

@damisparks yes please! Let's open a new issue to continue this discussion. Tag me on it as well. Please add a live reproduction that demonstrates the typescript error. You can also add a live reproduction for the storybook case.

[DiegoAndai]

@damisparks Someone else opened a related issue, but I think it's not quite the same, so let's open yours as well.

[damisparks]

@DiegoAndai, thanks for the update. I will raise an issue and provide minimal storybook case reproduction.

[DiegoAndai]

Hey everyone, we've been discussing this PR's implementation and changes in #41405 as it introduced some regressions. We're thinking of modifying it. If you're interested, please take a look and join the conversation.