TanStack / table

🤖 Headless UI for building powerful tables & datagrids for TS/JS - React-Table, Vue-Table, Solid-Table, Svelte-Table
https://tanstack.com/table
MIT License
24.61k stars 3.04k forks source link

Improve typing for RowSelectionState #5479

Open lucarge opened 3 months ago

lucarge commented 3 months ago

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.

KevinVandy commented 3 months ago

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

lucarge commented 3 months ago

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.