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.24k stars 3.08k forks source link

getCanSort returning false when manualSorting enabled #4136

Open ValentinH opened 2 years ago

ValentinH commented 2 years ago

Describe the bug

I noticed that header.column.getCanSort() returns false if I don't provide an accessorKey or accessorFn, per this condition: https://github.com/TanStack/table/blob/main/packages/table-core/src/features/Sorting.ts#L296-L301

The thing is that when using the manualSorting option, I think getCanSort() could ignore whether accessorKey or accessorFn are present.

What do you think?

Your minimal, reproducible example

N/A

Steps to reproduce

N/A

Expected behavior

When sorting is enabled and manualSorting is true, header.column.getCanSort() should return true.

How often does this bug happen?

No response

Screenshots or Videos

No response

Platform

MacOS

react-table version

8.2.1

TypeScript version

No response

Additional context

No response

Terms & Code of Conduct

mtrenker commented 2 years ago

Hey @ValentinH :) Interesting question, would you still use the column.getToggleSortingHandler? I feel like getCanSort tells you more about the tables ability to sort, not "can the user do it", if this makes sense. Or in other words, if you opt-out of sorting, do everything yourself.

This is not a strong opinion, just a thought

tannerlinsley commented 2 years ago

This is a tough one. I'd like to see some scenarios where you want a server-side sortable column that doesn't have an accessor. Even for "display" columns or columns that show computed values like row.original.firstName + row.original.lastName, all it takes is accessor: '_' or accessor: () => {}.

ValentinH commented 2 years ago

This might probably be a bad pattern but we are actually never using the accessorKey/accessorFn properties but rely on the cell one where we only use the original property of the row when our tables logic is fully server-driven (pagination, filtering or sorting).

accessorKey: '_' generates a TS error because _ isn't part of our data while doing accessorFn: () => ({}) does work. However, I'd like to be able to omit them because I'm scared that someone will think they are useless in the future, remove them and break the sorting logic without noticing.

tannerlinsley commented 2 years ago

Hmm. Not using accessor keys will likely cause other weird behavior. I’ll keep thinking about this.

ValentinH commented 2 years ago

I actually noticed that not using a column containing a primitive value for accessorKey was breaking the sorting logic even if manualSorting is true. I'm actually wondering how manualSorting is expecting to be used. Is it possible to have the sorting UI being handled by tanstack/table and only have the sorting logic backend-side. What we are trying to do is to have the sort parameters coming from the URL so not using the default sorting state. Is there an example of something like this? If not, I could try to setup a minimal example of what we are trying to do.

tannerlinsley commented 2 years ago

Manual sorting essentially skips the sorting model stage and takes the rows in the order that they are defined.

tannerlinsley commented 2 years ago

So where did we end up on this? I'm using manualSorting in my own stuff just fine and it's behaving as expected along with the can sort logic. Did everyone here find a solution to their issues?

ValentinH commented 2 years ago

The root point of this issue is related to being forced to provide accessorKey or accessorFn. I understand that we are probably using this library in a little weird way. Indeed, we never actually use them: in v7, we were defining them just to have the TS types behaving properly. On that side, this is not required anymore but some features, UI-related, don't work in v8 if we don't provide them.

Regarding my use case with manualSorting and using the URL as the sorting state, I managed to make it work by using a sorting state and a onSortingChange that are based on the URL with this hook (if it can be useful to someone):

type UseSortingStateFromUrlProps = {
  defaultSort: SortingState;
};

export const useSortingStateFromUrl = ({ defaultSort }: UseSortingStateFromUrlProps) => {
  const router = useRouter();
  const sort = extractSort(router.query);
  const defaultSortRef = useRef(defaultSort);

  useEffect(() => {
    if (!sort) {
      const newSort = defaultSortRef.current[0];
      router.replace({
        query: {
          sort_direction: newSort.desc ? 'desc' : 'asc',
          order_by: newSort.id,
        }
      });
    }
  }, [sort, router]);

  const sorting: SortingState = sort
    ? [
        {
          id: sort?.by,
          desc: sort?.direction === 'desc',
        },
      ]
    : defaultSort;

  const onSortingChange: OnChangeFn<SortingState> = (updaterOrValue) => {
    let newSort: SortingState;
    if (typeof updaterOrValue === 'function') {
      newSort = updaterOrValue(sorting);
    } else {
      newSort = updaterOrValue;
    }
    router.push({
      query: {
        sort_direction: newSort[0].desc ? 'desc' : 'asc',
        order_by: newSort[0].id,
      }
    });
  };

  return { sorting, onSortingChange };
};

