iTwin / iTwinUI-react

A react component library for iTwinUI.
https://github.com/iTwin/iTwinUI
Other
83 stars 23 forks source link

Table: row is not getting re-rendered correctly #104

Closed Juliakas closed 3 years ago

Juliakas commented 3 years ago

EDIT: Included another scenario, that better reflects our case (Scenario 4).

Environment

Code Sandbox

Please read additional information below. Scenario 1 - memoized table data Scenario 2 - correct behavior Scenario 3 - table data passed as props Scenario 4 - table data destructured from state

Steps to reproduce

  1. Go to code scenario 1 or 3.
  2. Click any button on the last table row.

Actual behavior

Button stays the same color upon clicking it. Captured state in columns is not correct, is always false which results in button always being blue.

Expected behavior

Button clicks toggle color between green and blue.

Additional information

Some interesting behavior was observed in different scenarios. In scenario 1 incorrect state is captured by cell that renders buttons, in this case data is memoized and is never updated, columns is memoized and updated on state changes.

In scenario 2 I removed useMemo for table data creation - this results in correct row state and everything behaves as expected - only problem is that data is not being memoized.

In scenario 3 everything is the same, only this time I decided to pass down tableData as props, no useMemo is involved - the only difference is that table data is now provided by parent component. For some reason table rows are not updating again - I am experiencing same issues as in scenario 1.

For scenario 4, refer to this comment.

mayank99 commented 3 years ago

This is interesting.

If you think about it, it makes sense that if you memoize the table data, the rows will not re-render until the data changes. Same thing with scenario 3 (props don't change -> no re-render).

I was able to "fix" scenario 1 by adding state to the useMemo call. image

I'm guessing your actual use case is more complex than just a single state variable, which makes me think that we need to support useRowState. @bentleyvk

Juliakas commented 3 years ago

@mayank99 We have a case where we use sortItems(data) function every time while passing it to Table component. And this function actually created a new array to avoid mutation - I'm still trying to figure why the rows are not updating. Also just to note, we only started to experience this issue when updating to 1.16 (on 1.12 everything behaved as expected).

Juliakas commented 3 years ago

@mayank99 @bentleyvk After investigating this further, I managed to reproduce this issue that is very close to our case. So the main difference among other scenarios is that I use table data as a state. And that means that table will ignore updates even if I use variables that derive from that state, for example: const data = [...state] or even const data = [...state.sort(<...>)]. However, a workaround like this seems to work: const data = state.map((x) => ({ ...x })).

I have no explanation for this... maybe its because react-table retains the same instance of original (that is used later by React.memo for TableRowMemoized) by doing a table data deep comparison?

bentleyvk commented 3 years ago

@mayank99 @bentleyvk After investigating this further, I managed to reproduce this issue that is very close to our case. So the main difference among other scenarios is that I use table data as a state. And that means that table will ignore updates even if I use variables that derive from that state, for example: const data = [...state] or even const data = [...state.sort(<...>)]. However, a workaround like this seems to work: const data = state.map((x) => ({ ...x })).

I have no explanation for this... maybe its because react-table retains the same instance of original (that is used later by React.memo for TableRowMemoized) by doing a table data deep comparison?

Table row checks data entry so when you sort it, data reference changes but entries references remain the same and therefore rows are not rerendered. In your codesandblox example data.map((x) => ({ ...x })) creates new array of data and entries references are new too. If you want to make a change to data entry, you need to create its new reference inside data array:

const newData= [...data];
newData[index] = {...newData[index], ...otherChanges};
return newData;

But I think it is fixed with this change: https://github.com/iTwin/iTwinUI-react/pull/114