Skip to content

Conversation

@OlaAlsaker
Copy link
Contributor

@OlaAlsaker OlaAlsaker commented Aug 7, 2024

The mergeOptions causes huge performance issues in the Vue-adapter whenever the state of the table is updated. This is probably because it merges proxies within proxies, within proxies etc.

As far as I know, there is no need for any special handling of option-merging in the Vue-adapter. Removing it fixes the problem, and the library works like it should.

@nx-cloud
Copy link

nx-cloud bot commented Aug 7, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit dcee185. 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.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Aug 7, 2024

commit: dcee185

@tanstack/angular-table

pnpm add https://pkg.pr.new/@tanstack/angular-table@5698

@tanstack/lit-table

pnpm add https://pkg.pr.new/@tanstack/lit-table@5698

@tanstack/match-sorter-utils

pnpm add https://pkg.pr.new/@tanstack/match-sorter-utils@5698

@tanstack/qwik-table

pnpm add https://pkg.pr.new/@tanstack/qwik-table@5698

@tanstack/react-table

pnpm add https://pkg.pr.new/@tanstack/react-table@5698

@tanstack/react-table-devtools

pnpm add https://pkg.pr.new/@tanstack/react-table-devtools@5698

@tanstack/solid-table

pnpm add https://pkg.pr.new/@tanstack/solid-table@5698

@tanstack/svelte-table

pnpm add https://pkg.pr.new/@tanstack/svelte-table@5698

@tanstack/table-core

pnpm add https://pkg.pr.new/@tanstack/table-core@5698

@tanstack/vue-table

pnpm add https://pkg.pr.new/@tanstack/vue-table@5698

Open in Stackblitz

More templates

@KevinVandy
Copy link
Member

@Mokshit06 Do you remember why a special proxy merge was needed in the vue adapter?

@KevinVandy KevinVandy merged commit fb2d72f into TanStack:main Aug 9, 2024
@Mokshit06
Copy link
Contributor

@KevinVandy If I remember correctly, It was so because without mergeOptions based as merging proxies, we would just spread the defaultOptions with the other options, making them no longer reactive. mergeOptions was creating a proxy of this so that it lazily accesses the latest value from the options.

@OlaAlsaker Can you check if the options still stay reactive even after this change?

@OlaAlsaker
Copy link
Contributor Author

I later found out that it was needed to make the "old" way work (using JS-getters and proxy). But when using Vue's reactivity, it is not needed. So I ended up just disabling it when the data is reactive 😄

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.

3 participants