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
25.03k stars 3.07k forks source link

t function from i18next breaks dnd when given as a dependency to columns' useMemo #5421

Closed sherman89 closed 6 months ago

sherman89 commented 6 months ago

TanStack Table version

8.13.2

Framework/Library version

18.2.0

Describe the bug and the steps to reproduce it

When the t function from const { t } = useTranslation(); is given as a dependency to columns' useMemo (which has to be memoized for dnd to work) drag n drop only works once then stops working.

To reproduce:

  1. Install i18next and react-i18next
  2. Import useTranslation: import { useTranslation } from "react-i18next";
  3. call hook const { t } = useTranslation();
  4. Pass t to columns useMemo
  5. Try to drag n drop more than once

I've added a Code Sandbox that reproduces the problem.

Even if I'm able to find a workaround, it seems like this is a very common scenario (localizing table headers) that should be supported. Seems like dnd+react-table are extremely sensitive to the data and columns props.

Is this a bug or a skill issue? 😅

Thank you very much!

Your Minimal, Reproducible Example - (Sandbox Highly Recommended)

https://codesandbox.io/p/devbox/summer-dream-v5nhwl?file=%2Fsrc%2Fmain.tsx

Screenshots or Videos (Optional)

No response

Do you intend to try to help solve this bug with your own PR?

No, because I do not have time to dig into it

Terms & Code of Conduct

KevinVandy commented 6 months ago

Not able to look at the sandbox right now, but make sure your columns and data have a stable reference. That is critical.

Also, don't call hooks conditionally.

sherman89 commented 6 months ago

@KevinVandy So it seems like the t dependency makes the columns property unstable, because if I fetch the translation outisde useMemo and pass the string to useMemo then it seems to work.

This means I would have to pass all my localized text in that way? Is there no way to make this work without having to worry about destabilizing my columns property?

The sandbox I linked is just an edited version of the react-table dnd example, I only added the t dependency to the columns useMemo.

KevinVandy commented 6 months ago

I just simply pass the t function as a param to the components

sherman89 commented 6 months ago

@KevinVandy Wouldn't that just add another layer that t has to pass through, but resulting in the same problem? Would it be possible that you post an example of what you mean?

