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

[Autocomplete] Fix type for multiple values to use readonly arrays #55

Open
pcorpet opened this issue Nov 2, 2023 · 3 comments
Open
Labels
component: autocomplete This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature typescript

Comments

@pcorpet
Copy link

pcorpet commented Nov 2, 2023

Steps to reproduce 🕹

The following should work

  // values accepts const and value has correct type
  useAutocomplete({
    options: ['1', '2', '3'] as const,
    multiple: true,
    value: ['1', '3'] as const,
    onChange(event, value) {
      expectType<ReadonlyArray<'1' | '2' | '3'>, typeof value>(value);
    },
  });

Current behavior 😯

Typescript complains that the value is an immutable array.

Expected behavior 🤔

accept it.

Context 🔦

A fix was proposed in mui/material-ui#38253 and got merged, it got reverted in mui/material-ui#38253 because of mui/material-ui#38817.

The steps proposed by @mnajdova were to

  • move the current useAutocomplete in Material UI
  • iterate on the useAutocomplete in Base UI - test the changes in Joy UI
  • in v6, migrate Material UI's Autocomplete to use the Base UI's useAutocomplete

Your environment 🌎

No response

@pcorpet
Copy link
Author

pcorpet commented Nov 2, 2023

@mnajdova I'm not sure what you mean by moving the "current" useAutocomplete in Material UI:

  • is Material UI, the package in the folder mui-material?
  • are you suggesting that we copy the current version of useAutocomplete to mui-material for instance without exporting it and just using it the Autocomplete component?

@mnajdova
Copy link
Member

mnajdova commented Nov 3, 2023

Thanks for creating the issue @pcorpet. To give some clarification, at this moment Material UI uses the useAutocomplete from Base UI. If we want to make breaking changes to this hook at this point, we can't do it because a stable package depends on it. This is why I said that we should copy the current version to Material UI: /packages/mui-material/ and then we can safely iterate on the Base UI's version of the useAutocomplete. We can replace the useAutocomplete in Material UI in the next major, together with the other planned breaking changes.

@mnajdova mnajdova removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 3, 2023
pcorpet referenced this issue in pcorpet/material-ui Nov 3, 2023
This will allow us to iterate on the base's useAutocomplete types and only release
to Material UI when stable enough.

See #39727
pcorpet referenced this issue in pcorpet/material-ui Nov 3, 2023
There is no reason to mutate the array once passed as a prop or when sending it to onChange.

This is a second attempt of mui#38253, but not affecting Material UI package. See #39727 for context.
@pcorpet
Copy link
Author

pcorpet commented Nov 3, 2023

@mnajdova is mui/material-ui#39738 what you had in mind for the first steps?

@michaldudak michaldudak transferred this issue from mui/material-ui Feb 27, 2024
@michaldudak michaldudak added component: autocomplete This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature typescript
Projects
None yet
Development

No branches or pull requests

4 participants