Improve typing for RowSelectionState

[lucarge]

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:

table/packages/table-core/src/features/RowSelection.ts

Lines 433 to 441 in c40d734

	table.getIsSomeRowsSelected = () => {
	const totalSelected = Object.keys(
	table.getState().rowSelection ?? {}
	).length
	return (
	totalSelected > 0 &&
	totalSelected < table.getFilteredRowModel().flatRows.length
	)
	}

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]

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

[lucarge]

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:

table/packages/table-core/src/features/RowSelection.ts

Lines 547 to 579 in c40d734

	const mutateRowIsSelected = <TData extends RowData>(
	selectedRowIds: Record<string, boolean>,
	id: string,
	value: boolean,
	includeChildren: boolean,
	table: Table<TData>
	) => {
	const row = table.getRow(id, true)
	// const isGrouped = row.getIsGrouped()
	// if ( // TODO: enforce grouping row selection rules
	// !isGrouped ||
	// (isGrouped && table.options.enableGroupingRowSelection)
	// ) {
	if (value) {
	if (!row.getCanMultiSelect()) {
	Object.keys(selectedRowIds).forEach(key => delete selectedRowIds[key])
	}
	if (row.getCanSelect()) {
	selectedRowIds[id] = true
	}
	} else {
	delete selectedRowIds[id]
	}
	// }
	if (includeChildren && row.subRows?.length && row.getCanSelectSubRows()) {
	row.subRows.forEach(row =>
	mutateRowIsSelected(selectedRowIds, row.id, value, includeChildren, table)
	)
	}
	}

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.