We can change the language in real-time so if t is removed from useMemo deps (either directly or indirectly) then we cannot anymore change table header language :(

sherman89 commented 6 months ago

For reference, this is the table component:

import React, {useCallback, useMemo} from 'react';
import {
  useReactTable,
  getCoreRowModel,
  getSortedRowModel,
  type RowData,
  type ColumnDef,
  type Row
} from '@tanstack/react-table';
import classNames from 'classnames';
import {
  DndContext,
  useSensors,
  type DndContextProps,
  useSensor,
  MouseSensor,
  TouchSensor,
  KeyboardSensor,
  type DragEndEvent,
  closestCenter
} from '@dnd-kit/core';
import {SortableContext, verticalListSortingStrategy} from '@dnd-kit/sortable';
import {restrictToParentElement, restrictToVerticalAxis} from '@dnd-kit/modifiers';
import '../../style/components/ReactTable.scss';
import DraggableRow from './DraggableRow';
import ReactTableHeader from './ReactTableHeader';
import ReactTableFooter from './ReactTableFooter';

export type ReactTableProps<T> = {
  data: T[];
  columns: ColumnDef<T>[];
  enableSorting?: boolean;
  enableDragging?: boolean;
  getRowCanExpand?: (row: Row<T>) => boolean;
  renderSubComponent?: (props: {id: string; toggleSubComponent: () => void}) => React.ReactElement;
  isSubComponentUnderRow?: boolean;
  onRowPositionChanged?: (oldIndex: number, newIndex: number) => void;
  getRowId?: (originalRow: T, index: number, parent?: Row<T> | undefined) => string;
};

declare module '@tanstack/react-table' {
  // eslint-disable-next-line unused-imports/no-unused-vars
  interface ColumnMeta<TData extends RowData, TValue> {
    className?: string;
    footerOptions?: {
      isHeader?: boolean;
    };
  }
}

declare module '@tanstack/react-table' {
  // eslint-disable-next-line unused-imports/no-unused-vars
  interface CellContext<TData extends RowData, TValue> {
    dragHandleProps?: Record<string, unknown>;
  }
}

const ReactTable = <T,>({
  data,
  columns,
  enableSorting = false,
  enableDragging = false,
  getRowCanExpand = () => true,
  renderSubComponent,
  isSubComponentUnderRow = true,
  onRowPositionChanged,
  getRowId
}: ReactTableProps<T>) => {
  const table = useReactTable({
    data,
    columns,
    enableSorting,
    getCoreRowModel: getCoreRowModel(),
    getSortedRowModel: getSortedRowModel(),
    getRowCanExpand,
    getRowId
  });

  if (enableDragging && !getRowId) {
    // Since react-table uses indexes by default as row identities, this will not anymore work
    // with dnd since the indexes will change. In this case, we need a unique non-index Id.
    throw new Error('Custom getRowId is required when drag n drop is enabled');
  }

  const headerGroups = table.getHeaderGroups();
  const rowModel = table.getRowModel();
  const footerGroups = table.getFooterGroups();

  const anyFooter = footerGroups
    .flatMap((fg) => fg.headers)
    .some((h) => h.column.columnDef.footer !== undefined);

  // by default returns indexes unless custom getRowId is provided
  const identifiers = rowModel.rows.map((r) => r.id);

  // #region dnd

  const sensors = useSensors(
    useSensor(MouseSensor, {}),
    useSensor(TouchSensor, {}),
    useSensor(KeyboardSensor, {})
  );

  const handleDragEnd = useCallback(
    (event: DragEndEvent) => {
      const {active, over} = event;

      if (!over) {
        return;
      }

      if (active.id !== over.id) {
        const oldIndex = identifiers.indexOf(active.id as string);
        const newIndex = identifiers.indexOf(over.id as string);
        onRowPositionChanged?.(oldIndex, newIndex);
      }
    },
    [identifiers, onRowPositionChanged]
  );

  const dndContextProps = useMemo<DndContextProps>(() => {
    return {
      collisionDetection: closestCenter,
      modifiers: [restrictToVerticalAxis, restrictToParentElement],
      onDragEnd: handleDragEnd,
      sensors
    };
  }, [handleDragEnd, sensors]);

  // #endregion dnd

  return (
    <DndContext {...dndContextProps}>
      <div className="table-container">
        <table
          className={classNames('table table-hover react-table', {'table-sortable': enableSorting})}
        >
          <ReactTableHeader headerGroups={headerGroups} />
          <SortableContext items={identifiers} strategy={verticalListSortingStrategy}>
            {rowModel.rows.map((row) => {
              const skiptBody = renderSubComponent && isSubComponentUnderRow;
              if (!skiptBody) {
                return (
                  <tbody key={row.id}>
                    <DraggableRow
                      row={row}
                      renderSubComponent={renderSubComponent}
                      isSubComponentUnderRow={isSubComponentUnderRow}
                    />
                  </tbody>
                );
              }
              return (
                <DraggableRow
                  key={row.id}
                  row={row}
                  renderSubComponent={renderSubComponent}
                  isSubComponentUnderRow={isSubComponentUnderRow}
                />
              );
            })}
          </SortableContext>
          {anyFooter && <ReactTableFooter footerGroups={footerGroups} />}
        </table>
      </div>
    </DndContext>
  );
};

export default ReactTable;

DraggableRow

import React, {useCallback, useMemo} from 'react';
import type {CSSProperties} from 'react';
import {useSortable} from '@dnd-kit/sortable';
import {CSS} from '@dnd-kit/utilities';
import {flexRender, type Row} from '@tanstack/react-table';
import classNames from 'classnames';

export type DraggableRowProps<T> = {
  row: Row<T>;
  renderSubComponent?: (props: {id: string; toggleSubComponent: () => void}) => React.ReactElement;
  isSubComponentUnderRow?: boolean;
  rowDraggable?: boolean;
  draggingEnabled?: boolean;
  hideSubComponentWhileDragging?: boolean;
};

const DraggableRow = <T,>({
  row,
  renderSubComponent,
  isSubComponentUnderRow = true,
  rowDraggable = false,
  draggingEnabled = true,
  hideSubComponentWhileDragging = true
}: DraggableRowProps<T>) => {
  const rowId = row.id;

  const {attributes, listeners, transform, transition, setNodeRef, isDragging} = useSortable({
    id: rowId,
    disabled: !draggingEnabled
  });

  const toggleSubComponent = useCallback(() => {
    row.toggleExpanded();
  }, [row]);

  const style: CSSProperties = {
    transform: CSS.Translate.toString(transform),
    transition,
    opacity: isDragging ? 0.8 : 1,
    zIndex: isDragging ? 1 : 0,
    position: 'relative'
  };

  const visibleCells = row.getVisibleCells();
  const rowExpanded = row.getIsExpanded();

  const showSubComponentUnderRow = renderSubComponent && isSubComponentUnderRow;
  const showSubComponentInPlaceOfRow =
    !showSubComponentUnderRow && rowExpanded && renderSubComponent;

  const hideSubComponent = hideSubComponentWhileDragging && isDragging && showSubComponentUnderRow;

  // Memoize content since "style" prop will cause many rerenders during drag n drop that should not affect node content
  const RowContent = useMemo(() => {
    if (showSubComponentUnderRow) {
      return (
        <>
          <tr id={rowId}>
            {visibleCells.map((cell) => {
              return (
                <td key={cell.id} id={cell.id}>
                  {flexRender(cell.column.columnDef.cell, cell.getContext())}
                </td>
              );
            })}
          </tr>
          {rowExpanded && (
            <tr id={`${rowId}-subcomponent`} className={classNames({'d-none': hideSubComponent})}>
              <td colSpan={visibleCells.length}>
                {renderSubComponent({id: rowId, toggleSubComponent})}
              </td>
            </tr>
          )}
        </>
      );
    }

    if (showSubComponentInPlaceOfRow) {
      return (
        <td colSpan={visibleCells.length}>{renderSubComponent({id: rowId, toggleSubComponent})}</td>
      );
    }

    return visibleCells.map((cell) => {
      return (
        <td key={cell.id} id={cell.id}>
          {flexRender(cell.column.columnDef.cell, cell.getContext())}
        </td>
      );
    });
  }, [
    showSubComponentUnderRow,
    showSubComponentInPlaceOfRow,
    renderSubComponent,
    visibleCells,
    rowId,
    rowExpanded,
    hideSubComponent,
    toggleSubComponent
  ]);

  // If we want to make the row draggable, place attributes and listeners on the node itself
  const nodeProps = rowDraggable ? {...attributes, ...listeners} : undefined;

  if (showSubComponentUnderRow) {
    return (
      <tbody ref={setNodeRef} style={style} {...nodeProps}>
        {RowContent}
      </tbody>
    );
  }

  if (showSubComponentInPlaceOfRow) {
    return (
      <tr id={`${rowId}-subcomponent`} ref={setNodeRef} style={style} {...nodeProps}>
        {RowContent}
      </tr>
    );
  }

  return (
    <tr id={rowId} ref={setNodeRef} style={style} {...nodeProps}>
      {RowContent}
    </tr>
  );
};

export default DraggableRow;

Did not include header and footer since they're not relevant to the problem.

sherman89 commented 6 months ago

I tried moving columns outside of the component that renders the table, I tried passing t as a prop, I tried wrapping t in a useCallback but obviously none of these work because at the end of the day t changes, which causes all dependencies to update and destabilize the columns.

I'm at my wits end here... what am I doing wrong? 😞

How am I supposed change the headers text (change language) in real-time if I can't "destabilize" the columns? I don't get it

sherman89 commented 6 months ago

For those with a similar problem, the only workaround so far that I found was to do the translation in a component like so:

{
  accessorKey: 'someProp',
  // eslint-disable-next-line react/no-unstable-nested-components
  header: () => <Translate translation={{key: `i18nextKeyGoesHere`}} />,
  cell: (c) => c.getValue()
}

@tannerlinsley Is there any way to eliminate this requirement that columns must have a stable reference? I want to have a better development experience in our team so developers won't scratch their head when dnd stops working due to passing t function to columns memo, which is a very common scenario in our codebase.

Thank you very much!

pranair commented 6 months ago

I have the same issue! Unfortunately @sherman89's fix does not work for me as I'm struggling to figure out how to pass an external state to the table. The only two methods seems to be within the data itself or with meta, but that doesn't seem to work with useState.

Paosder commented 5 months ago

Memoizing header column and pass external state(ex. make wrapper component with recoil) could workaround this issue but seems filter or sort also breaks dnd 😢

sherman89 commented 4 months ago

@Paosder @pranair So I just had another issue with dnd not working after toggling visibility on the drag handle column, and the problem had nothing to do with tanstack table library... I have a feeling that this issue here also is not related to tanstack table.

The issue in question is this one that I also opened: https://github.com/clauderic/dnd-kit/issues/1417

Basically the issue seems to be in SortableContext and forcing it to remount works around these problems.

If you want to check that this is really the case, try adding key={`${Date.now()}`} like this:

<SortableContext
  items={identifiers}
  strategy={verticalListSortingStrategy}
  disabled={!enableDragging}
  key={`${Date.now()}`} // <-- add this
>
  // rows
</SortableContext>

If it works, then you can proceed to implement a proper workaround instead.

Before explaining how I implemented it though, I want to say that you might not need any of this. Maybe it's enough to generate a key based on some other property passed to your table, and try to keep your drag handle component as stable as possible to avoid having to use this workaround as much as possible.

I'm rendering my drag handle component this way:

const renderDragHandle = useCallback((c: CellContext<T, unknown>) => {
  const {isDraggingEnabled} = c.cell.getContext();
  return <DragHandle id={c.row.id} draggingEnabled={isDraggingEnabled} />;
}, []);

You can pass a lot of stuff using the context to avoid adding stuff to the dependency array :) as you can see mine is empty.

