feat(base-data-service): Support persisting the cache - #9445
Conversation
ceae8cf to
7412c82
Compare
## Explanation Separated out for review from MetaMask#9445, since it is needed for multiple PRs. Makes it easier to write tests for `BaseDataService` going forward. ## References N/A ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/processes/updating-changelogs.md) - [ ] I've introduced [breaking changes](https://github.com/MetaMask/core/tree/main/docs/processes/breaking-changes.md) in this PR and have prepared draft pull requests for clients and consumer packages to resolve them
| (error) => this.#messenger.captureException?.(error), | ||
| ); | ||
| }, | ||
| this.#persistConfig.debounce ?? inMilliseconds(10, Duration.Second), |
There was a problem hiding this comment.
Open to suggestions for defaults here
There was a problem hiding this comment.
Seems fine. Could consider shortening to 5 seconds if this is too slow in practice. But even 10 seconds doesn't seem bad.
There was a problem hiding this comment.
Since they are non-critical writes, my thinking was starting higher and tweaking later, but 🤷♂️
| this.#rehydrateCache().catch( | ||
| /* istanbul ignore next */ | ||
| (error) => this.#messenger.captureException?.(error), | ||
| ); |
There was a problem hiding this comment.
init does not await rehydration
Medium Severity
init() starts #rehydrateCache but returns immediately without a Promise, so callers cannot await completion. Fetches issued right after init() may run before persisted data is hydrated and can hit the network even when valid cached data exists.
Reviewed by Cursor Bugbot for commit c12b77f. Configure here.
There was a problem hiding this comment.
I don't think we want to wait for this anywhere, so this was intended 🤔
There was a problem hiding this comment.
Why don't we want to wait?
There was a problem hiding this comment.
I was thinking this would be similar to controllers providing a fire and forget init function to do setup.
I don't think we would want to block anywhere in the clients to wait for loading these persisted caches 🤔
There was a problem hiding this comment.
Yeah I guess the question is, if cache persistence is not fully initialized will this prevent the data service from functioning correctly or would it cause adverse effects? It sounds like it would not, but just want to make sure.
There was a problem hiding this comment.
It shouldn't, it just means that we may have to refetch queries. Similarly to if fetching the cache fails for whatever reason.
There was a problem hiding this comment.
Okay, that's what I thought. Yeah I guess that's not so bad.
| this.#rehydrateCache().catch( | ||
| /* istanbul ignore next */ | ||
| (error) => this.#messenger.captureException?.(error), | ||
| ); |
There was a problem hiding this comment.
Why don't we want to wait?
f0e55be to
43b812d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 43b812d. Configure here.
43b812d to
79adfe5
Compare


Explanation
Add support for cache persistence, which can be enabled by configuring
persistenceConfig. Persistence is achieved usingStorageServiceand debounced to not cause too many writes. Rehydration of the cache happens oninit, while expired or invalid data is discarded. Any errors with persistence are logged, but ultimately ignored, due to the reduced importance of the data persisted.This approach of storing the dehydrated state is similar to how
@tanstack/query-persist-client-coreworks.References
https://consensyssoftware.atlassian.net/browse/WPC-1128
Checklist
Note
Medium Risk
Touches shared data-layer caching and storage I/O; stale or rehydrated cache could affect offline/API behavior, though persistence is opt-in and errors are non-fatal.
Overview
Adds optional TanStack Query cache persistence to
BaseDataServicevia a new constructorpersistenceConfig(maxAge, optionalwriteDelay/maxWriteDelay). When enabled, successful query cache entries are dehydrated and written through messenger actions to@metamask/storage-serviceunder keycache, with lodash debounced writes after cache add/update/remove andremoveItemwhen the cache is empty.Introduces
init()to rehydrate from storage on startup; entries older thanmaxAgeare dropped and removed. Persistence is off unlesspersistenceConfigis set; storage failures are swallowed (logged viacaptureException).destroy()now cancels pending debounced writes.The package gains dependencies on
@metamask/storage-serviceandlodash, exportsPersistenceConfiguration, and includes broad persistence tests. README dependency graph and changelog are updated.Reviewed by Cursor Bugbot for commit 79adfe5. Bugbot is set up for automated code reviews on this repo. Configure here.