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

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

Merged

Conversation

PaulKristoffersson
Copy link
Contributor

@PaulKristoffersson PaulKristoffersson commented Sep 24, 2023

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.

@mui-bot
Copy link

mui-bot commented Sep 24, 2023

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

@danilo-leal danilo-leal changed the title fix(TablePagination): remove type error props in SelectProps [material-ui][Table Pagination] Remove type error props in SelectProps Sep 24, 2023
@danilo-leal danilo-leal added package: material-ui Specific to @mui/material typescript component: TablePagination The React component. labels Sep 24, 2023
@michaldudak
Copy link
Member

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 PaulKristoffersson force-pushed the fix-type-error-tablepagination branch 2 times, most recently from 2c9d6d2 to fe22382 Compare October 3, 2023 14:34
@PaulKristoffersson
Copy link
Contributor Author

@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

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@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
Copy link
Contributor Author

@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
Copy link
Member

ZeeshanTamboli commented Oct 19, 2023

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Member

ZeeshanTamboli commented Nov 30, 2023

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
Copy link
Contributor Author

PaulKristoffersson commented Dec 5, 2023

@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
Copy link
Contributor Author

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

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

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.

@PaulKristoffersson PaulKristoffersson force-pushed the fix-type-error-tablepagination branch 2 times, most recently from 096a693 to e50fb50 Compare December 13, 2023 14:18
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

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

@PaulKristoffersson
Copy link
Contributor Author

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

Added the test 👍

Comment on lines +180 to +188
export type SelectProps<
Value = unknown,
Variant extends SelectVariants = SelectVariants,
> = BaseSelectProps<Value> &
(Variant extends 'filled'
? FilledSelectProps
: Variant extends 'standard'
? StandardSelectProps
: OutlinedSelectProps);
Copy link
Contributor

@Methuselah96 Methuselah96 Feb 25, 2024

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

Fixed it in #41359

@smmccabe
Copy link

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
Copy link

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

@damisparks
Copy link

I started having issues here.
image
image

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
Copy link
Member

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
Copy link
Member

@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
Copy link

damisparks commented Mar 1, 2024

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
Copy link

@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
Copy link
Member

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
Copy link

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

@damisparks
Copy link

damisparks commented Mar 4, 2024

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

image

@DiegoAndai
Copy link
Member

DiegoAndai commented Mar 4, 2024

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
Copy link
Member

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

@damisparks
Copy link

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

@DiegoAndai
Copy link
Member

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.

sjdemartini added a commit to sjdemartini/mui-tiptap that referenced this pull request Jul 2, 2024
As of
mui/material-ui#39137, MUI's `SelectProps` is
now a `type` and not an `interface`, so we can no longer `extend` it for
our own `MenuSelectProps`. As such, we now make `MenuSelectProps` a
`type` and use `&` syntax to inject additional props. This should
support both MUI < v5.15.11 (where `SelectProps` is an `interface`) and
>= 5.15.11 (where `SelectProps` is a `type`).
sjdemartini added a commit to sjdemartini/mui-tiptap that referenced this pull request Jul 2, 2024
As of
mui/material-ui#39137, MUI's `SelectProps` is
now a `type` and not an `interface`, so we can no longer `extend` it for
our own `MenuSelectProps`. As such, we now make `MenuSelectProps` a
`type` and use `&` syntax to inject additional props. This should
support both MUI < v5.15.11 (where `SelectProps` is an `interface`) and
>= 5.15.11 (where `SelectProps` is a `type`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! component: TablePagination The React component. package: material-ui Specific to @mui/material typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TablePagination][material-ui] Type error for hiddenLabel prop in SelectProps