table icon indicating copy to clipboard operation
table copied to clipboard

Improve typing for RowSelectionState

Open lucarge opened this issue 1 year ago • 2 comments

Hi everyone. Today I faced a bug while using this project that can be easily avoided by this type change.

Change explanation

As the RowSelectionState is currently typed, if I had a collection of 3 items and I wanted to initially pre-select the second one I'd construct the state as follows:

const rowSelectionState: RowSelectionState = {
  "0": false,
  "1": true,
  "2": false,
}

The problem with the code above is that it would break the table, because the code expects only the selected rows to be in the state. One example of this can be seen in this piece of code: https://github.com/TanStack/table/blob/c40d734a20c6af7020a50600f1c1b198bf196744/packages/table-core/src/features/RowSelection.ts#L433-L441

By constructing the state like I did above, the condition totalSelected < table.getFilteredRowModel().flatRows.length would always be false, although with the state above it should clearly be true.

With this little type change, we can force the user to remove false entries from the state, communicating better the intended behavior of this piece of code.

lucarge avatar Apr 10 '24 14:04 lucarge

I think this exposes a bug in that code more than us needing to narrow the types more.

KevinVandy avatar Apr 10 '24 22:04 KevinVandy

For sure there's a discrepancy between how the RowSelectionState is typed and how it's being used into the library @KevinVandy; I'm not familiar with the code, but while debugging I haven't found a single place in the library that reads the RowSelectionState's values.

Here's another example from the library: https://github.com/TanStack/table/blob/c40d734a20c6af7020a50600f1c1b198bf196744/packages/table-core/src/features/RowSelection.ts#L547-L579

This is the code that runs for toggleSelected, for example, but instead of toggling the value it deletes the entry to unselect the row.

I think that for the next major version the state could be changed to be a list of indexes to avoid any confusion, but for now to avoid breaking changes I think this type change is the best that can be done.

lucarge avatar Apr 11 '24 10:04 lucarge