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] Rename getOptionSelected to isOptionEqualToValue #26173

Merged
merged 5 commits into from
May 17, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented May 6, 2021

Breaking changes

  • [Autocomplete] Rename getOptionSelected to isOptionEqualToValue to better describe its purpose.

    <Autocomplete
    -  getOptionSelected={(option, value) => option.title === value.title}
    +  isOptionEqualToValue={(option, value) => option.title === value.title}

Part of #20012
Closes #24855

In the RFC, the suggestion was optionEqualValue, but I think isOptionEqualToValue is a better name. It was suggested in #24855 (comment).

@m4theushw m4theushw changed the title [Autocomplete] Rename getOptionSelected to isOptionEqualToValue [Autocomplete] Rename getOptionSelected to isOptionEqualToValue May 6, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented May 6, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 9089962

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Isn't this function just areOptionsEqual<T>(optionA: T, optionB: T): boolean? As far as I can tell value is synonymous with selectedOption.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 7, 2021

@eps1lon This sounds about right. The types are the same. In #23708 we talk about separating them. IMHO, the main value of the isOptionEqualToValue's name is to highlight that use case. The method is always used to compare one selected value with one option.

@oliviertassinari oliviertassinari added breaking change component: autocomplete This is the name of the generic UI component, not the React module! labels May 7, 2021
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

The name is leaking implementation details. We should use the same name you would use in React.memo.

areOptionsEqual is a better name that isn't tied to the current implementation.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 7, 2021

The nominal use case for the prop we are discussing is when the value and options props are fetched from the database into two different requests. They have a different JS reference but the same type (type might change with #23708),

I don't follow the implementation detail aspect. This comparison prop only exists because there are two different props: value and options. For comparing two options from options, using the reference is enough.

@m4theushw
Copy link
Member Author

areOptionsEqual is a better name that isn't tied to the current implementation.

We're not comparing options with options. This function compares an option with the selected value (or values, if multiple).

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 7, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented May 7, 2021

So I guess this is a matter of angle we pick to look at the problem.

  • If we consider it from a type perspective, then areOptionsEqual is more accurate.
  • If we consider the core of the problem solved by the prop, then isOptionEqualToValue is less confusing.

I would vote for giving less weight to the type in this very instance as we have indicators in #23708 that sharing the same type might not be ideal.

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels May 7, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 12, 2021
@m4theushw
Copy link
Member Author

May I push forward this one? I feel we didn't come to an agreement about the name: areOptionsEqual or isOptionEqualToValue.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 12, 2021

@m4theushw What's your preference on the naming Y?

Looking a bit closer at #23708. If we implement a getOptionValue prop like react-select has, then it will impact the behavior of Y. Effectively, we will then be able to use getOptionValue to remove the need for developers to implement Y, as we would be able to use getOptionValue most of the time, returning a plain string|number we can strictly compare (===). If we do such, it reinforces the idea that the semantic of isOptionEqualValue is less confusing for Y but also makes the prop less important (no need for using it in the demos anymore).

In https://select2.org/getting-started/basic-usage, it's actually simpler, they force you to have the equivalent of getOptionValue, so there is no concepts of Y.

@m4theushw
Copy link
Member Author

m4theushw commented May 12, 2021

Just looking on what we have now, I would rename to isOptionEqualToValue. Although the argument types are the same, the value may not have the same shape of option in an application, so it's good to make a distinction between the option and the value.

If we implement the getOptionValue prop, the isOptionEqualToValue prop will be less important. I can think in only one situation where it's needed, which is when the value is an object and the options change over time (e.g. async loaded options). We have only one demo using it and I think we should keep it.

@oliviertassinari
Copy link
Member

Just looking on what we have now, I would rename to isOptionEqualToValue

Ok, let's move forward with this then.

@m4theushw m4theushw merged commit 71452ff into mui:next May 17, 2021
@m4theushw m4theushw deleted the rename-autocomplete-getOptionSelected branch May 17, 2021 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: autocomplete This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] Rename getOptionSelected to optionEqualValue
4 participants