Eui: Incorrect row stays expanded after sorting an EuiInMemoryTable

[peteharverson]

For an EuiInMemoryTable with expanded rows, if you expand a row, then sort the rows by clicking on a column header, the same row number stays expanded, showing the content for the item that was previously in that position prior to sorting. The expanded content should move to its new page/row position, or alternatively all rows should be collapsed on sorting.

e.g. expand first row, sorted by decreasing severity:

then sort by increasing severity - first row stays expanded, but showing content for previous item in that position:

Found in version 0.0.46

[chandlerprall]

I don't think we can properly support sortable columns when the items don't have associated IDs. The IDs we auto-assign are based on their position in the data, and sorting fundamentally alters this association.

You've said before that this data has no guaranteed, unique identifier. You could either stamp the item's original index within the data as its ID, or you could get a hash of the object (e.g. with object-hash to use instead.

I can also add a check to EuiTable that console.errors when sorting is enabled but the first (any?) item doesn't return an item id when passed to EuiBasicTable::itemId to help catch and resolve this case in the future.

[peteharverson]

Looking at the code for the renderItemRow() function in basic_table.js, am I right in thinking that if selection is not enabled on the table (which it isn't in our case), then the rowIndex is used to check whether the row is expanded in itemIdToExpandedRowMap. In which case, any itemId property I set on the row data would be ignored?

If so, should this behavior be changed, allowing for an itemId property to be checked even if selection is not enabled on the table?

Or as a workaround is there some way of collapsing all rows if the user does a sort?

[chandlerprall]

Indeed. As itemId only works if selection is present (as selection describes how to get the item's id), and passing selection has other side effects, I think the best option is to expose a catch-all table update that can be used to clear the expanded item map.

EuiBasicTable implements an onChange callback that happens anytime sorting or pagination is updated, and EuiInMemoryTable can be made to expose the same, and you can listen for that event and update the expanded item map. How does that sound?

[peteharverson]

Is using itemId for tables without selection an option? Or is the only viable option clearing the expanded item map if the user sorts or pages the data? The functionality does currently work in EuiInMemoryTable with pagination, so losing that by clearing the expanded item map in onChange to prevent it breaking on sorting would seem a shame.

[chandlerprall]

I wouldn't want to assume a field named itemId is present, and it also prescribes the incoming data's format.

Adding a new property onto the table that duplicates selection's functionality is possible but care would be needed to add proptype logic that selection and the new prop are mutually exclusive. It also adds more complexity to the table code.

As the table components' functionality includes a dependency on having IDs outside of selectables, and providing the UI consistency around if/when expanded rows collapse, I do think adding a more generic itemId makes sense. I wonder if selection.itemId could even be migrated away from - thoughts @cjcenizal ?

[cjcenizal]

I think we're definitely on the right track here! I do think that this issue is evidence that selection.itemId unnecessarily couples the concept of item IDs with selection, and we should migrate away from that.

Here's what I suggest:

• Remove selection.itemId
• Add a prop called itemId, which acts as selection.itemId currently does
• If feasible, add the onChange prop as Chandler suggests

The first two will solve Pete's issue, per Chandler's suggestion to supply a UUID to each row on the client-side.

I suggest the last bullet since I think it's a design flaw if EuiInMemoryTable and EuiBasicTable don't adhere to the Liskov substitution principle. I think it's a safe assumption for people to make that if EuiBasicTable supports a prop, then EuiInMemoryTable should too, since the docs present the latter as a more robust/featureful version of the former.

[chandlerprall]

Turns out onChange _is_ currently exposed via search={{onChange: () => {}}} prop on EuiInMemoryTable. It does have a built-in side effect that if it returns a falsey value the in-memory table assumes the data update will be done by the parent.