Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for reactivity in Vue-adapter #5687

Merged
merged 5 commits into from
Aug 3, 2024

Conversation

OlaAlsaker
Copy link
Contributor

This PR updates the useVueTable composable in the TanStack Table Vue adapter to support reactive data. Now, when you pass a reactive ref to the data option, the table will automatically update when the data changes.

Changes

  • Extended the types to handle reactive data.
  • Modified useVueTable to check if the data option is a ref and handle it correctly.
  • Updated documentation with examples showing how to use the new feature.

Example usage

const dataRef = ref([
  { id: 1, name: 'John' },
  { id: 2, name: 'Jane' }
])

const columns = [
  { accessor: 'id', Header: 'ID' },
  { accessor: 'name', Header: 'Name' }
]

const table = useVueTable({
  columns,
  data: dataRef, // Pass the reactive data ref
})

// Update dataRef later to automatically update the table
dataRef.value = [
  { id: 1, name: 'John' },
  { id: 2, name: 'Jane' },
  { id: 3, name: 'Doe' }
]

Related issues or discussions

This will resolve discussion #5102

@jariz
Copy link

jariz commented Aug 2, 2024

Why not add ref support for all the other options as well? as of writing I'm really struggling with making it respect changes to the pageCount property as well.
It just will not use it's updated value, only after a hot reload it will actually update to the correct value. (clearly indicating it not correctly picking up changes to the options)
Adding a getter doesn't even do anything!
I think this library is great in react and that's my own background mostly but having to use this in vue now is absolutely infuriating.

(sorry for the small side tangent rant)

@KevinVandy
Copy link
Member

We've been looking into improvements for both Vue and Solid reactivity. Would want to confirm that this brings no breaking changes for v8.

Alternatively, we could have breaking changes if larger parts of the reactivity needed to be rethought for v9 in the alpha branch.

@OlaAlsaker
Copy link
Contributor Author

OlaAlsaker commented Aug 3, 2024

Based on my simple (and limited) testing, this should not be a breaking change. The only thing it does is allow and add support for reactivity, using Vue's MaybeRef. But it is not required, and the "old" ways should work as normal.

I have just started using this library, so I created this PR to fix some bugs I experienced when using JS getters (as per the documentation) together with Tanstack Query. Adding support for reactivity fixed my issue, so I made a PR for that 😄

I do agree that the library should have reactivity on the other options as well, and I might be able to fix that in another PR another time in the future 🚀

Maybe we should create an issue with a list of improvements that should be made to the Vue-adapter? I would be happy to have a look at it!

Copy link

nx-cloud bot commented Aug 3, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0baf66c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Aug 3, 2024

commit: 0baf66c

pnpm add https://pkg.pr.new/@tanstack/angular-table@5687
pnpm add https://pkg.pr.new/@tanstack/lit-table@5687
pnpm add https://pkg.pr.new/@tanstack/match-sorter-utils@5687
pnpm add https://pkg.pr.new/@tanstack/qwik-table@5687
pnpm add https://pkg.pr.new/@tanstack/react-table@5687
pnpm add https://pkg.pr.new/@tanstack/react-table-devtools@5687
pnpm add https://pkg.pr.new/@tanstack/solid-table@5687
pnpm add https://pkg.pr.new/@tanstack/svelte-table@5687
pnpm add https://pkg.pr.new/@tanstack/table-core@5687
pnpm add https://pkg.pr.new/@tanstack/vue-table@5687

Open in Stackblitz

More templates

@KevinVandy
Copy link
Member

@OlaAlsaker @jariz I think we should look at making all possible table options that need to be reactive reactive, but possibly save it for v9. A pr to alpha could be made now for that though. The alpha branch is in heavy development right now.

Can ship the change for reactive data now.

@KevinVandy KevinVandy merged commit ab8e318 into TanStack:main Aug 3, 2024
5 checks passed
@9mm
Copy link

9mm commented Aug 8, 2024

I don't know if it's this PR, or #5694 this one, but this has made vue-datatable beyond unusable. My table crashes 100% of the time in chrome with 100+ GB of memory. I have ~20,000-50,000 rows which is handled with absolute zero effort normally.

IMO the benefit of tanstack table is this feature... it's the same as if i use a shallowRef when working with massive data objects. If i have nearly 100K rows I definitely dont want that array to be fully reactive. At the very least maybe we can have a refresh function similar to shallowRef, instead of making it fully reactive?

So yeah - I dont know if this is technically "breaking" but i had to revert this, and it definitely broke my current use case. I havent gotten a chance to look at how to actually turn this feature off or read the code/changes, but just wanted to make a comment now while im here reverting it. I'll look more in detail later. This is the current way I use it:

    const table = useVueTable({
      get data() {
        return data.value;
      },
      get columns() {
        return columns.value;
      },
      state: {
        get expanded() {
          return expanded.value;
        },
        get sorting() {
          return sorting.value;
        },
        get rowSelection() {
          return rowSelection.value;
        },
      },
      initialState: {
        pagination: {
          pageIndex: 0,
          pageSize: 250,
        },
      },
      enableRowSelection: true,
      getCoreRowModel: getCoreRowModel(),
      getExpandedRowModel: getExpandedRowModel(),
      getPaginationRowModel: getPaginationRowModel(),
      getRowId: row => row[rowKey.value],
      getSortedRowModel: getSortedRowModel(),
      getSubRows: row => row.sub_rows,
      onExpandedChange,
      onSortingChange,
      onRowSelectionChange,
      debugTable: false,
    });

