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

**perf** improve perf, as discussed in #8091 #8092

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

joeljeske
Copy link

No description provided.

@joeljeske joeljeske changed the title [perf] improve perf, as discussed in #8091 **perf** improve perf, as discussed in #8091 Sep 23, 2024
@joeljeske joeljeske force-pushed the perf/proxy-insteadoxy-instead-of-define-prop branch from e9d1a6c to 7618bdf Compare September 25, 2024 23:57
Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I think proxies should work well. Just for context, when we started out, we had to support browsers that didn’t have proxy support and we never re-visited it.

I think there are two other places that use Object.defineProperty. Can you take a stab at them as well please?

also, docs would need an update here please:

https://tanstack.com/query/v5/docs/framework/react/guides/render-optimizations#tracked-properties

Copy link

nx-cloud bot commented Oct 1, 2024

☁️ Nx Cloud Report

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


🟥 Failed Commands
nx run-many --target=build --exclude=examples/** --exclude=integrations/**
✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@Aghassi
Copy link
Contributor

Aghassi commented Oct 3, 2024

Thanks - I think proxies should work well. Just for context, when we started out, we had to support browsers that didn’t have proxy support and we never re-visited it.

I think there are two other places that use Object.defineProperty. Can you take a stab at them as well please?

also, docs would need an update here please:

https://tanstack.com/query/v5/docs/framework/react/guides/render-optimizations#tracked-properties

@TkDodo Thanks for this! @joeljeske and I work together and from our profiling this is the only major hot spot. We didn't see improvement in updating the other call sites. Does your test suite cover any performance tests to assert no regressions if other sites are updated? I think it may be useful to incrementally land this as this one is a known issue that we discovered.

Also happy to help update the docs, what would you like updated? I can help take this over from @joeljeske as we'd love to give this fix back to the community and not maintain it internally

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 9, 2024

We didn't see improvement in updating the other call sites

Understood, but for having a unified approach, I think we should use either proxies or Object.defineProperty everywhere.

Also happy to help update the docs, what would you like updated?

This page (https://tanstack.com/query/v5/docs/framework/react/guides/render-optimizations#tracked-properties) shouldn’t refer to custom getters anymore, but state that Proxy is used instead.

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 15, 2024

@joeljeske @Aghassi do you plan to continue working on this contribution ?

@Aghassi
Copy link
Contributor

Aghassi commented Dec 3, 2024

@TkDodo yes sorry, been very busy with work and the holidays. I will be taking this up, sorry for the delay

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

Successfully merging this pull request may close these issues.

3 participants