[Autocomplete] Wrong onChange value type with wrapper and multiline #25502

JanKaczmarkiewicz · 2021-03-25T15:50:44Z

Hi, First of all, I really love this library. Thanks for Your job :)

I am trying to extend Autocomplete type based on https://github.com/mui-org/material-ui/blob/1fd983abd9c26d3d751f99d2efbfa2e5ab063786/packages/material-ui-lab/src/Autocomplete/Autocomplete.spec.tsx
And I have a problem with onChange value type

Current Behavior 😯

onChange callback value arg type is T | T[] (without multiple prop)

Expected Behavior 🤔

onChange callback value arg should have just T type (without multiple prop)

Steps to Reproduce 🕹

https://codesandbox.io/s/stupefied-meadow-6c98o

oliviertassinari · 2021-03-25T18:14:57Z

The pain seems to be, more or less, the same as #25013. Here is a failing test case:

diff --git a/packages/material-ui/src/Autocomplete/Autocomplete.spec.tsx b/packages/material-ui/src/Autocomplete/Autocomplete.spec.tsx
index 89dda93512..4a8a529de7 100644
--- a/packages/material-ui/src/Autocomplete/Autocomplete.spec.tsx
+++ b/packages/material-ui/src/Autocomplete/Autocomplete.spec.tsx
@@ -33,6 +33,14 @@ function MyAutocomplete<
   multiple
 />;

+<MyAutocomplete
+  options={['1', '2', '3']}
+  onChange={(event, value) => {
+    expectType<string | null, typeof value>(value);
+  }}
+  renderInput={() => null}
+/>;
+
 interface Tag {
   color: string;
   label: string;

I don't have the faintest idea as to where to start looking to fix it. If you have a solution, please submit a pull request. It would be great.

JanKaczmarkiewicz · 2021-03-25T19:46:05Z

I found solutions :)

(component use fix) Explicitly pass props

   multiple={false} or multiple={undefined} 
   freeSolo={false} or freeSolo={undefined}
   disableUnderline={false} or disableUnderline={undefined}

or

(component definition fix) assign defaults to component definition generics

  Multiple extends boolean | undefined = undefined,
  DisableClearable extends boolean | undefined = undefined,
  FreeSolo extends boolean | undefined = undefined

I think the second option is better because it will be "global" and much cleaner:)

Explanation
The reason for this odd behaviour is that if we don't pass default type to generic (Multiple extends boolean | undefined [ without = undefined]) and without explicit prop passing value type [without multiple={false}]
Type for generic Multiple will be boolean | undefined and this causes problems later in here:

export type Value<T, Multiple, DisableClearable, FreeSolo> = Multiple extends undefined | false
  ? DisableClearable extends true
    ? NonNullable<T | AutocompleteFreeSoloValueMapping<FreeSolo>>
    : T | null | AutocompleteFreeSoloValueMapping<FreeSolo>
  : Array<T | AutocompleteFreeSoloValueMapping<FreeSolo>>;

But why it is working on pure Autocompleter
The current Autocompleter component works as expected because it has correct default types:

export default function Autocomplete<
  T,
  Multiple extends boolean | undefined = undefined,
  DisableClearable extends boolean | undefined = undefined,
  FreeSolo extends boolean | undefined = undefined
>(props: AutocompleteProps<T, Multiple, DisableClearable, FreeSolo>): JSX.Element;

Now I don't think that this is a bug anymore but I will open a pull request to add test cases and update Autocomplete.spec.tsx with the second option for future reference :) If you don't mind ofc. Feel free to close this issue.

I hope it will help someone. Have a great day! :)

oliviertassinari · 2021-03-25T22:49:00Z

@JanKaczmarkiewicz Great, thank you for spending time on this problem. So, if I understand correctly, this is doing it:

diff --git a/packages/material-ui/src/Autocomplete/Autocomplete.spec.tsx b/packages/material-ui/src/Autocomplete/Autocomplete.spec.tsx
index 89dda93512..2cba2349ab 100644
--- a/packages/material-ui/src/Autocomplete/Autocomplete.spec.tsx
+++ b/packages/material-ui/src/Autocomplete/Autocomplete.spec.tsx
@@ -16,9 +16,9 @@ interface MyAutocompleteProps<

 function MyAutocomplete<
   T,
-  Multiple extends boolean | undefined,
-  DisableClearable extends boolean | undefined,
-  FreeSolo extends boolean | undefined
+  Multiple extends boolean | undefined = undefined,
+  DisableClearable extends boolean | undefined = undefined,
+  FreeSolo extends boolean | undefined = undefined
 >(props: MyAutocompleteProps<T, Multiple, DisableClearable, FreeSolo>) {
   return <Autocomplete {...props} />;
 }
@@ -33,6 +33,14 @@ function MyAutocomplete<
   multiple
 />;

+<MyAutocomplete
+  options={['1', '2', '3']}
+  onChange={(event, value) => {
+    expectType<string | null, typeof value>(value);
+  }}
+  renderInput={() => null}
+/>;
+
 interface Tag {
   color: string;
   label: string;

I will open a pull request to add test cases and update

Go for it :)

JanKaczmarkiewicz added the status: needs triage label Mar 25, 2021

oliviertassinari added component: Autocomplete typescript and removed status: needs triage labels Mar 25, 2021

oliviertassinari changed the title ~~Typescript MyAutocomplete onChange value wrong type~~ [Autocomplete] Wrong onChange value type with wrapper and multiline Mar 25, 2021

oliviertassinari added the good first issue label Mar 25, 2021

mui-org / material-ui

[Autocomplete] Wrong onChange value type with wrapper and multiline #25502

[Autocomplete] Wrong onChange value type with wrapper and multiline #25502

JanKaczmarkiewicz commented Mar 25, 2021 •

edited

oliviertassinari commented Mar 25, 2021

JanKaczmarkiewicz commented Mar 25, 2021 •

edited

oliviertassinari commented Mar 25, 2021

mui-org / material-ui

Sponsor mui-org/material-ui

[Autocomplete] Wrong onChange value type with wrapper and multiline #25502

[Autocomplete] Wrong onChange value type with wrapper and multiline #25502

Comments

JanKaczmarkiewicz commented Mar 25, 2021 • edited

Current Behavior 😯

Expected Behavior 🤔

Steps to Reproduce 🕹

oliviertassinari commented Mar 25, 2021

JanKaczmarkiewicz commented Mar 25, 2021 • edited

oliviertassinari commented Mar 25, 2021

JanKaczmarkiewicz commented Mar 25, 2021 •

edited

JanKaczmarkiewicz commented Mar 25, 2021 •

edited