And as I mentioned above, make a Translate component to avoid having to pass t as a dependency, further reducing rerenders.

Anyway my workaround for remounting SortableContext is this:

My solution was to expose a function from the table component to the parent, which when called, increments a number which is used as the key for the SortableContext, like this:

// Incrementing this value forces remount of SortableContext.
// See comment for remountSortableContext above.
const [sortableContextKey, setSortableContextKey] = useState(0);

// Expose table ref and other functions to parent. useImperativeHandle works only with forwardRef.
useImperativeHandle(
  ref,
  () => {
    return {
      remountSortableContext() {
        setSortableContextKey(sortableContextKey + 1);
      },
      table
    };
  },
  [sortableContextKey, table]
);

// .....

<SortableContext
  items={identifiers}
  strategy={verticalListSortingStrategy}
  disabled={!enableDragging}
  key={sortableContextKey}
>
 // stuff
</SortableContext>

For useImperativeHandle to work, you must use forwardRef which doesn't support generics, but generics support can be added this way:

export type ReactTableProps<T> = {
  data: T[];
  columns: ColumnDef<T>[];
};

export type TableRef<T> = {
  table: Table<T>;
  remountSortableContext: () => void;
};

const ReactTableInternal = <T,>(
  {
    data,
    columns
  }: ReactTableProps<T>,
  ref: ForwardedRef<TableRef<T>>
) => {
  // useReactTable etc...
};

// ReactTable is exposed this way so that generics would work with forwardRef
const ReactTable = forwardRef(ReactTableInternal) as <T>(
  props: ReactTableProps<T> & {ref?: ForwardedRef<TableRef<T>>}
) => ReturnType<typeof ReactTableInternal>;

export default ReactTable;

Then in parent:

const tableRef = useRef<TableRef<Person>>(null);

// onclick..
tableRef.current.remountSortableContext();

In my case I can now just remount the sortable context when I'm showing the drag handle column:

const tableSelectModeToggledHandler = useCallback((isInSelectMode: boolean) => {
  if (!tableRef.current) {
    return;
  }

  const {table} = tableRef.current;

  const selCol = table.getColumn(BuiltInColumnsIds.rowSelectColumnId);
  const dndCol = table.getColumn(BuiltInColumnsIds.dragHandleColumnId);

  selCol?.toggleVisibility(isInSelectMode);
  dndCol?.toggleVisibility(!isInSelectMode);

  if (!isInSelectMode) {
    table.toggleAllRowsSelected(false);
    tableRef.current.remountSortableContext();
  }

  setIsTableInSelectMode(isInSelectMode);
}, []);