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(query-core): add support for symbol in cache #7857

Closed
wants to merge 1 commit into from

Conversation

Ayc0
Copy link
Contributor

@Ayc0 Ayc0 commented Aug 5, 2024

I noticed when I was using symbols as keys in the value of objects with queryCache.setQueryData() that keys containing symbol were sometimes stripped out

This PR addresses this by using Object.getOwnPropertySymbols() to iterate over symbol keys in the cache

I added new tests & ran test before and after my changes and they indeed fail on before, and pass after

Before After
image image

Copy link

nx-cloud bot commented Aug 5, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a7ad13c. 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 5, 2024

commit: a7ad13c

pnpm add https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@7857
pnpm add https://pkg.pr.new/@tanstack/angular-query-experimental@7857
pnpm add https://pkg.pr.new/@tanstack/eslint-plugin-query@7857
pnpm add https://pkg.pr.new/@tanstack/query-async-storage-persister@7857
pnpm add https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@7857
pnpm add https://pkg.pr.new/@tanstack/query-core@7857
pnpm add https://pkg.pr.new/@tanstack/query-devtools@7857
pnpm add https://pkg.pr.new/@tanstack/query-persist-client-core@7857
pnpm add https://pkg.pr.new/@tanstack/query-sync-storage-persister@7857
pnpm add https://pkg.pr.new/@tanstack/react-query@7857
pnpm add https://pkg.pr.new/@tanstack/react-query-devtools@7857
pnpm add https://pkg.pr.new/@tanstack/react-query-next-experimental@7857
pnpm add https://pkg.pr.new/@tanstack/react-query-persist-client@7857
pnpm add https://pkg.pr.new/@tanstack/solid-query@7857
pnpm add https://pkg.pr.new/@tanstack/solid-query-devtools@7857
pnpm add https://pkg.pr.new/@tanstack/solid-query-persist-client@7857
pnpm add https://pkg.pr.new/@tanstack/svelte-query@7857
pnpm add https://pkg.pr.new/@tanstack/svelte-query-devtools@7857
pnpm add https://pkg.pr.new/@tanstack/svelte-query-persist-client@7857
pnpm add https://pkg.pr.new/@tanstack/vue-query@7857
pnpm add https://pkg.pr.new/@tanstack/vue-query-devtools@7857

Open in Stackblitz

More templates

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.31%. Comparing base (075ba1c) to head (a7ad13c).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           main    #7857       +/-   ##
=========================================
+ Coverage      0   63.31%   +63.31%     
=========================================
  Files         0      127      +127     
  Lines         0     4539     +4539     
  Branches      0     1266     +1266     
=========================================
+ Hits          0     2874     +2874     
- Misses        0     1437     +1437     
- Partials      0      228      +228     
Components Coverage Δ
@tanstack/angular-query-devtools-experimental ∅ <ø> (∅)
@tanstack/angular-query-experimental 86.58% <ø> (∅)
@tanstack/eslint-plugin-query ∅ <ø> (∅)
@tanstack/query-async-storage-persister 43.85% <ø> (∅)
@tanstack/query-broadcast-client-experimental ∅ <ø> (∅)
@tanstack/query-codemods ∅ <ø> (∅)
@tanstack/query-core 92.85% <100.00%> (∅)
@tanstack/query-devtools 5.24% <ø> (∅)
@tanstack/query-persist-client-core 57.73% <ø> (∅)
@tanstack/query-sync-storage-persister 82.50% <ø> (∅)
@tanstack/react-query 92.44% <ø> (∅)
@tanstack/react-query-devtools 10.71% <ø> (∅)
@tanstack/react-query-next-experimental ∅ <ø> (∅)
@tanstack/react-query-persist-client 100.00% <ø> (∅)
@tanstack/solid-query 78.20% <ø> (∅)
@tanstack/solid-query-devtools ∅ <ø> (∅)
@tanstack/solid-query-persist-client 100.00% <ø> (∅)
@tanstack/svelte-query 87.33% <ø> (∅)
@tanstack/svelte-query-devtools ∅ <ø> (∅)
@tanstack/svelte-query-persist-client 100.00% <ø> (∅)
@tanstack/vue-query 71.42% <ø> (∅)
@tanstack/vue-query-devtools ∅ <ø> (∅)

@TkDodo
Copy link
Collaborator

TkDodo commented Aug 5, 2024

sorry, symbols aren't supported in query keys because they are't json serializable, which is what we use for hashing. You can address that on your end by providing a queryKeyHashFn that hashes those into something unique. This can also be provided globally.

@TkDodo TkDodo closed this Aug 5, 2024
@Ayc0
Copy link
Contributor Author

Ayc0 commented Aug 5, 2024

But those aren't in query types. But in the cached values

I didn't change the logic of the keys themselves (I saw a Object.keys for those but I kept it as it is)

@Ayc0
Copy link
Contributor Author

Ayc0 commented Aug 5, 2024

You can see here in the code:

const key = queryKey()
const s = Symbol('s');

queryClient.setQueryData(key, { [s]: 1, a: 1 })

The symbol is not in the query key but in the data itself

@TkDodo
Copy link
Collaborator

TkDodo commented Aug 5, 2024

ah, sorry about that. So you basically changed the implementation of replaceEqualDeep ? This feature is for structural sharing, which again, also only supports json serializable structures per default. This too can be customized by passing your own structuralSharing: (old, new) => merged function.


The reason why I'm hesitant to add this to the library is because once we go away from "must be json serializable", we have to (and already did get) many requests about Maps, Sets, Dates etc. superjson is generally really good at that.

@Ayc0
Copy link
Contributor Author

Ayc0 commented Aug 5, 2024

Makes sense, I'll check that then

@Ayc0 Ayc0 deleted the Ayc0/symbol branch August 6, 2024 07:43
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.

2 participants