[material-ui][Autocomplete] Component with `freeSolo` and `isOptionEqualToValue` doesn't use string for type of value

[WillSmithTE]

Duplicates

• [X] I have searched the existing issues

Latest version

• [X] I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/jolly-sid-x28zcd?file=/src/App.tsx

Steps:

1. Type something into the input
2. Press enter
3. Click on the input again

Current behavior 😯

Page crashes because of error Cannot read properties of undefined (reading 'id')

Expected behavior 🤔

Typescript tells me that value in isOptionEqualToValue could be a string

Context 🔦

I have a similar example to this in my project with a nested object, and just discovered it was crashing because I don't check if the value is a string. onChange seems to use the correct types of S | string.

Your environment 🌎

Using the mui template codesandbox, which is a browser sandbox not cloud one.

[Abhushit]

You can use Optional Chaining Operator like this, I think this will solve your probelm.

[konrazem]

I see this issue is fresh, so I will not do fouther searching. I think I have related problem. I need controlled, async autocomplete. The problem is that I want to show title as label but inputValue need to be id. This is because I can have many elements with the same title like

{ id: '1', title: "John Doe", age: 23 }, { id: '2', title: "John Doe", age: 45 }, ... ]

In example you will notice error There is no film with id "The Dark Knight"". Do you know maybe how to fix it? https://codesandbox.io/s/thirsty-sun-59m6vg?file=/Demo.tsx

I am sorry if I am hijacking this issue I can create new one or do fouther searching 😊

[konrazem]

onInputChange as second argument returns label. I need to tell Autocomplete which field is unique and this is not a field returned by getOptionLabel. Shortly speaking: in most cases, you do use id as label.

It would be usefull prop like getRowId (getOptionId) in DataGrid

[WillSmithTE]

Thanks @Abhushit but I know how to fix the runtime error, this is about catching it earlier with typings

[WillSmithTE]

Maybe this

  isOptionEqualToValue?: (option: Value, value: Value) => boolean;

needs to become

  isOptionEqualToValue?: (option: Value, value: AutocompleteValue<Value, Multiple, DisableClearable, FreeSolo>) => boolean;

[michaldudak]

@WillSmithTE The value type in isOptionEqualToValue should be Value | AutocompleteFreeSoloValueMapping<FreeSolo>, similarly to what's in getOptionLabel. Would you like to open a PR?

EDIT: Actually, there is a PR that aims to solve this: #37291

[ZeeshanTamboli]

@WillSmithTE But why do you need to use isOptionEqualToValue prop when the Autocomplete is freeSolo?

[WillSmithTE]

@ZeeshanTamboli actually I inherited the code so I'm not sure. Our tests are still green if I remove it, but we reuse the same component which uses it a lot, so I'm hesitant to remove it without doing more testing.

[joshuakb2]

@WillSmithTE But why do you need to use isOptionEqualToValue prop when the Autocomplete is freeSolo?

In my case, the option type is an object so we need isOptionEqualToValue to judge whether the value the user has typed matches the unique ID of an existing option. But we still want to allow the user to type a new value themselves, and when they press enter we open a dialog to help them finish creating the new option.

[ZeeshanTamboli]

It could be a breaking change. See https://github.com/mui/material-ui/pull/37291#pullrequestreview-1747951842.

[WillSmithTE]

You could argue that almost every bug fix is a breaking change

[newsiberian]

I think this not necessary will a breaking change for current users, because if they already faced w/ that, they probably came up w/ some sort of type guard to fix original bug. wdyt?

[newsiberian]

Also, wdyt about such solution:

getOptionLabel?: (option: FreeSolo extends true ? AutocompleteFreeSoloValueMapping<FreeSolo> : T) => string;

isOptionEqualToValue?: (option: T, value: FreeSolo extends true ? AutocompleteFreeSoloValueMapping<FreeSolo> : T) => boolean;

I've done a briefly check and it looks like it works perfectly

[ZeeshanTamboli]

You could argue that almost every bug fix is a breaking change

Certainly. The key is in the degree of impact. I'd like other's opinion - @michaldudak @DiegoAndai @mj12albert

---

Also, wdyt about such solution:

getOptionLabel?: (option: FreeSolo extends true ? AutocompleteFreeSoloValueMapping<FreeSolo> : T) => string;

isOptionEqualToValue?: (option: T, value: FreeSolo extends true ? AutocompleteFreeSoloValueMapping<FreeSolo> : T) => boolean;

I've done a briefly check and it looks like it works perfectly

@newsiberian I also checked but it still errors. For example if someone has:

isOptionEqualToValue={(option, value) => option.book.id === value?.book?.id}

TypeScript will throw:

Property 'book' does not exist on type 'string'.

[newsiberian]

@newsiberian I also checked but it still errors. For example if someone has:

as long as I understand, if you have freeSolo prop enabled, you can't receive such value in this method, it could be a string type only, thus, you can't use such condition, am I wrong here?

[newsiberian]

I've checked how autocomplete works and freeSolo basically means that if you type something and press "Enter" or blur*, the value will be a string, but if you select exact option from the list by clicking it, value will be an option object then, so yes, @ZeeshanTamboli, conditional type doesn't fit here

[newsiberian]

conditional type doesn't fit here

Or it does 😄 @ZeeshanTamboli , @DiegoAndai , could you please take a look at such conditional vvv

Subject: [PATCH] introduce AutocompleteValueOrFreeSoloValueMapping
---
Index: docs/data/material/components/autocomplete/FreeSolo.tsx
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/docs/data/material/components/autocomplete/FreeSolo.tsx b/docs/data/material/components/autocomplete/FreeSolo.tsx
--- a/docs/data/material/components/autocomplete/FreeSolo.tsx   (revision 5afa2dacb3cc15b22f1cbb8b56eaeeda4f4535d7)
+++ b/docs/data/material/components/autocomplete/FreeSolo.tsx   (date 1701234535623)
@@ -28,6 +28,20 @@
           />
         )}
       />
+      <Autocomplete
+        id="free-solo-demo3"
+        freeSolo
+        options={top100Films}
+        renderInput={(params) => <TextField {...params} label="freeSolo" />}
+        getOptionLabel={option => typeof option ==='string' ? option : option.title}
+        // this demo demonstrates how the value can be of both types, typeof option and a string
+        isOptionEqualToValue={(option, value) => {
+          if (typeof value === 'string') {
+            return option.title === value
+          }
+          return option.title === value?.title
+        }}
+      />
     </Stack>
   );
 }
Index: packages/mui-base/src/useAutocomplete/useAutocomplete.d.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/packages/mui-base/src/useAutocomplete/useAutocomplete.d.ts b/packages/mui-base/src/useAutocomplete/useAutocomplete.d.ts
--- a/packages/mui-base/src/useAutocomplete/useAutocomplete.d.ts    (revision 5afa2dacb3cc15b22f1cbb8b56eaeeda4f4535d7)
+++ b/packages/mui-base/src/useAutocomplete/useAutocomplete.d.ts    (date 1701234720907)
@@ -27,6 +27,8 @@

 export type AutocompleteFreeSoloValueMapping<FreeSolo> = FreeSolo extends true ? string : never;

+export type AutocompleteValueOrFreeSoloValueMapping<Value, FreeSolo> = FreeSolo extends true ? Value | string : Value;
+
 export type AutocompleteValue<Value, Multiple, DisableClearable, FreeSolo> = Multiple extends true
   ? Array<Value | AutocompleteFreeSoloValueMapping<FreeSolo>>
   : DisableClearable extends true
@@ -169,11 +171,11 @@
    *
    * If used in free solo mode, it must accept both the type of the options and a string.
    *
-   * @param {Value} option
+   * @param {AutocompleteValueOrFreeSoloValueMapping<Value, FreeSolo>} option
    * @returns {string}
    * @default (option) => option.label ?? option
    */
-  getOptionLabel?: (option: Value | AutocompleteFreeSoloValueMapping<FreeSolo>) => string;
+  getOptionLabel?: (option: AutocompleteValueOrFreeSoloValueMapping<Value, FreeSolo>) => string;
   /**
    * If provided, the options will be grouped under the returned string.
    * The groupBy value is also used as the text for group headings when `renderGroup` is not provided.
@@ -209,12 +211,12 @@
    * ⚠️ Both arguments need to be handled, an option can only match with one value.
    *
    * @param {Value} option The option to test.
-   * @param {Value|AutocompleteFreeSoloValueMapping<FreeSolo>} value The value to test against.
+   * @param {AutocompleteValueOrFreeSoloValueMapping<Value, FreeSolo>} value The value to test against.
    * @returns {boolean}
    */
   isOptionEqualToValue?: (
     option: Value,
-    value: Value | AutocompleteFreeSoloValueMapping<FreeSolo>,
+    value: AutocompleteValueOrFreeSoloValueMapping<Value, FreeSolo>,
   ) => boolean;
   /**
    * If `true`, `value` must be an array and the menu will support multiple selections.

The idea here: is to leave Value as typeof option for non-freeSolo cases and use Value | string for freeSolo cases. And perhaps such changes might be not really a breaking changes

[ZeeshanTamboli]

@newsiberian Regarding this part of code from above comment:

          isOptionEqualToValue={(option, value) => {
            if (typeof value === 'string') {
              return option.title === value;
            }
            return option.title === value?.title;
          }}

This is what I mentioned in https://github.com/mui/material-ui/pull/37291#pullrequestreview-1747951842. User will have to update to use type narrowing, which would be a breaking change for minor or patch release.

[joshuakb2]

@newsiberian Regarding this part of code from above comment:

          isOptionEqualToValue={(option, value) => {
            if (typeof value === 'string') {
              return option.title === value;
            }
            return option.title === value?.title;
          }}

This is what I mentioned in https://github.com/mui/material-ui/pull/37291#pullrequestreview-1747951842. User will have to update to use type narrowing, which would be a breaking change for minor or patch release.

Is it a breaking change to start telling the user about a bug in their code when we were previously silent about it? Any code that fails to type check after this change was already broken to begin with, right?

[newsiberian]

@ZeeshanTamboli , I don't see that in that way. Because, if you are using typescript and you use freeSolo prop, you would anyway wrote you function that way, otherwise it will just throw you an exception, this will be a JS bug, but additionally you will have to add something like @ts-ignore because in a current implementation the second argument can't have type other than Value type.

So, again, those changes can be breaking only in a way that you will be able to remove @ts-ignore from your code

[ZeeshanTamboli]

Is it a breaking change to start telling the user about a bug in their code when we were previously silent about it? Any code that fails to type check after this change was already broken to begin with, right?

@joshuakb2 You're right, and I'm okay with implementing this change. I pointed out that it might cause inconvenience to users, but I think the final decision should also be made by other MUI team members and come to an agreement, as it could necessitate users updating their code especially for minor or patch release, even if it was incorrect before. Maybe we could release this in a minor version.

@newsiberian, go ahead and submit your changes in the pull request. We'll check if all tests pass. If you haven't already, please add new TypeScript tests. Thanks.