fix(query-core): handle errors that occur inside `setData` method #7966

Efimenko · 2024-08-27T14:48:20Z

As PR is a bit stuck, this PR implements the same, with respect to the comments left in the first PR. Fix #6954

Efimenko · 2024-08-27T14:59:00Z

@TkDodo Also, currently, after the second fetch, the error will be maximum call stack size exceeded. So, probably, we need to check if the data is serializable or not, rather than wait call stack size exceeded. Inside replaceEqualDeep, some kind of:

try {
  JSON.stringify(data)
} catch {
  throw new Error('Data is not serializable')
}

packages/query-core/src/__tests__/query.test.tsx

nx-cloud · 2024-08-27T15:49:39Z

☁️ Nx Cloud Report

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

packages/query-core/src/query.ts

pkg-pr-new · 2024-08-27T15:51:45Z

commit: 82d75b7

@tanstack/angular-query-devtools-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@7966

@tanstack/angular-query-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-experimental@7966

@tanstack/eslint-plugin-query

pnpm add https://pkg.pr.new/@tanstack/eslint-plugin-query@7966

@tanstack/query-async-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-async-storage-persister@7966

@tanstack/query-broadcast-client-experimental

pnpm add https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@7966

@tanstack/query-core

pnpm add https://pkg.pr.new/@tanstack/query-core@7966

@tanstack/query-devtools

pnpm add https://pkg.pr.new/@tanstack/query-devtools@7966

@tanstack/query-persist-client-core

pnpm add https://pkg.pr.new/@tanstack/query-persist-client-core@7966

@tanstack/query-sync-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-sync-storage-persister@7966

@tanstack/react-query

pnpm add https://pkg.pr.new/@tanstack/react-query@7966

@tanstack/react-query-devtools

pnpm add https://pkg.pr.new/@tanstack/react-query-devtools@7966

@tanstack/react-query-next-experimental

pnpm add https://pkg.pr.new/@tanstack/react-query-next-experimental@7966

@tanstack/react-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/react-query-persist-client@7966

@tanstack/solid-query

pnpm add https://pkg.pr.new/@tanstack/solid-query@7966

@tanstack/solid-query-devtools

pnpm add https://pkg.pr.new/@tanstack/solid-query-devtools@7966

@tanstack/solid-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/solid-query-persist-client@7966

@tanstack/svelte-query

pnpm add https://pkg.pr.new/@tanstack/svelte-query@7966

@tanstack/svelte-query-devtools

pnpm add https://pkg.pr.new/@tanstack/svelte-query-devtools@7966

@tanstack/svelte-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/svelte-query-persist-client@7966

@tanstack/vue-query

pnpm add https://pkg.pr.new/@tanstack/vue-query@7966

@tanstack/vue-query-devtools

pnpm add https://pkg.pr.new/@tanstack/vue-query-devtools@7966

Open in Stackblitz

More templates

…e for non production env

packages/query-core/src/utils.ts

