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
|
@metamaskbot publish-previews |
|
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
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
@metamaskbot publish-previews |
|
@metamaskbot publish-previews |
|
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}
| */ | ||
| const ALLOWED_INCONSISTENT_DEPENDENCIES = { | ||
| // '@metamask/json-rpc-engine': ['^9.0.3'], | ||
| '@tanstack/query-core': ['^4.43.0'], |
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).
| export default function greeter(name: string): string { | ||
| return `Hello, ${name}!`; | ||
| } | ||
| export * from './BaseDataService'; |
There was a problem hiding this comment.
What are your thoughts on not using export * and naming each export explicitly? We are considering banning these entirely, see here for more: MetaMask/eslint-config#331
| "@metamask/messenger": "^0.3.0", | ||
| "@metamask/utils": "^11.9.0", | ||
| "@tanstack/query-core": "^4.43.0", | ||
| "@tanstack/react-query": "^4.43.0", |
There was a problem hiding this comment.
It looks like react-query has a peer dependency on React 18.x or 19.x: https://github.com/TanStack/query/blob/67cf8b60d923ad158fdf89c80f86decea073f472/packages/react-query/package.json#L84. Do we want to declare this peer dependency here as well?
| // We provide re-exports of the underlying TanStack Query hooks with narrower types, | ||
| // removing `staleTime` and `queryFn` which aren't useful when using data services. | ||
|
|
||
| export function useQuery< |
There was a problem hiding this comment.
Should these hooks go in this package? Should we split this and createUIQueryClient off into their own UI-specific packages?
There was a problem hiding this comment.
Following up on the previous commit:
The advantage I can see for keeping UI-related code in this package is that all of the data service foundational stuff is in one place (although in that case perhaps we should rename the package to data-services to account for the scope?). The disadvantage is that the React dependency may not be completely obvious (and traditionally we have tried to keep stack-agnostic isolated, especially in core).
If we created a new package, here are some naming ideas:
@metamask/data-service-query-react@metamask/data-query-react@metamask/react-query-wrappers
|
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? |
|
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. |
|
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? |
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.
| // We provide re-exports of the underlying TanStack Query hooks with narrower types, | ||
| // removing `staleTime` and `queryFn` which aren't useful when using data services. | ||
|
|
||
| export function useQuery< |
There was a problem hiding this comment.
Following up on the previous commit:
The advantage I can see for keeping UI-related code in this package is that all of the data service foundational stuff is in one place (although in that case perhaps we should rename the package to data-services to account for the scope?). The disadvantage is that the React dependency may not be completely obvious (and traditionally we have tried to keep stack-agnostic isolated, especially in core).
If we created a new package, here are some naming ideas:
@metamask/data-service-query-react@metamask/data-query-react@metamask/react-query-wrappers
| }) { | ||
| this.name = name; | ||
|
|
||
| this.#messenger = messenger as unknown as Messenger< |
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); |
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.
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.
There was a problem hiding this comment.
BugBot just pointed out the same thing that I may have just realized above: #8039 (comment) 😄
| const { pages } = query.state.data as InfiniteData<TQueryFnData>; | ||
| const previous = options.getPreviousPageParam?.(pages[0], pages); | ||
|
|
||
| const direction = deepEqual(pageParam, previous) ? 'backward' : 'forward'; |
There was a problem hiding this comment.
If pageParam and previous are both undefined, then direction will be "backward". Is that a concern?
| }); | ||
| } | ||
|
|
||
| protected async fetchQuery< |
There was a problem hiding this comment.
What do you think about adding JSDoc summarizing why we are requiring queryKey and queryFn?
| return this.#client.fetchQuery(options); | ||
| } | ||
|
|
||
| protected async fetchInfiniteQuery< |
There was a problem hiding this comment.
What do you think about adding JSDoc that not only summarizes why we are requiring queryKey and queryFn but also how this is different from TanStack Query's version of fetchInfiniteQuery? I haven't been following the discussions very well so I admit that I don't have a firm grasp of what we have to do here to accommodate the existing architecture. If it takes more than three paragraphs to explain then maybe here is not the right place to document that knowledge, but maybe there are a few pointers you could provide at least.
| deepEqual(param, pageParam), | ||
| ); | ||
|
|
||
| return result.pages[pageIndex]; |
There was a problem hiding this comment.
Unguarded findIndex -1 returns undefined page data
Medium Severity
findIndex can return -1 if pageParam isn't found in result.pageParams, causing result.pages[-1] to silently return undefined instead of TData. This is especially likely when the initial fetchInfiniteQuery call stores undefined in pageParams[0] (TanStack's default for the first page) even though the actual fetch used a real pageParam via the wrapper on line 165. A subsequent call with that same pageParam enters the second branch, but findIndex won't match it against the stored undefined, returning -1.
| this.#broadcastCacheUpdate(event.query.queryKey); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Broadcasting on 'removed' dehydrates empty state, never propagates removal
Medium Severity
The cache listener in #setupCacheListener triggers #broadcastCacheUpdate on removed events. But #getDehydratedState calls dehydrate with a filter matching the just-removed query's hash — which no longer exists in the cache. This produces a DehydratedState with an empty queries array. When UI clients receive this via hydrate(client, payload.state), they hydrate an empty state, which is a no-op — so the removal is never actually propagated. UI clients retain stale cached data for queries that no longer exist on the service side.
Additional Locations (1)
| InfiniteData<TData> | ||
| >({ queryKey: options.queryKey }); | ||
|
|
||
| if (!query?.state.data || !pageParam) { |
There was a problem hiding this comment.
Falsy pageParam values incorrectly treated as absent
Medium Severity
The guard !pageParam in fetchInfiniteQuery uses a falsy check instead of an explicit undefined check. Since TPageParam extends Json, valid page params like 0, null, "", or false are all falsy but could be legitimate pagination values. These would incorrectly trigger the initial-fetch branch instead of the paginated-fetch branch, breaking pagination for any service using such page param types.
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.
@Gudahtt If we are talking what is essentially an HTTP client, no, I don't think there's any case where we wouldn't want to use BaseDataService, either. If we are talking about a data service that uses WebSockets or polling, then it's harder to say. I don't have a clear idea on how those patterns would work with TanStack Query. I mean, we haven't talked much about that lately, but do you have any thoughts on that? |
|
A query client would be even more useful with websockets or polling (request deduplication would be useful even more often in those cases, and is a bit trickier to implement by hand as well). Though I'm also not 100% sure what that would look like to implement, I haven't thought about it much yet. |
3 participants
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 a new caching/messaging abstraction built on TanStack Query that affects query invalidation, pagination, and cross-process cache synchronization; issues here could cause stale/incorrect UI data or missed invalidations.
Overview
Adds initial implementation of
@metamask/base-data-service, introducingBaseDataService(TanStack Query-backed query/infinite-query helpers plus a messenger:invalidateQueriesaction and:cacheUpdated/granular cache update events).Adds
createUIQueryClientto proxy UIQueryClientfetches through messenger actions, auto-subscribe/unsubscribe to per-query cache updates (hydrating local cache), and overrideinvalidateQueriesto also invalidate the backing data services.Exports the new APIs from
src/index.ts, adds typed wrapper hooks (useQuery,useInfiniteQuery), comprehensive Jest/Nock tests with anExampleDataService, and updates package deps/TS project refs/changelog (including a Yarn constraints exception for@tanstack/query-core).Written by Cursor Bugbot for commit 7f1fe7d. This will update automatically on new commits. Configure here.