Open
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
FrederikBolding
force-pushed
the
fb/data-service-base
branch
3 times, most recently
from
1b629b1 to
c19b3d7
Compare
Member
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
FrederikBolding
force-pushed
the
fb/data-service-base
branch
from
86eff63 to
5111712
Compare
Member
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
Member
Author
|
@metamaskbot publish-previews |
FrederikBolding
force-pushed
the
fb/data-service-base
branch
from
1afe924 to
c36e274
Compare
Member
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
FrederikBolding
commented
Mar 10, 2026
| */ | ||
| const ALLOWED_INCONSISTENT_DEPENDENCIES = { | ||
| // '@metamask/json-rpc-engine': ['^9.0.3'], | ||
| '@tanstack/query-core': ['^4.43.0'], |
Member
Author
There was a problem hiding this comment.
core-backend will need to downgrade to use BaseDataService or wait until we are able to bump everywhere (blocked by extension not supporting React 18).
mcmire
reviewed
Mar 10, 2026
Contributor
|
I know that TanStack Query is the motivation for this PR, but before we merge this I want to make sure that we've (at least briefly) considered how this new package would overlap with the other "data service layer" features which are already implemented and which we've discussed adding in the future. Namely:
I guess the theme of these questions is that I want to understand what the intended domain of this package is. Should it be only restricted to TanStack Query integration in the future — meaning that we may create other packages to solve other problems later — or should it be designed to encompass other things that are data-service-related in the future? |
Member
|
I was expecting that we'd continue to use Cockatiel. TanStack does have basic built-in retry functionality, but nothing remotely similar to what our current retry/circuit break policies do. |
Member
|
Related question: Are there any data services where this query-related functionality would not be useful? i.e. where extending this base class would be unwanted. If so, we could rename this to I think the answer here is "no" though. When wouldn't we want request deduplication, and easier-to-use caching options? |
4 tasks
mcmire
reviewed
Mar 11, 2026
Contributor
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Still working through this PR, but made some more suggestions.
| }) { | ||
| this.name = name; | ||
|
|
||
| this.#messenger = messenger as unknown as Messenger< |
Contributor
There was a problem hiding this comment.
Should we add a comment explaining why we are doing this? (It's essentially the same reason we are doing it in BaseController, I think.) Unless there is a way to avoid this?
| #setupCacheListener(): void { | ||
| this.#client.getQueryCache().subscribe((event) => { | ||
| if (['added', 'updated', 'removed'].includes(event.type)) { | ||
| this.#broadcastCacheUpdate(event.query.queryKey); |
Contributor
There was a problem hiding this comment.
I see that we are publishing the same :cacheUpdate event even if a cache key is added or removed. Do we want to have separate events? Should we at least not publish :cacheUpdate if a key is removed? Or maybe I am misunderstanding how we intend this event to be used.
Member
Author
There was a problem hiding this comment.
cacheUpdate contains the full cache for a specific query key (not the diff). The assumption is that publishing that is enough to add, update and clear out any cache entries on the UI-side. Thinking more about it, I should verify that removed works as expected though.
We could have separate events and/or provide diffs, which may be a bit more efficient, but may also introduce complexity.
Member
Author
There was a problem hiding this comment.
BugBot just pointed out the same thing that I may have just realized above: #8039 (comment) 😄
Member
Author
There was a problem hiding this comment.
Updated to expand cacheUpdated with a type property: c12bca9
FrederikBolding
force-pushed
the
fb/data-service-base
branch
from
019cd62 to
0d3c01d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
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
4 participants
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.
Explanation
This PR implements
BaseDataServiceand a function to wrapQueryClientto proxy requests accordingly.The
BaseDataService, similarly to theBaseControllerprovides the framework for building a service that can be registered and accessed via the messenger system, but also provides guarantees about per-request deduping, retries, caching, invalidation, state-while-revalidate etc via@tanstack/query-core.The
BaseDataServiceprovides two utilities for this:fetchQueryandfetchInfiniteQuery, which is similar but one is separated for special pagination behaviour. Each service has its own cache for the APIs that it exposes that must also be synchronized with the UI processes. To facilitate this synchronization, theBaseDataServicealso automatically provides acacheUpdateevent.The overall goal of the PR is to provide a base layer that can keep as much compatibility as possible with native TanStack Query while also simultaneously allowing us to have one source of truth per data service.
The synchronization is achieved via a special
QueryClientcreated bycreateUIQueryClient, which wraps functionality such as cache invalidation, provides the default proxied fetch behaviour and subscribes to cache updates from data services that it is observing (e.g. has active queries for).References
https://consensyssoftware.atlassian.net/browse/WPC-445
Checklist
Note
Medium Risk
Introduces new shared data-fetching/caching infrastructure (TanStack Query + messenger cache sync + service policy retries), which could impact cache coherence and invalidation behavior across processes. Risk is mitigated by extensive unit tests, but pagination and cache event handling are non-trivial.
Overview
Adds an initial
@metamask/base-data-servicepackage that provides aBaseDataServicewrapper around@tanstack/query-core, includingfetchQuery/fetchInfiniteQuery, a service-policy execution wrapper (retries/circuit breaking), a registered:invalidateQueriesaction, and automatic cache synchronization events (:cacheUpdatedplus per-query-hash granular events).Adds a new
@metamask/react-data-querypackage withcreateUIQueryClient, which proxies TanStack QueryqueryFncalls through the messenger to data service actions, subscribes/unsubscribes to granular cache update events based on active observers to hydrate/remove UI cache entries, and overridesinvalidateQueriesto also invalidate the underlying service.Cleans up template
greeterexports/tests, adds comprehensive nock-based tests + example service/mocks, updates changelogs, TypeScript project references, Yarn constraints (allow@tanstack/query-corerange mismatch and allow React peer deps), and lockfile/dependencies accordingly.Written by Cursor Bugbot for commit 1fdda05. This will update automatically on new commits. Configure here.