Skip to content

[Autocomplete] Rename getOptionSelected to isOptionEqualToValue#26173

Merged
m4theushw merged 5 commits into
mui:nextfrom
m4theushw:rename-autocomplete-getOptionSelected
May 17, 2021
Merged

[Autocomplete] Rename getOptionSelected to isOptionEqualToValue#26173
m4theushw merged 5 commits into
mui:nextfrom
m4theushw:rename-autocomplete-getOptionSelected

Conversation

@m4theushw

@m4theushw m4theushw commented May 6, 2021

Copy link
Copy Markdown
Collaborator

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

mui-pr-bot commented May 6, 2021

Copy link
Copy Markdown

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 9089962

@eps1lon eps1lon left a comment

Copy link
Copy Markdown
Member

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

oliviertassinari commented May 7, 2021

Copy link
Copy Markdown
Member

@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 Introduces changes that are not backward compatible. scope: autocomplete Changes related to the autocomplete. This includes ComboBox. labels May 7, 2021

@eps1lon eps1lon left a comment

Copy link
Copy Markdown
Member

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

oliviertassinari commented May 7, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
Collaborator 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

oliviertassinari commented May 7, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
Collaborator Author

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

@oliviertassinari

oliviertassinari commented May 12, 2021

Copy link
Copy Markdown
Member

@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

m4theushw commented May 12, 2021

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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 Introduces changes that are not backward compatible. scope: autocomplete Changes related to the autocomplete. This includes ComboBox.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[autocomplete] Rename getOptionSelected to optionEqualValue

4 participants