table icon indicating copy to clipboard operation
table copied to clipboard

unnecessary rerenders of every row and cell in examples/react/editable-data

Open jasekiw opened this issue 1 year ago • 13 comments

Describe the bug

Every time one cell is edited in the examples/react/editable-data example, the entire table rerenders. If a user wants to render a large amount of rows, this strategy will start getting slow. At 50 rows in the example it takes 14.4 milliseconds to rerender the whole table with basic data. I've narrowed the unstable reference down to cell.getContext().

Your minimal, reproducible example

https://github.com/TanStack/table/tree/main/examples/react/editable-data

Steps to reproduce

  1. Open react dev tools profiler
  2. Start profiling
  3. Click on a cell to edit it
  4. Click off the cell
  5. Stop the profiler
  6. Check the results

Expected behavior

Only the affected cell should rerender unless my other cells have a direct dependency on it. I've even tried creating memoized components all the way down to the cell.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

Windows

react-table version

8.8.5

TypeScript version

No response

Additional context

If this is something the library doesn't support then I think there should be a documentation section on it describing the pitfalls of rendering a large amount of rows and/or using complex cells.

Terms & Code of Conduct

  • [X] I agree to follow this project's Code of Conduct
  • [X] I understand that if my bug cannot be reliable reproduced in a debuggable environment, it will probably not be fixed and this issue may even be closed.

jasekiw avatar Apr 03 '23 15:04 jasekiw

hi @jasekiw, there are a few workarounds for this issue

the first reason all the cells re-render is that the state of the parent component,App, is changed - we can React.memo the cell to try and solve that.

that won't be enough because the cell context also changes, some things rightfully so, like the table and the row, because their data has changed. and some things change without a seemingly good reason, like the getValue function.

so we stop using flexRender and implement our own function to extract what we need from the cellContext and pass to the cell component.

here is a sandbox with this solution.

the other option is more complicated, the basic idea is to pass only ids to the table and let each cell/row fetch and update its own data. it will require some external store for the data and other changes in the table implementation.

I personally prefer the second option

if you want to I can prepare a sandbox for it

alissaVrk avatar Apr 05 '23 14:04 alissaVrk

It seems it's the same issue from v7 (#4449).

hstevanoski avatar Apr 06 '23 09:04 hstevanoski

@alissaVrk That first sandbox is an improvement turning 136ms render time down to 85ms with a table of 1000 items showing. Because the td and each row is still being rendered however I believe there's still a lot more that can be optimized. Memoizing each row would be great but that might be unfeasible with the current design. Like you mentioned, only passing the ids to the table might be the best route for performance.

jasekiw avatar Apr 06 '23 19:04 jasekiw

@jasekiw Unfortunately it seems you're right, I mean memoizing only the cells will have a significant impact only if the cells are heavy enough. But to memoize rows/cells properly the row should not change if nothing in it changed, same for the cell and the cellContext ( plus cellContext should not reference the table).

On a different note maybe adding an example for a table with only ids with an explanation would be nice

alissaVrk avatar Apr 07 '23 06:04 alissaVrk

@alissaVrk Can you add example with ids?

Tsyklop avatar May 24 '23 10:05 Tsyklop

@Tsyklop here is a sandbox with a simple example. you will need some store to implement it.

there are other options for how to implement it, for example, you could pass a useItem(string id) => Person to the table and get the item per row instead of the cell

alissaVrk avatar May 27 '23 06:05 alissaVrk

I just noticed the same behaviour in my table, when I start editing it. It really is a bummer. And I do not think, that @alissaVrk ´s solution should be the go-to way. It works, but it is something that should be fixed by tanstack/table imo... :/

dennemark avatar Jun 05 '23 16:06 dennemark

@dennemark I agree, though since the whole data is passed to the table, the table will have to be re-rendered. and to avoid re-rendering the rows some memoization will have to be involved

alissaVrk avatar Jun 06 '23 04:06 alissaVrk

https://github.com/TanStack/table/assets/30996641/a5952f15-1ff9-4b51-bbca-63800b4aa21f

https://github.com/TanStack/table/assets/30996641/33c6521b-286c-4786-9d91-c505b36c0e81

muhaimincs avatar Jul 14 '23 01:07 muhaimincs

any idea why it keeps rerendering?

muhaimincs avatar Jul 14 '23 01:07 muhaimincs

I found this piece of code that make my table keep rendering

{header.column.getCanResize() ? (
     <div
        {...{
             onMouseDown: header.getResizeHandler(),
             onTouchStart: header.getResizeHandler(),
             className: `resizer ${
                   header.column.getIsResizing() ? 'isResizing' : ''
             }`,
         }}
      />
  ) : null}

