feat: replace JSON.stringify with replaceDeepEqual in structural sharing integrity check#8030
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit cd2c14c. 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 targetsSent with 💌 from NxCloud. |
packages/query-core/src/utils.ts
Outdated
| if (seen.has(a)) { | ||
| throw new Error('circular reference detected.') | ||
| } | ||
| seen.add(a) |
packages/query-core/src/utils.ts
Outdated
| if (a === b) { | ||
| return a | ||
| } | ||
| const seen = new WeakSet() |
More templates
@tanstack/eslint-plugin-query
@tanstack/angular-query-experimental
@tanstack/query-async-storage-persister
@tanstack/query-broadcast-client-experimental
@tanstack/query-core
@tanstack/query-devtools
@tanstack/query-persist-client-core
@tanstack/query-sync-storage-persister
@tanstack/react-query
@tanstack/react-query-devtools
@tanstack/react-query-next-experimental
@tanstack/react-query-persist-client
@tanstack/solid-query
@tanstack/solid-query-devtools
@tanstack/solid-query-persist-client
@tanstack/svelte-query
@tanstack/svelte-query-devtools
@tanstack/svelte-query-persist-client
@tanstack/vue-query
@tanstack/vue-query-devtools
@tanstack/angular-query-devtools-experimental
commit: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8030 +/- ##
=========================================
+ Coverage 0 61.85% +61.85%
=========================================
Files 0 135 +135
Lines 0 4679 +4679
Branches 0 1306 +1306
=========================================
+ Hits 0 2894 +2894
- Misses 0 1543 +1543
- Partials 0 242 +242 |
TkDodo
left a comment
There was a problem hiding this comment.
I think we had a bit of a miscommunication. Ideally, we don't want any runtime changes for production builds - that's why the initial code was behind an env check.
So my idea was something like this:
if (process.env.NODE_ENV !== 'production') {
try {
return replaceEqualDeep(prevData, data)
}
catch(error) {
console.error(...)
}
}
return replaceEqualDeep(prevData, data)
This will remove the dev mode overhead of json stringify. If everything goes right, replaceEqualDeep will only be called once. If there is a circularity, the first call will log the error and the second call will then throw, so it will be caught in setData and the query will go to error state.
I think this would be the best of both worlds.
packages/query-core/src/utils.ts
Outdated
| // Structurally share data between prev and new data if needed | ||
| return replaceEqualDeep(prevData, data) | ||
| } catch (error) { | ||
| console.error( |
There was a problem hiding this comment.
Realised this should be under a dev env guard. Will fix when I’m back at computer.
Got it. Can do that! |
replaceDeepEqual contains circular referencesJSON.stringify with replaceEqualDeep in structural sharing integrity check
|
@TkDodo - lmk what you think |
JSON.stringify with replaceEqualDeep in structural sharing integrity checkJSON.stringify with replaceDeepEqual in structural sharing integrity check
This PR is a follow up on #7966 to:
not surface console errors for values that are referentially stable (e.g.BigInt)detect circular references inreplaceDeepEqualcatch errors fromreplaceDeepEqualinreplaceData, and surface them in the console + propagated to the query.JSON.stringifywithreplaceDeepEqualin structural sharing integrity check