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

V2: Remove queryKeyHashFn provided by createQueryOptions #461

Merged
merged 2 commits into from
Oct 18, 2024

Conversation

paul-sachs
Copy link
Collaborator

@paul-sachs paul-sachs commented Oct 17, 2024

The manually provided queryKeyHashFn is not compatible with the (admittedly limited) queryClient.getQueryData API

@paul-sachs paul-sachs force-pushed the psachs/v2-fix-query-client-get-query-data branch from 5324536 to 162bd94 Compare October 17, 2024 18:02
Signed-off-by: Paul Sachs <[email protected]>
@paul-sachs paul-sachs force-pushed the psachs/v2-fix-query-client-get-query-data branch from 6f28f77 to cb1e4cf Compare October 17, 2024 18:07
@paul-sachs paul-sachs requested a review from timostamm October 17, 2024 18:07
@paul-sachs paul-sachs changed the title Remove query fn provided by createQueryOptions Remove queryKeyHashFn provided by createQueryOptions Oct 17, 2024
@paul-sachs paul-sachs requested a review from smaye81 October 17, 2024 20:09
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

This looks like a shortcoming in TanStack / Query to me, but removing the property is not a big deal, it was just a very small perf improvement.

@timostamm timostamm changed the title Remove queryKeyHashFn provided by createQueryOptions V2: Remove queryKeyHashFn provided by createQueryOptions Oct 18, 2024
@timostamm timostamm merged commit 53b721f into v2 Oct 18, 2024
8 checks passed
@timostamm timostamm deleted the psachs/v2-fix-query-client-get-query-data branch October 18, 2024 10:13
@paul-sachs
Copy link
Collaborator Author

This looks like a shortcoming in TanStack / Query to me, but removing the property is not a big deal, it was just a very small perf improvement.

This is correct. The APIs in question (getQueryData and setQueryData) don't have a way of providing a custom queryKeyHashFn despite useQuery having a way to define it. We may be able to enable this with something like setQueryDefaults but for now, this unblocks us.

@paul-sachs
Copy link
Collaborator Author

I can confirm that this could be fixed with this:

queryClient.setQueryDefaults(["connect-query"], {
  queryKeyHashFn: JSON.stringify,
});
return queryClient.getQueryData(...)

Which we could reintroduce if we had a custom query client. It seems that the tanstack team seems hesitate to fix this in a cleaner way: TanStack/query#2352 (comment)

@timostamm
Copy link
Member

Sure, but this may have side-effects. Not worth it for the very small perf improvement.

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.

2 participants