Then, we use it like this:

  const { sorting, onSortingChange } = useSortingStateFromUrl({
    defaultSort: [
      {
        id: 'firstName'
        desc: false
      },
    ]
  });

  const table = useReactTable({
    columns,
    data,,
    getCoreRowModel: getCoreRowModel(),
    enableSortingRemoval: false,
    manualSorting: true,
    state: {
      sorting,
    },
    onSortingChange,
  });
carlosbaraza commented 2 years ago

We have a custom wrapper used for a table that will generate the correct filter and sort query to fetch data from our backend. I stumbled upon this limitation because we use cell only too. The accessor is not sufficient for our use case because some cells are more than just a string. We render complex components in each cell, for example to handle a state machine for the row, a tags component or relationships to other data rows.

Our wrapper column type definition:

export type CmsTableColumn<T extends CmsTableDataBase, W, O> = {
  columnDef: Omit<ColumnDef<T>, 'enableColumnFilter' | 'enableSorting'> & {
    id: string;
    cell: ColumnDef<T>['cell'];
    columns?: never; // disallow nested columns
  };
  /**
   * If provided, the column UI will show the filter. It will only be called if there is a filter.
   * The returned value should be the graphql query to filter the data in the backend.
   */
  getFilterQuery?: (
    filter: string,
    cmsTable: CmsTable<T, W, O>,
    columnId: string
  ) => W;
  /**
   * If provided, the column UI will allow sorting clicking in the column name. It will only be called
   * if the column is sorted. The returned value should be the graphql query to sort the
   * data in the backend, which will be added to all other sorting queries. The first defined
   * column goes first in the sorting.
   */
  getSortQuery?: (
    sortDesc: boolean,
    cmsTable: CmsTable<T, W, O>,
    columnId: string
  ) => O;
};
mattleonowicz commented 1 year ago

Sorting is somewhat inconsistent in this lib. I was hoping for sorting on server side, so I turned manualSorting: true and hoped that the lib will help me with just the UI representation on the column headers (to show what is it sorted by), but leave the data that is pass unprocessed. But it seems like once you enable manualSorting, you cannot count on the library to hold the sorting state anymore. state.sort is accepted, even onSortingChange is called, but the data just does not rerender when you do that. So it is impossible to rely on calling headerContext.column.getIsSorted() on those entries that you get from table.getHeaderGroups(). This is always stale data


EDIT: ok, it actually seems that it's because I was delegating the rendering to children components which are memoized, and because table.getHeaderGroups() always points to the same reference, they were not rerendering.

tigfamon commented 1 year ago

This is a tough one. I'd like to see some scenarios where you want a server-side sortable column that doesn't have an accessor. Even for "display" columns or columns that show computed values like row.original.firstName + row.original.lastName, all it takes is accessor: '_' or accessor: () => {}.

@tannerlinsley , we have a simillar scenario. Like that:

columnHelper.display({
  header: 'Invoice number',
  id: InvoiceOrder.DETAILS,
  enableSorting: true,
  cell: ({ row }) => {
    const { prefix, number } = row.original;
    return (
      <Typography.Text>{`${prefix}-${number}`}</Typography.Text>
    );
  },
}),

The column value includes the prefix and the number values from a server. At the same time, we would like to make a server-side sortable column. But the header.column.getCanSort() function always returns false and the column is rendered as non-sortable.

What is the best way to implement server-side sorting at this case 🤔 ?

royalknight56 commented 7 months ago

same

This is a tough one. I'd like to see some scenarios where you want a server-side sortable column that doesn't have an accessor. Even for "display" columns or columns that show computed values like row.original.firstName + row.original.lastName, all it takes is accessor: '_' or accessor: () => {}.

@tannerlinsley , we have a simillar scenario. Like that:

columnHelper.display({
  header: 'Invoice number',
  id: InvoiceOrder.DETAILS,
  enableSorting: true,
  cell: ({ row }) => {
    const { prefix, number } = row.original;
    return (
      <Typography.Text>{`${prefix}-${number}`}</Typography.Text>
    );
  },
}),

The column value includes the prefix and the number values from a server. At the same time, we would like to make a server-side sortable column. But the header.column.getCanSort() function always returns false and the column is rendered as non-sortable.

What is the best way to implement server-side sorting at this case 🤔 ?

same