fix(query-persist-client-core/createPersister): await 'storage.setItem' when persisting a query - #11410
Conversation
…m' when persisting a query `persistQuery` in packages/query-persist-client-core/src/createPersister.ts awaited `serialize` but not `storage.setItem`, so the promise it returns resolved as soon as the write was started rather than when it finished. Two consequences with an asynchronous storage such as React Native AsyncStorage or idb-keyval. `await persister.persistQueryByKey(queryKey, queryClient)`, which the createPersister docs show being used in `onMutate` to persist an optimistic update, returned before the entry was in storage. And a failing write, for example a quota error, surfaced as an unhandled promise rejection instead of rejecting the returned promise. `createAsyncStoragePersister` already awaits its `storage.setItem` call in packages/query-async-storage-persister/src/index.ts. Adding the `await` fixes both. The two new tests in packages/query-persist-client-core/src/__tests__/createPersister.test.ts cover the ordering and the rejection, and both fail without the change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesPersisted storage write handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Persistence now completes only after storage writes finish, and storage write failures propagate to callers. The changed completion and error behavior is covered by focused tests, with no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
✨ 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 |
1 participant
🎯 Changes
persistQueryinpackages/query-persist-client-core/src/createPersister.tsawaitsserializebut notstorage.setItem, so the promise it returns resolves as soon as the write is started rather than when it finishes. This adds the missingawait.Two things go wrong with an asynchronous storage such as React Native AsyncStorage or
idb-keyval. First,await persister.persistQueryByKey(queryKey, queryClient)returns before the entry is actually in storage, which matters because the createPersister docs show that call being used insideonMutateto persist an optimistic update. Second, a write that rejects, a quota error for example, becomes an unhandled promise rejection instead of rejecting the promise the caller is awaiting.createAsyncStoragePersisteralready awaits itsstorage.setItemcall inpackages/query-async-storage-persister/src/index.ts, so this makes the two persisters agree.The existing tests miss this because their mock storage writes into a
Mapsynchronously and then returns an already resolved promise, which hides the difference. The two tests I added tocreatePersister.test.tsuse a storage whose write is gated on a promise the test resolves, and a storage whose write rejects. Both fail onmainand pass with the one line change.Verification, all from the repo root:
pnpm nx run @tanstack/query-persist-client-core:test:libgoes from 55 passing to 57 passing, and the two new tests fail on unmodifiedmain(the rejection one also surfaces the unhandled rejection there).pnpm nx run @tanstack/query-persist-client-core:test:eslintreports 0 errors and the 2 pre-existingno-shadowwarnings on lines 275 and 323, which this change does not touch.pnpm nx run @tanstack/query-persist-client-core:test:typespasses on TS 5.6 through 7.0.pnpm run test:lib,pnpm run test:types,pnpm run test:build,pnpm run build,pnpm run test:sherif,pnpm run test:knipandpnpm run test:docsall pass.pnpm exec prettier --checkis clean on the three changed files.✅ Checklist
pnpm run test:pr, or these tests do not apply to this pull request.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests