Material-ui: [Autocomplete] Warn wrong comparison getOptionSelected implementation

[tagaychinova]

Bug Demo: https://codesandbox.io/s/material-demo-q3fno

[oliviertassinari]

It's a duplicate of #18499.

[oliviertassinari]

@tagaychinova Interesting case, I propose the following new error message:

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
index dfd6171c8..47e2a4c59 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js
@@ -439,6 +439,24 @@ export default function useAutocomplete(props) {
       const item = newValue;
       newValue = Array.isArray(value) ? [...value] : [];

+      if (process.env.NODE_ENV !== 'production') {
+        let matches = 0;
+        for (let i = 0; i < newValue.length; i += 1) {
+          if (getOptionSelected(item, newValue[i])) {
+            matches += 1;
+          }
+        }
+
+        if (matches > 1) {
+          console.error(
+            [
+              'Material-UI: the `getOptionSelected` method of useAutocomplete do not handle the arguments correctly.',
+              `The component expects a single value to match a given option but found ${matches}.`,
+            ].join('\n'),
+          );
+        }
+      }
+
       const itemIndex = findIndex(newValue, valueItem => getOptionSelected(item, valueItem));

       if (itemIndex === -1) {

[ahmad-reza619]

@oliviertassinari why not we try to solve this issue instead of giving error message?

[oliviertassinari]

@ahmad-reza619 This error message is a lead/tip on how to solve the problem in your codebase. If you can't figure it out with it, then this warning might not be good enough. I wonder what lead to this wrong usage of the API and how we could better help 🤔.

[oliviertassinari]

Maybe we should rename getOptionSelected to optionEqualValue or something similar 🤔.

[ahmad-reza619]

Maybe we should rename getOptionSelected to optionEqualValue or something similar 🤔.

sounds good, and btw why not replace the for with Array.prototype.some for edgy fp style 😎

[ahmad-reza619]

something like this

const matches = newValue.some(val => optionEqualValue(item, val));

if (matches) {
   console.error(
        [
            'Material-UI: the `getOptionSelected` method of useAutocomplete do not handle the arguments correctly.',
            `The component expects a single value to match a given option but found ${matches}.`,
        ].join('\n')
   )
}

[eps1lon]

console.error

I'd prefer a warning here and I'm not even sure there aren't valid use cases for this e.g. fallbacks.

And I'm not sure why this should warn. What is wrong with the provided codesandbox? It seems like a bug on our side to me.

[oliviertassinari]

@ahmad-reza619 What about IE 11?
@eps1lon makes me think of a previous discussion we didn't resolve. When should we use error vs warn?

Ok, here is the solution. The getOptionSelected method accepts two arguments. A value and an option. Its purpose is to signal when two objects from these ensembles are identical. We use it to construct a function that takes an option and maps it to the corresponding value. You can't say option A points to all the values.

[eps1lon]

You can't say option A points to all the values.

How does this apply here? Each option and current value is unique in the codesandbox.

Ok, here is the solution. The getOptionSelected method accepts two arguments.

Tried that in the codesandbox. The second parameter is undefined.

[oliviertassinari]

I hope it will be clearer with https://codesandbox.io/s/material-demo-b3nsz.

-getOptionSelected={option => value.find(v => v.id === option.id)}
+getOptionSelected={(option, value) => value.id === option.id}

[ahmad-reza619]

@oliviertassinari Ok sorry for misunderstanding, but looks like Array.prototype.some is actually supported in IE based on this MDN article.

But well yeah it only return true if there is one of its predicate is true 🤔

[ahmad-reza619]

Can i give it a go?

[oliviertassinari]

Perfect for IE 11 :) but I think that it could be interesting display the number of matches.

[oliviertassinari]

Honestly, I'm not sure the warning will be enough. It took me a good chunk of time to figure out what was wrong with your example. Considering I have myself designed most of the component, I can only imagine how it can be confusing for somebody else.
@eps1lon what do you think about changing the name of the prop?

[scrozier]

FWIW, I would favor (also) renaming the prop, as you suggested, @oliviertassinari. It specifically does not do what it name implies. I have to rethink it every time I come back to it.

BTW, I love this component. :-)

[oliviertassinari]

@scrozier What name you would suggest?

[scrozier]

@oliviertassinari It's tricky, isn't it? I wonder if that means that there's an underlying architectural issue? You're really wanting to find out which option is selected, ergo getOptionSelected, but this particular prop does not do that, it's just a supporting player. optionEqualValue seems pretty good, if not exactly elegant. Other thoughts: optionEquivalence, optionMatch, optionMatcher, optionFinder, optionByValue. I don't think I know the body of work well enough to know what fits with the semantic direction. I think I land on optionByValue or your original optionEqualValue, without a strong opinion either way.

BTW, thank you for your shepherding of this excellent project. You do a great job.

[oliviertassinari]

@scrozier Both options sound great.

[scrozier]

@oliviertassinari Should I open an issue for this?

[oliviertassinari]

@scrozier Sure, if you want to submit a pull request directly, that will do it too :)

[scrozier]

@oliviertassinari I shall give it a whirl....