codecov · 2024-08-30T10:07:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.36%. Comparing base (82bfc34) to head (82d75b7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #7966       +/-   ##
===========================================
+ Coverage   45.09%   63.36%   +18.26%     
===========================================
  Files         188      127       -61     
  Lines        7149     4545     -2604     
  Branches     1604     1267      -337     
===========================================
- Hits         3224     2880      -344     
+ Misses       3562     1437     -2125     
+ Partials      363      228      -135

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.87% <100.00%> (+0.04%)`	⬆️
@tanstack/query-devtools	`5.24% <ø> (ø)`
@tanstack/query-persist-client-core	`57.73% <ø> (ø)`
@tanstack/query-sync-storage-persister	`82.50% <ø> (ø)`
@tanstack/react-query	`92.50% <ø> (ø)`
@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

thanks. I adapted the message a bit

ethos-vitalii · 2024-08-30T23:19:51Z

packages/query-core/src/utils.ts

+    if (process.env.NODE_ENV !== 'production') {
+      try {
+        JSON.stringify(prevData)
+        JSON.stringify(data)


This now throws an error when data contains BigInt. But it seems like react-query supports having BigInt in the query result. Should I create an issue for this?

I'm think it's still good to log the error because structuralSharing creates an overhead and you should likely turn it off if you have something that isn't serializable. But we should likely remove the throw. Logging is enough. Please file a PR

Thank you @Efimenko for raising that PR 🙌

@TkDodo I have a question. From what I see in replaceEqualDeep, there's nothing leading BigInt comparison to break. For example, 123n === 123n will be true. Is there anything else I'm missing where BigInt could break this.setData()?

interesting. I didn't know that BigInt was referentially equal, but not json serializable. According to the documentation, we only support json serializable values in structuralSharing, so I think the warning is still warranted.

Yeah, this seems to have affected a lot of upstream Wagmi users. Wonder if this should have been a breaking change. Changing from throwing to just a warning would be better though!

It's "just" a dev mode warning. Why would it be breaking?

Apologies. Didn't see this commit.

@TkDodo – would you be open to a PR that checked for cyclic references instead of checking for non-serializable values via JSON.stringify? Feel like a lot of consumers would be invoking query functions that return BigInts, so it might be a bit odd seeing these warning messages flying around when replaceEqualDeep is also compatible with BigInts.

Wonder if it’s worth conveying that structural sharing is moreso only compatible with referentially stable values instead of JSON-serializable values.

Wonder if it’s worth conveying that structural sharing is moreso only compatible with referentially stable values instead of JSON-serializable values.

I simply wasn't aware that replaceEqualDeep works with BigInt - our general recommendation is to use json serializable values. If we say BigInt is supported, then why isn't Map supported, or Set, or Date, or ... it's a slippery slope. Also, in queryKeys, we actually use JSON.stringify, so if you use a BigInt there, it will fail.

Why is JavaScript so weird that BigInt is referentially stable across values, but not serializable lol.

would you be open to a PR that checked for cyclic references instead of checking for non-serializable values via JSON.stringify?

yeah I guess that's fine. I think just calling replaceEqualDeep itself in a try/catch would just work, too ?

done! #8030

Efimenko added 2 commits August 27, 2024 17:42

fix(query-core): add test case for not serializable data

a9e281b

fix(query-core): handle setData error

b04bf82

github-actions bot added the package: query-core label Aug 27, 2024

TkDodo mentioned this pull request Aug 27, 2024

fix(query-core): add error management around this.setData(data) #7642

Closed

TkDodo reviewed Aug 27, 2024

View reviewed changes

packages/query-core/src/__tests__/query.test.tsx Outdated Show resolved Hide resolved

packages/query-core/src/__tests__/query.test.tsx Outdated Show resolved Hide resolved

Update packages/query-core/src/__tests__/query.test.tsx

02f6fa5

TkDodo reviewed Aug 27, 2024

View reviewed changes

packages/query-core/src/query.ts Outdated Show resolved Hide resolved

Efimenko added 2 commits August 27, 2024 18:56

fix(query-core): add return statement

18ae646

Merge branch 'issue-6954' of github.com:Efimenko/query into issue-6954

87b241e

Efimenko requested a review from TkDodo August 27, 2024 15:58

Efimenko added 3 commits August 27, 2024 19:36

fix(query-core): throw the error in case when data is not serializabl…

1ff334e

…e for non production env

fix(query-core): delete packages/query-core/tsconfig.vitest-temp.json

b0627f5

Merge branch 'main' into issue-6954

af4584a

TkDodo requested changes Aug 28, 2024

View reviewed changes

packages/query-core/src/utils.ts Outdated Show resolved Hide resolved

Efimenko added 2 commits August 28, 2024 22:41

fix(query-core): lift serializability check and add console.error

e733174

Merge branch 'issue-6954' of github.com:Efimenko/query into issue-6954

3f2b14b

Efimenko requested a review from TkDodo August 30, 2024 08:29

Merge branch 'main' into issue-6954

93d14ec

chore: message

82d75b7

TkDodo approved these changes Aug 30, 2024

View reviewed changes

TkDodo merged commit 50315ac into TanStack:main Aug 30, 2024
8 checks passed

Efimenko deleted the issue-6954 branch August 30, 2024 17:29

ethos-vitalii reviewed Aug 30, 2024

View reviewed changes

Efimenko mentioned this pull request Sep 2, 2024

fix(query-core): Remove error thrown inside replaceData #8004

Merged

jxom mentioned this pull request Sep 8, 2024

feat: replace JSON.stringify with replaceDeepEqual in structural sharing integrity check #8030

Merged

DimitriRomano mentioned this pull request Sep 20, 2024

[Snyk] Upgrade: , jotai, lucide-react, react-hook-form, react-router-dom, tailwind-merge Munch-Ynov/munch-front#15

Open

This was referenced Sep 21, 2024

[Snyk] Upgrade @tanstack/react-query from 5.52.2 to 5.53.1 zntb/ReactEvents-With-TanStack-Query#20

Merged

[Snyk] Upgrade: , , zntb/crypto-ticker#47

Merged

CalPinSW mentioned this pull request Sep 21, 2024

[Snyk] Upgrade: , CalPinSW/playlist-manager#52

Merged

zntb mentioned this pull request Sep 21, 2024

[Snyk] Upgrade: , viem zntb/nextjs-web3-app-zksync#65

Closed

ypreiser mentioned this pull request Sep 21, 2024

[Snyk] Upgrade @tanstack/react-query from 5.52.2 to 5.53.1 ypreiser/TDD#4

Merged

DimitriRomano mentioned this pull request Sep 21, 2024

[Snyk] Upgrade: , jotai, lucide-react, react-hook-form, react-router-dom, tailwind-merge Munch-Ynov/munch-front#16

Open

DimitriRomano mentioned this pull request Sep 23, 2024

[Snyk] Upgrade: , jotai, lucide-react, react-hook-form, react-router-dom, tailwind-merge Munch-Ynov/munch-front#17

Open

This was referenced Sep 24, 2024

[Snyk] Upgrade @tanstack/react-query from 5.53.1 to 5.53.3 ypreiser/TDD#5

Open

[Snyk] Upgrade @tanstack/react-query from 5.53.1 to 5.54.1 ypreiser/TDD#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(query-core): handle errors that occur inside `setData` method #7966

fix(query-core): handle errors that occur inside `setData` method #7966

Efimenko commented Aug 27, 2024

Efimenko commented Aug 27, 2024

nx-cloud bot commented Aug 27, 2024 •

edited

Loading

pkg-pr-new bot commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

TkDodo left a comment

ethos-vitalii Aug 30, 2024

TkDodo Aug 31, 2024

ethos-vitalii Sep 2, 2024 •

edited

Loading

TkDodo Sep 3, 2024

jxom Sep 6, 2024 •

edited

Loading

TkDodo Sep 7, 2024

jxom Sep 7, 2024

jxom Sep 7, 2024 •

edited

Loading

TkDodo Sep 8, 2024

jxom Sep 8, 2024

fix(query-core): handle errors that occur inside setData method #7966

fix(query-core): handle errors that occur inside setData method #7966

Conversation

Efimenko commented Aug 27, 2024

Efimenko commented Aug 27, 2024

nx-cloud bot commented Aug 27, 2024 • edited Loading

☁️ Nx Cloud Report

pkg-pr-new bot commented Aug 27, 2024 • edited Loading

codecov bot commented Aug 30, 2024 • edited Loading

Codecov Report

TkDodo left a comment

Choose a reason for hiding this comment

ethos-vitalii Aug 30, 2024

Choose a reason for hiding this comment

TkDodo Aug 31, 2024

Choose a reason for hiding this comment

ethos-vitalii Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

TkDodo Sep 3, 2024

Choose a reason for hiding this comment

jxom Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

TkDodo Sep 7, 2024

Choose a reason for hiding this comment

jxom Sep 7, 2024

Choose a reason for hiding this comment

jxom Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

TkDodo Sep 8, 2024

Choose a reason for hiding this comment

jxom Sep 8, 2024

Choose a reason for hiding this comment

fix(query-core): handle errors that occur inside `setData` method #7966

fix(query-core): handle errors that occur inside `setData` method #7966

nx-cloud bot commented Aug 27, 2024 •

edited

Loading

pkg-pr-new bot commented Aug 27, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

ethos-vitalii Sep 2, 2024 •

edited

Loading

jxom Sep 6, 2024 •

edited

Loading

jxom Sep 7, 2024 •

edited

Loading