Using `.indexOf(selectedItem)` will lead to unexpected results when items are dynamically fetched objects

[seanhuggins1]

• downshift version: 7.6.0
• node version: 19.3.0
• npm (or yarn) version: yarn 1.22.19

Relevant code or config

https://github.com/downshift-js/downshift/blob/7200d446d630fd72a2b40b858d8809f8355f5421/src/hooks/utils.js#L272

What you did: I'm using downshift useCombobox to create an async search dropdown component in my application.

Items are fetched from the server, and a new request is made to the server when the user types, the items are filtered server-side based on the user input and then returned to the client, it's a fairly standard setup, and I modelled it after the "Controlling State" example in the docs: https://www.downshift-js.com/use-combobox#controlling-state.

The items passed to useCombobox in this case are objects, ex. I may be fetching a list of books, with

{
  id: string;
  title: string;
}

What happened: After clicking on an item, the search query fires again. The value of the search query sent to the server becomes the name of the selected item, so if I picked "To Kill a Mockingbird", the server would filter all books, & would send back one item to show in the dropdown.

[{
 id: "some_id",
 title: "To Kill a Mockingbird",
}]

I noticed that after the items were replaced, if I clicked on the dropdown again, the selected item would not be highlighted, and would have aria-selected="false"

Problem description: Could be wrong here, but I dug into the code and noticed that, to set the initial highlighted index, we are finding the index of the item items.indexOf(selectedItem).

Here, if items is a list of objects, we're relying on the selectedItem object having the same reference as the corresponding object in the items list.

In the "Controlling State" example, this works fine https://www.downshift-js.com/use-combobox#controlling-state. Because there is a fixed list of objects, so the references never change.

But in the case where objects which are fetched from the server, this can't work, because the object references will change when the new items are fetched. Hence this is why I'm seeing my selected item as unselected, because the reference is different, even though the corresponding object is the same.

In JS, I can do

>> const items = [{id: "1"}];
>> items.indexOf(items[0])
0

But this is obviously broken

>> const items = [{id: "1"}];
>> items.indexOf({id: "1"});
-1

Suggested solution: We could have useCombobox accept a mapping function, say getItemKey, then use this mapping function for a more reliable lookup of the index,

so if we have items with

{ id: string, title: string}

We can have getItemKey={(item) => item.id}

items.map((item) => getItemKey(item)).indexOf(getItemKey(selectedItem))

Temporary workaround Right now, I found a workaround, which is to set defaultHighlightedIndex, which essentially performs the mapping above to the items I fetch from the server

Thanks so much!

[silviuaavram]

Hi! Do you think we could leverage the selectedItemChange prop here?

On Tue, 11 Apr 2023 at 17:27, Sean Huggins @.***> wrote:

• downshift version: 7.6.0
• node version: 19.3.0
• npm (or yarn) version: yarn 1.22.19

Relevant code or config

https://github.com/downshift-js/downshift/blob/7200d446d630fd72a2b40b858d8809f8355f5421/src/hooks/utils.js#L272

What you did: I'm using downshift useCombobox to create an async search dropdown component in my application.

Items are fetched from the server, and a new request is made to the server when the user types, the items are filtered server-side based on the user input and then returned to the client, it's a fairly standard setup, and I modelled it after the "Controlling State" example in the docs: https://www.downshift-js.com/use-combobox#controlling-state.

The items passed to useCombobox in this case are objects, ex. I may be fetching a list of books, with

{ id: string; title: string; }

What happened: After clicking on an item, the search query fires again, this time the value of the search to filter for is the same as the name of the selected option, so if I picked "To Kill a Mockingbird", the server would filter all books, & would send back one item to show in the dropdown.

[{ id: "some_id", title: "To Kill a Mockingbird", }]

I noticed that after the items were replaced, if I clicked on the dropdown again, the selected item would not be highlighted, and would have aria-selected="false"

Problem description: Could be wrong here, but I dug into the code and noticed that, to set the initial highlighted index, we are finding the index of the item items.indexOf(selectedItem).

Here, if items is a list of objects, we're relying on the selectedItem object having the same reference as the corresponding object in the items list.

In the "Controlling State" example, this works fine https://www.downshift-js.com/use-combobox#controlling-state. Because there is a fixed list of objects, so the references never change.

But in the case where objects which are fetched from the server, this can't work, because the object references will change when the new items are fetched. Hence this is why I'm seeing my selected item as unselected, because the reference is different, even though the corresponding object is the same.

In JS, I can do

const items = [{id: "1"}]; items.indexOf(items[0]) 0

But this is obviously broken

const items = [{id: "1"}]; items.indexOf({id: "1"}); -1

Suggested solution: We could have useCombobox accept a mapping function, say getItemKey, then use this mapping function for a more reliable lookup of the index,

so if we have items with

{ id: string, title: string}

We can have getItemKey={(item) => item.id}

items.map((item) => getItemKey(item)).indexOf(getItemKey(selectedItem))

Temporary workaround Right now, I found a workaround, which is to set defaultHighlightedIndex, which essentially performs the mapping above to the items I fetch from the server

Thanks so much!

— Reply to this email directly, view it on GitHub https://github.com/downshift-js/downshift/issues/1495, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACWAZAFHOX2C5TOCP4W3TC3XAXEEVANCNFSM6AAAAAAW2Z3B4U . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[seanhuggins1]

Thanks for the reply. I can't think of how that would help. Did you have an idea in mind?

[silviuaavram]

Since it’s the callback we use to check for items equality, I thought it could be useful here as well. I will look into it if it’s not something obvious right now.

On Tue, 11 Apr 2023 at 18:02, Sean Huggins @.***> wrote:

Hmm, I can't think of how that would help. Did you have an idea in mind?

— Reply to this email directly, view it on GitHub https://github.com/downshift-js/downshift/issues/1495#issuecomment-1504174619, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACWAZAGR5LC5R2PEGYM4D7LXAXIF5ANCNFSM6AAAAAAW2Z3B4U . You are receiving this because you commented.

[seanhuggins1]

Ah yeah I see, looks like that function could be a nice way to test equality of items

So rather than items.indexOf(selectedItem)

you could have

items.findIndex((item) => !selectedItemChanged(item, selectedItem))

Although selectedItemChanged isn't the best name for what this achieves here.

Essentially equivalent to having:

items.findIndex((item) => getItemKey(item) === getItemKey(selectedItem))

[seanhuggins1]

Could introduce getItemKey or maybe itemToKey (so that it's similar to itemToString) as an optional prop to make it a non-breaking change.

Then wherever we have items.indexOf(selectedItem), could replace with:

!!getItemKey ? 
  items.map((item) => getItemKey(item)).indexOf(getItemKey(selectedItem)) : items.indexOf(selectedItem)

[Noyabronok]

1. itemToKey is a good idea.
2. I had a simpler case with this issue - was resetting a static list of items inside the component, and wrapping it with useMemo fixed the comparison.
3. But for the OP case, you could keep a reference to the selected object in state, and make sure that this item was used instead of the same value item retrieved from the server.

[silviuaavram]

I think itemToKey is a good idea as well. We can have it in a future update, and we can get rid of selectedItemChange in the process.

It will be a breaking change, but if it's a good change, then we should do it.