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

fix: major performance bug in Vue-adapter #5698

Merged
merged 2 commits into from
Aug 9, 2024

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.

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.

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
5 checks passed
@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