@OlaAlsaker
Copy link
Contributor Author

OlaAlsaker commented Aug 9, 2024

I don't know if it's this PR, or #5694 this one, but this has made vue-datatable beyond unusable. My table crashes 100% of the time in chrome with 100+ GB of memory. I have ~20,000-50,000 rows which is handled with absolute zero effort normally.

IMO the benefit of tanstack table is this feature... it's the same as if i use a shallowRef when working with massive data objects. If i have nearly 100K rows I definitely dont want that array to be fully reactive. At the very least maybe we can have a refresh function similar to shallowRef, instead of making it fully reactive?

So yeah - I dont know if this is technically "breaking" but i had to revert this, and it definitely broke my current use case. I havent gotten a chance to look at how to actually turn this feature off or read the code/changes, but just wanted to make a comment now while im here reverting it. I'll look more in detail later. This is the current way I use it:

    const table = useVueTable({

      get data() {

        return data.value;

      },

      get columns() {

        return columns.value;

      },

      state: {

        get expanded() {

          return expanded.value;

        },

        get sorting() {

          return sorting.value;

        },

        get rowSelection() {

          return rowSelection.value;

        },

      },

      initialState: {

        pagination: {

          pageIndex: 0,

          pageSize: 250,

        },

      },

      enableRowSelection: true,

      getCoreRowModel: getCoreRowModel(),

      getExpandedRowModel: getExpandedRowModel(),

      getPaginationRowModel: getPaginationRowModel(),

      getRowId: row => row[rowKey.value],

      getSortedRowModel: getSortedRowModel(),

      getSubRows: row => row.sub_rows,

      onExpandedChange,

      onSortingChange,

      onRowSelectionChange,

      debugTable: false,

    });

Thanks for the report! Can you check if the latest version (#5698) fixes the problem?

If not, I think I will just revert the reactivity-changess all together and instead push it to the v9 branch. Or, at least put it behind a _experimentalReactivity-flag.

This was of course not meant to be a breaking change. Sorry for the inconvenience 😇

(cc @KevinVandy )

@romansp
Copy link

romansp commented Aug 10, 2024

@OlaAlsaker first thanks for looking into adding reactivity support for Vue. It would be amazing to eventually get to a point where all table properties can be reactive (e.g. expanded, selection, etc.).

I quickly tried the latest version with reactive data support and something is still feels off about reactivity. Here's my stackblitz: https://stackblitz.com/edit/tanstack-table-dv5fcj?file=src%2FApp.vue&preset=node

  1. Table doesn't update after new element is pushed to ref.
  2. Table doesn't update after new element was added with immutability.

But strangely table will update if data is coming from computed derived from original ref.

Although in my actual project data flows through computed, Table still doesn't seem to re-render even though computed value is changing. We use vue-query so I will try to create a minimal repro with vue-query as well.

@OlaAlsaker
Copy link
Contributor Author

OlaAlsaker commented Aug 10, 2024

@OlaAlsaker first thanks for looking into adding reactivity support for Vue. It would be amazing to eventually get to a point where all table properties can be reactive (e.g. expanded, selection, etc.).

I quickly tried the latest version with reactive data support and something is still feels off about reactivity. Here's my stackblitz: https://stackblitz.com/edit/tanstack-table-dv5fcj?file=src%2FApp.vue&preset=node

  1. Table doesn't update after new element is pushed to ref.
  2. Table doesn't update after new element was added with immutability.

But strangely table will update if data is coming from computed derived from original ref.

Although in my actual project data flows through computed, Table still doesn't seem to re-render even though computed value is changing. We use vue-query so I will try to create a minimal repro with vue-query as well.

Thanks for the report! Took a look at your Stackblitz, and it looks like you have small error in your example:

  <button class="border p-2" @click="addRow">Add row</button>
  <button class="border p-2" @click="addRow">Add row immutable</button>

Both of your test-buttons triggers the same function on click. Also your function addRowImmutable is trying to reference data.value, which does not exist.

Test out your addRowImmutable-function (and fix the bug), and you will see that is works like expected 😄

We use shallowRef under the hood for performance reasons. This means that the data is not deeply reactive, only the .value is.

const data = ref([{ id: 1, name: 'John' }]);

// This will NOT work ❌
data.value.push({ id: 2, name: 'Doe' });

// This will work ✅
data.value = [...data.value, { id: 2, name: 'Doe' }];

As for the use together with Tanstack Query (vue-query), I find myself having to use computed most of the time anyway, since Tanstack Table doesn't allow null or undefined data.

const { data } = useQuery(...);

const table = useVueTable({
  data: computed(() => data.value ?? []),
  columns,
  getCoreRowModel: getCoreRowModel(),
});

@9mm
Copy link

9mm commented Aug 10, 2024

Hey @OlaAlsaker cool deal! I will have to try that later as I just came down with covid so now im lying in bed. Hopefully someone else is able to take over with help in the meantime. thanks for your quick response checking into it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants