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.
| #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.
mcmire
reviewed
Mar 20, 2026
Contributor
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Getting closer on this. I admit I'm not following the code in createUIQueryClient. The tests do help in demonstrating the behavior, but I feel like I need to read about observers to fully grasp it. That said, overall, I don't have any major concerns, just minor things. I'll try to do a final pass next week.
| name: ServiceName; | ||
| messenger: ServiceMessenger; | ||
| queryClientConfig?: QueryClientConfig; | ||
| servicePolicyOptions?: CreateServicePolicyOptions; |
Contributor
There was a problem hiding this comment.
Nit: One could argue this is the service, so we could drop the service prefix if we like? Unless you think "policy" is too generic a term.
Suggested change
| servicePolicyOptions?: CreateServicePolicyOptions; | |
| policyOptions?: CreateServicePolicyOptions; |
| * Prepares the service for garbage collection. This should be extended | ||
| * by any subclasses to clean up any additional connections or events. | ||
| */ | ||
| protected destroy(): void { |
Contributor
There was a problem hiding this comment.
Should we make this public by default?
Suggested change
| protected destroy(): void { | |
| destroy(): void { |
Comment on lines
+217
to
+220
| beforeEach(() => { | ||
| cleanAll(); | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
Nit: This should already be happening here:
Suggested change
| beforeEach(() => { | |
| cleanAll(); | |
| }); |
| mockAssets({ status: 500 }); | ||
|
|
||
| await expect(service.getAssets(MOCK_ASSETS)).rejects.toThrow( | ||
| 'invalid json response body', |
Contributor
There was a problem hiding this comment.
Should we mock the endpoint to return valid JSON instead? Usually in production code we might see an HttpError if retries are exhausted.
| it('fetches using observers', async () => { | ||
| const { clientA, clientB } = createClients(); | ||
|
|
||
| const observerA = new QueryObserver(clientA, { |
Contributor
There was a problem hiding this comment.
Wow, there's all kinds of tools in TanStack Query, isn't there 👀 👀
| }); | ||
|
|
||
| await expect(promise).rejects.toThrow( | ||
| 'Queries must use data service actions.', |
Contributor
There was a problem hiding this comment.
Should we put something in the error message that instructs engineers on how to do this?
| action, | ||
| ...(options.queryKey.slice(1) as Json[]), | ||
| options.pageParam, | ||
| )) as Json; |
Contributor
There was a problem hiding this comment.
Why does this have to be Json? Or, better question, where are we defining this requirement?
| "peerDependencies": { | ||
| "react": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", | ||
| "react-dom": "^16.8.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", | ||
| "react-native": "*" |
Contributor
There was a problem hiding this comment.
Is it worth targeting a minimum version?
| } | ||
|
|
||
| /** | ||
| * Expect that if a non-workspace package lists another package in its |
Contributor
There was a problem hiding this comment.
I'm not sure why I didn't restrict this check to just workspace packages. This comment also doesn't seem right 🤔 Anyway, your fix seems fine 😂
7 tasks
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-service/query caching abstractions and cache-sync behavior across processes, which could impact data freshness and invalidation semantics if adopted broadly. Also changes dependency/constraints configuration (TanStack v4 allowance and peer-dep exceptions) that may affect monorepo builds.
Overview
Adds an initial
@metamask/base-data-servicepackage implementingBaseDataService, wrapping@tanstack/query-corewith service-policy retries/circuit breaking,fetchQuery/fetchInfiniteQuery, messenger-basedinvalidateQueries, and automatic:cacheUpdated/granular cache update events.Adds
@metamask/react-data-querywithcreateUIQueryClientto proxy TanStack Query requests through messenger actions, subscribe/unsubscribe to per-query cache update events to hydrate/remove UI cache entries, and overrideinvalidateQueriesto also invalidate the backing data service; also exports typeduseQuery/useInfiniteQuerywrappers. Updates tests/mocks, TypeScript project references, changelogs, Yarn constraints (allow@tanstack/query-core@^4.43.0), and permits React peer dependencies without requiring dev installs.Written by Cursor Bugbot for commit e0a7d03. This will update automatically on new commits. Configure here.