muhaimincs avatar Jul 14 '23 04:07 muhaimincs

I'm seeing my memoized flexRendered component not only rerendering on interactions, but actually unmounting without arg changes. Is this likely related to this issue?

chriskuech avatar Nov 14 '23 06:11 chriskuech

I was able to get no unnecessary rerenders by extracting only the properties I need out of each row/cell and then creating a memoized Row and Cell Components:


function arrayContentsEqual<T>(arr1: T[], arr2: T[]) {
  if(arr1.length !== arr2.length) return false;
  for(let i = 0; i < arr1.length; i++) {
    if(arr1[i] !== arr2[i]) return false;
  }
  return true;
}

function objectContentsEqual<T extends Record<string, any>>(obj1: T, obj2: T) {
  const obj1Keys = Object.keys(obj1);
  const obj2Keys = Object.keys(obj2);
  if(obj1Keys.length !== obj2Keys.length) return false;
  if(!arrayContentsEqual(obj1Keys, obj2Keys)) return false;
  for (let obj1Key of obj1Keys) {
    if(obj1[obj1Key] !== obj2[obj1Key]) {
      console.log(obj1Key, obj1[obj1Key], obj2[obj1Key], 'differ')
      return false;
    }
  }
  return true;
}

type MemoizedRow<TData extends RowData> = {
  id: string;
  cells: MemoizeCell<TData>[];
}

type MemoizeCell<TData extends RowData> = {
  id: string;
  rowIndex: number
  columnId: string;
  value: TData
  updateData: (rowIndex: number, columnId: string, value: any) => void;
}


function useMemoizedRows<TData extends RowData>(rows: Row<TData>[]) {
  const startTime = Date.now();
  const rowsRef = React.useRef<MemoizedRow<TData>[]|null>(null);
  let newRows = React.useMemo(() => {
    return rows.map<MemoizedRow<TData>>((row, rowIndex) => {
      const cells = row.getVisibleCells();
      return {
        id: row.id,
        cells: cells.map((cell, cellIndex) => {
          const cellContext = cell.getContext();
          return {
            id: cell.id,
            rowIndex: cellContext.row.index,
            columnId: cellContext.column.id,
            value: cellContext.getValue(),
            updateData: cellContext.table.options.meta?.updateData!,
          } as MemoizeCell<TData>;
        })
      }
    })
  }, [rows]);

  if(!rowsRef.current)
    rowsRef.current = newRows;

  newRows = newRows.map((newRow, rowIndex) => {
    const memoizedRow = rowsRef.current![rowIndex]!;
    let anyCellChanged = false;
    const resultingRow = {
      id: newRow.id,
      cells: newRow.cells.map((newCell, cellIndex) => {
        const memoizedCell = memoizedRow?.cells[cellIndex];
        if(memoizedCell && objectContentsEqual(newCell, memoizedCell)) return memoizedCell;
        // console.log('rowIndex', rowIndex, 'cellIndex', newCell, 'changed')
        anyCellChanged = true;
        return newCell;
      })
    } as MemoizedRow<TData>;
    if(!anyCellChanged && newRow.id === memoizedRow?.id)
      return memoizedRow;
     // console.log('rowIndex', rowIndex, 'changed')
    return resultingRow;
  });
  const endTime = Date.now();
  console.info('memoization took: ', endTime - startTime, 'ms');
  if(arrayContentsEqual(newRows, rowsRef.current!)) {
    rowsRef.current = newRows;
    return rowsRef.current!;
  }
  rowsRef.current = newRows;
  return rowsRef.current!;
}
  <tbody>
          {memoizedRows.map(row => {
            return (
              <RowComponent key={row.id} row={row} />
            )
          })}
        </tbody>
function RowInner({row}: {row: MemoizedRow<Person>}) {
  return (
    <tr>
      {row.cells.map(cell => {
        return (
          <CellComponent
            key={cell.id}
            rowIndex={cell.rowIndex}
            columnId={cell.columnId}
            value={cell.value}
            updateData={cell.updateData}
          />);
      })}
    </tr>
  )
}

const RowComponent = React.memo(RowInner);
function CellInner({
  value,
  rowIndex,
  columnId,
  updateData,
}: MemoizeCell<any>)  {
  return (
     <td>
      <input
        value={value as string}
        onChange={(e) => updateData?.(rowIndex, columnId, e.target.value)}
      />
     </td>
  );
}
const CellComponent = React.memo(CellInner);

This cuts down on all re-renders. I found however that getRowModel seems to always take around 30ms regardless how many items are rendered on 10,000 items

jasekiw avatar Nov 21 '23 18:11 jasekiw