fix(query-persist-client-core): restore undefined persisted query payloads#10842
Open
raashish1601 wants to merge 2 commits into
Open
fix(query-persist-client-core): restore undefined persisted query payloads#10842raashish1601 wants to merge 2 commits into
raashish1601 wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors internal persistence data shapes by wrapping restored query payloads in a ChangesQuery Persistence and Environment Detection Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/query-persist-client-core/src/createPersister.ts (1)
156-156: JSON serialization silently dropsundefinedvalues.The default serializer
JSON.stringifyomits object properties withundefinedvalues, so{ state: { data: undefined } }becomes'{"state":{}}'. On deserialization,persistedQuery.state.dataisundefined(missing property access), which coincidentally produces the correct result{ data: undefined }. This works but is fragile: custom serializers that explicitly encodeundefineddifferently (e.g., asnullor a sentinel) may break this assumption.Consider documenting this behavior in the
serializeoption's JSDoc, or use a more explicit encoding like{ hasData: boolean, data: T }if custom serializers are expected to handleundefineddifferently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/query-persist-client-core/src/createPersister.ts` at line 156, The code assumes missing vs explicit undefined are equivalent when returning persistedQuery.state.data in createPersister (the line returning { data: persistedQuery.state.data as T }), which breaks with custom serializers that encode undefined differently; change the persisted shape to an explicit envelope like { hasData: boolean, data?: T } when serializing and deserializing so presence is unambiguous: update the serializer/deserializer usage in createPersister (and where persistedQuery.state is created/read) to write hasData = Object.prototype.hasOwnProperty.call(payload, 'data') (or data !== undefined) and to return { data: hasData ? payload.data : undefined }, and update related types/JSDoc for the serialize option accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/query-persist-client-core/src/createPersister.ts`:
- Line 132: The public API for retrieveQuery on the object returned by
experimental_createQueryPersister now returns Promise<PersistedResult<T> |
undefined> (an object with a data property) instead of Promise<T | undefined>;
update all consumer-facing docs (docs/framework/react/plugins/createPersister.md
and docs/framework/vue/plugins/createPersister.md) to reflect the new signature
and show unwrapping via result?.data, and either provide a note/example in the
docs demonstrating how to migrate callers of retrieveQuery to use result.data or
restore a thin helper on the persister (e.g., retrieveQueryValue) that returns T
| undefined for backward compatibility while keeping retrieveQuery typed as
PersistedResult<T> | undefined.
---
Nitpick comments:
In `@packages/query-persist-client-core/src/createPersister.ts`:
- Line 156: The code assumes missing vs explicit undefined are equivalent when
returning persistedQuery.state.data in createPersister (the line returning {
data: persistedQuery.state.data as T }), which breaks with custom serializers
that encode undefined differently; change the persisted shape to an explicit
envelope like { hasData: boolean, data?: T } when serializing and deserializing
so presence is unambiguous: update the serializer/deserializer usage in
createPersister (and where persistedQuery.state is created/read) to write
hasData = Object.prototype.hasOwnProperty.call(payload, 'data') (or data !==
undefined) and to return { data: hasData ? payload.data : undefined }, and
update related types/JSDoc for the serialize option accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0269f704-bfb9-4474-b9c0-15b2bb400b9a
📒 Files selected for processing (4)
packages/query-persist-client-core/src/__tests__/createPersister.test.tspackages/query-persist-client-core/src/createPersister.tspackages/vue-query/src/__tests__/vueQueryPlugin.test.tspackages/vue-query/src/vueQueryPlugin.ts
| queryHash: string, | ||
| afterRestoreMacroTask?: (persistedQuery: PersistedQuery) => void, | ||
| ) { | ||
| ): Promise<PersistedResult<T> | undefined> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for direct usages of retrieveQuery outside this file to assess impact
# Search for retrieveQuery calls that may be affected by the return type change
rg -nP --type=ts --type=tsx -C3 'retrieveQuery\s*\(' \
-g '!**/createPersister.ts' \
-g '!**/__tests__/**' \
-g '!**/node_modules/**'Repository: TanStack/query
Length of output: 86
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect createPersister.ts around retrieveQuery export + its return type
FILE="packages/query-persist-client-core/src/createPersister.ts"
if [ -f "$FILE" ]; then
echo "== retrieveQuery export context =="
rg -n "export\s+(async\s+)?function\s+retrieveQuery|export\s*\{\s*retrieveQuery|retrieveQuery\s*=" "$FILE"
echo
# Print a small window around the first match
LINE=$(rg -n "export\s+(async\s+)?function\s+retrieveQuery|export\s*\{\s*retrieveQuery|retrieveQuery\s*=" "$FILE" | head -n1 | cut -d: -f1)
START=$((LINE-20))
END=$((LINE+60))
sed -n "${START},${END}p" "$FILE" | nl -ba
fi
# 2) Search for direct usages of retrieveQuery outside createPersister.ts
echo
echo "== retrieveQuery call sites outside createPersister.ts =="
rg -n "retrieveQuery\s*\(" \
--glob "!**/createPersister.ts" \
--glob "!**/__tests__/**" \
--glob "!**/node_modules/**" \
--glob "**/*.ts" \
--glob "**/*.tsx" \
-SRepository: TanStack/query
Length of output: 93
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate retrieveQuery in the target file
FILE="packages/query-persist-client-core/src/createPersister.ts"
echo "== locate retrieveQuery in $FILE =="
if [ -f "$FILE" ]; then
rg -n "retrieveQuery" "$FILE" || true
echo
# show around the most likely export area (lines near 371 as mentioned in review comment)
sed -n '340,420p' "$FILE" | nl -ba | sed -n '1,120p'
echo
# show around line 120-160 for the type snippet mentioned
sed -n '100,170p' "$FILE" | nl -ba
else
echo "File not found: $FILE"
fi
echo
echo "== locate retrieveQuery across repository (excluding node_modules) =="
rg -n "retrieveQuery" \
--glob "!**/node_modules/**" \
--glob "!**/dist/**" \
--glob "!**/build/**" \
-SRepository: TanStack/query
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/query-persist-client-core/src/createPersister.ts"
echo "== show retrieveQuery definition around line 129 =="
sed -n '110,170p' "$FILE" | sed -n '1,120p'
echo
echo "== show internal call around line 216 (restoredData) =="
sed -n '190,260p' "$FILE" | sed -n '1,120p'
echo
echo "== show export block around line 371 =="
sed -n '350,410p' "$FILE" | sed -n '1,120p'
echo
echo "== locate retrieveQuery references across repository (excluding node_modules/dist/build) =="
rg -n "retrieveQuery\s*<|retrieveQuery\s*\(" \
--glob "!**/node_modules/**" \
--glob "!**/dist/**" \
--glob "!**/build/**" \
-SRepository: TanStack/query
Length of output: 5586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "retrieveQuery<T>\(queryHash" docs packages -S || true
echo
echo "== docs/framework/react/plugins/createPersister.md excerpt =="
sed -n '80,130p' docs/framework/react/plugins/createPersister.md | nl -ba | head -n 80 || true
echo
echo "== docs/framework/vue/plugins/createPersister.md excerpt =="
sed -n '70,120p' docs/framework/vue/plugins/createPersister.md | nl -ba | head -n 70 || true
echo
echo "== search for PersistedResult in docs/framework/** createPersister =="
rg -n "PersistedResult" docs/framework/react/plugins/createPersister.md docs/framework/vue/plugins/createPersister.md -S || true
echo
echo "== search for retrieveQuery return type hints in typescript files =="
rg -n "retrieveQuery.*Promise<" packages docs -S || trueRepository: TanStack/query
Length of output: 890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/query-persist-client-core/src/createPersister.ts"
echo "== find PersistedResult definition in createPersister.ts =="
rg -n "type\s+PersistedResult|interface\s+PersistedResult|export\s+type\s+PersistedResult|PersistedResult\s*<" "$FILE" -S || true
echo
echo "== find retrieveQuery signature types near its declaration =="
sed -n '120,155p' "$FILE" | sed -n '1,120p'
echo
echo "== find where createPersister returns exported API shape (persister object) =="
rg -n "return\s*\{\s*persisterFn|return\s*\{" "$FILE" -n -S || true
echo
echo "== look for exported type(s) for the persister returned by createPersister =="
rg -n "export (type|interface) .*Persister|createPersister.*export|PersisterOptions" packages/query-persist-client-core/src -S || trueRepository: TanStack/query
Length of output: 2447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="packages/query-persist-client-core/src/createPersister.ts"
echo "== show top of createPersister.ts (types + exports) =="
sed -n '1,120p' "$FILE"
echo
echo "== show createPersister function signature and return statement =="
sed -n '120,220p' "$FILE"
sed -n '320,410p' "$FILE"
echo
echo "== check for public return type/interface mentioning retrieveQuery =="
rg -n "createPersister|retrieveQuery|Persister" "$FILE"
echo
echo "== inspect docs for retrieveQuery return type formatting around those lines =="
sed -n '85,120p' docs/framework/react/plugins/createPersister.md
sed -n '80,115p' docs/framework/vue/plugins/createPersister.mdRepository: TanStack/query
Length of output: 11743
Fix/communicate the public retrieveQuery return type change
retrieveQuery is exposed on the persister object returned by experimental_createQueryPersister, and it now returns Promise<PersistedResult<T> | undefined> (i.e., { data: T }) instead of Promise<T | undefined>. Consumers must unwrap result.data.
Docs are still written for the old signature in docs/framework/react/plugins/createPersister.md and docs/framework/vue/plugins/createPersister.md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/query-persist-client-core/src/createPersister.ts` at line 132, The
public API for retrieveQuery on the object returned by
experimental_createQueryPersister now returns Promise<PersistedResult<T> |
undefined> (an object with a data property) instead of Promise<T | undefined>;
update all consumer-facing docs (docs/framework/react/plugins/createPersister.md
and docs/framework/vue/plugins/createPersister.md) to reflect the new signature
and show unwrapping via result?.data, and either provide a note/example in the
docs demonstrating how to migrate callers of retrieveQuery to use result.data or
restore a thin helper on the persister (e.g., retrieveQueryValue) that returns T
| undefined for backward compatibility while keeping retrieveQuery typed as
PersistedResult<T> | undefined.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
efetchOnRestore: false.
Validation
px -y pnpm@11.0.0 vitest run packages/query-persist-client-core/src/tests/createPersister.test.ts
Summary by CodeRabbit
Bug Fixes
undefineddata.Improvements