React-table: Reducer in useRowSelect relies on external state

[spectras]

Pulled out of #2120, because this should be fixed even if 2120 is solved in another way or ends up being unrelated.

Describe the bug

The handler of toggleRowSelected action type reads isSelected from row objects:

• useRowSelect.js:126

As a result, when multiple selection changes happen within the same reducer run, toggleRowSelected uses the initial row selection state, regardless of whether another action updated it earlier in the same run.

Provide an example via Codesandbox!

Sorry I did not make one. The idea is just to do this:

assert(someRow.isSelected); // the bug shows only if row is selected

toggleAllRowsSelected(false) ;
someRow.toggleRowSelected(true);

What happens then is this:

• The reducer handles toggleAllRowsSelected.
  
  
  
  • It clears selectedRowIds.
  
  
  • At this point, the isSelected property of rows has not been updated.
  
  
• The reducer handles toggleRowSelected.
  
  
  
  • It sees it should select the row.
  
  
  • It looks at row.isSelected. It is true, because react did not yet recompute it.
  
  
  • It skips the update.
  
  
  • Therefore the row is not added back to selectedRowIds.
  
  
• At last, react recomputes selectedFlatRows, which updates isSelected as a side effect (see #2170).

It is hard to make a minimal example, because this triggers observable bugs depending on when exactly React chooses to recompute selectedFlatRows, which is further obfuscated by #2170.

Expected behavior

Reducers should never access any information outside of previousState. Otherwise they no longer are predictable.

Replacing row.isSelected by id in state.selectedRowIds or something similar should work I believe.

[tannerlinsley]

Yeah, I think you're right. This particular situation is further mitigated in the next branch, too where we have a new state management paradigm that gets around the stale state issue with reducers. There's a lot of places in React Table right now that could suffer from this same problem, so it seemed fitting that we move to something a bit safer.

[fparga]

What's the status on this issue? Is there a workaround?

[tannerlinsley]

@fparga no fix yet. I haven't seen an overwhelming amount of noise on this issue, so it doesn't seem to affect many right now. I currently have my sights set on v8 where this class of issue won't exist at all (by nature of some new architecture decisions). I would be happy to accept a PR to fix this issue in v7 and even offer any guidance on it.

[spectras]

@fparga a workaround is to

• avoid useRowSelect, or
• use it in controlled state mode (so, avoiding selection state update logic), or
• use selectedRowIds directly, ignoring helper methods and isSelected attribute.
  It is an annoyance, but at least you can use all other parts of react-table normally.

Looking forward to v8 though :)

[codecadwallader]

+1 to add some noise. I spent a couple hours integrating/debugging inconsistency bugs before finding this issue and tossing out the code to roll our own implementation. The design/spec looks great so I'm hopeful it's more stable in v8 🤞