Update BaseDataService to accommodate mutations, not just queries - #9324
Conversation
7e277f0 to
603d673
Compare
| 'mutationKey' | 'mutationFn' | ||
| >, | ||
| ): Promise<TData> { | ||
| const mutationCache = this.#queryClient.getMutationCache(); |
There was a problem hiding this comment.
QueryClient doesn't have a executeMutation method itself. I looked inside of the class, however, and saw that there was a getMutationCache method and followed where that led. This is not how useMutation works, which uses MutationObserver, but I don't know if that really matters. I figured it made more sense to mimic how fetchQuery works. But I'm not very familiar with TanStack Query, so if this is not the way we should be doing things, I'm happy to change this.
There was a problem hiding this comment.
I looked into this further and while this is not how useMutation works, it is how MutationObserver works. You can see that here:
So, we aren't really diverging from TanStack Query as much as I implied before.
| const mutation = mutationCache.build(this.#queryClient, { | ||
| ...options, | ||
| mutationFn: (context) => | ||
| this.#policy.execute(() => options.mutationFn(context)), |
There was a problem hiding this comment.
Hmm, are we safe to apply the policy as-is to mutations?
Just wondering if there could be a problem with accidentally doing double the mutation with retries 🤔
There was a problem hiding this comment.
Hmm, good point. We're still making an API request, so I feel like we would still want to have some kind of retry logic. But maybe it needs to be a little different than the logic used for GET requests.
I wonder if the default retry policy for createServicePolicy is too liberal. Right now, if the function you pass to the policy throws any error, then the function will get run again.
In RpcService we only retry connection errors, JSON parse errors, HTTP server errors (5xx), timeout errors, and "connection reset" errors. I wonder if BaseDataService should configure createServicePolicy such that it does the same thing?
Then I would be less worried here, because at that point, either we never make the request, or we do make the request but the server returns a 5xx. And in that case I feel like we ought to assume that the server is well-behaved, i.e. won't attempt to write to a database if it runs into an error. (If the server is not well-behaved, it shouldn't be our fault — the engineer writing the data service should know that and account for it.)
What do you think about that idea?
There was a problem hiding this comment.
Improving the defaults to be more "targeted" to BaseDataService makes sense to me. Though I still would be a bit worried that developers configure the service policy mainly for GET requests and don't realize how it may impact a PUT 🤔
There was a problem hiding this comment.
I wouldn't expect that most developers would configure the service policy, I would expect them to go with the defaults. But that's a good point. Should we have two kinds of service policies, one for queries and another for mutations? Then if a developer does configure a service policy, it should be more obvious what it's used for, and maybe it will cause them to think about it more critically. Or maybe this is solvable via documentation: we can add an "advanced" section to the tutorial/guide on data services that talks about the service policy, and we can remind the reader to consider non-GET requests when configuring it.
There was a problem hiding this comment.
Unsure if a separate service policy or modifications to the default service policy would be preferred. We could consider a default isServiceFailure function that changes depending on the type of request for example? But maybe a separate policy for maximum flexibility is preferred 🤔
There was a problem hiding this comment.
Thinking about this some more, I've see realized that it makes more sense to have one policy instead of two. All requests to an API should share one circuit and one counter to keep track of whether the circuit should break. Queries and mutations shouldn't be treated specially. I'll see if I figure out that path tomorrow.
There was a problem hiding this comment.
Perhaps it is enough to find a way to disable retrying for mutations?
There was a problem hiding this comment.
I guess that approach could work to get this PR merged, and then we could further refine it in another PR if we need to.
There was a problem hiding this comment.
I've disabled retrying for mutations by only using the circuit breaker policy to wrap the mutationFn instead of using the circuit breaker and retry policies.
|
Moving this PR back to draft. There's still more work to do here in order to properly support mutations. |
## Explanation <!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? * Are there any changes whose purpose might not obvious to those unfamiliar with the domain? * If your primary goal was to update one package but you found you had to update another one along the way, why did you do so? * If you had to upgrade a dependency, why did you do so? --> `createUIQueryClient` takes a messenger that is too broadly typed: it does not require that the actions and events that the messenger can access are actually scoped to the given data services. This was actually causing a type error in `createUIQueryClient.test.ts` — which replicates realistic usage of `createUIQueryClient` — but neither ESLint nor Jest caught it. This commit fixes the `MessengerAdapter` type so the type error goes away and adds a special test file we can run with `tsc` to ensure it doesn't pop up again. ## References <!-- Are there any issues that this pull request is tied to? Are there other links that reviewers should consult to understand these changes better? Are there client or consumer pull requests to adopt any breaking changes? For example: * Fixes #12345 * Related to #67890 --> This is blocking MetaMask#9324. https://consensyssoftware.atlassian.net/browse/WPC-1171 ## 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 - [x] 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) - [x] 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 - Extension PR: MetaMask/metamask-extension#44498 - Mobile PR: MetaMask/metamask-mobile#33450 <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Breaking TypeScript contract for `createUIQueryClient` and exported messenger adapter shapes may fail consumer builds until adapters are retyped; runtime query/invalidation behavior is largely unchanged. > > **Overview** > **`createUIQueryClient`** now takes a **readonly** data-service name tuple and a **`MessengerAdapter`** scoped to those names: `call` only accepts `` `${Service}:${string}` `` actions (with `unknown[]` params instead of `Json[]`), and `subscribe` / `unsubscribe` only accept granular `` `:cacheUpdated:${hash}` `` events. Runtime checks use the same name guards; query and invalidation paths no longer cast messenger arguments through `Json`. > > **`@metamask/base-data-service`** publicly exports **`DataServiceActions`** and **`DataServiceEvents`** so consumers can type messengers against data services. > > **Testing / repo hygiene:** `@metamask/react-data-query` adds **tstyche** (`createUIQueryClient.tst.ts`), Jest coverage for adapter proxying and invalidation forwarding, and build excludes `*.tst.ts`. **`yarn.config.cjs`** centralizes **`expectTestScripts`** so workspaces with `test:types` use `test:unit` + tstyche (messenger scripts renamed to match). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit fa890ae. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
5ce00ba to
ec84edc
Compare
|
I just realized that it's probably best if I wait until we upgrade |
ec84edc to
e9cba53
Compare
|
Almost done. Working through the tests to make sure that the way we test mutations is very similar to the way we've tested queries. |
9162ced to
4c370ee
Compare
Currently, `BaseDataService` has a `fetchQuery` method which is best
used for making read-only requests ("queries" in TanStack Query
parlance), but does not work as well for requests that change state on
the server side ("mutations"). For instance, queries are cacheable, but
mutations are not.
This commit adds a separate method, `executeMutation`, which
accommodates mutations better. Its implementation is different from
`fetchQuery` as it uses the mutation cache instead of the query cache.
Also, retries are disabled.
4c370ee to
eb74071
Compare
…efaultMutationOptions `createUIQueryClient` previously overrode `QueryClient.defaultMutationOptions` to inject a default `mutationFn` for data-service mutations, while queries got their default `queryFn` the ordinary way, through `defaultOptions.queries`. The asymmetry existed because, at the `@tanstack/query-core` version this package originally targeted (`^5.62.16`), a `mutationFn` received only `variables` and had no way to read its own `mutationKey`. The workaround closed over the per-call `defaultedOptions` to reach the `mutationKey`, which forced a fresh function per call, which in turn required a `WeakSet` so the `MutationCache.build` override could tell "our" mutation functions apart from engineer-supplied ones. query-core 5.89.0 added a `context` argument to `mutationFn` (mirroring the query context) that carries both `mutationKey` and `meta`. The code already relied on this context to read the `globalId` from `context.meta`, so the `^5.62.16` floor was already inaccurate. With the context available, a single shared `mutationFn` can serve every mutation the same way the default `queryFn` serves every query. This installs that shared `defaultDataServiceMutationFn` through `defaultOptions.mutations`, drops the `defaultMutationOptions` override and the `WeakSet`, and simplifies the `build` detection to a plain identity check against the shared function. Behavior is now symmetric with queries, including that a per-call `mutationFn` still wins. The `@tanstack/query-core` floor is raised to `^5.89.0` across all packages that declare it (the monorepo's constraints require a single consistent range), and `@tanstack/react-query` is raised to match in react-data-query.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
@FrederikBolding Ready for review again. |
## @metamask/base-data-service ## [2.1.0] ### Added - Add `executeMutation` protected method to `BaseDataService` to allow for making server-state-mutating requests ([#9324](#9324)) - These kinds of requests are never retried, unlike queries. - To use this, create a method in your data service class which takes whatever arguments you need, plus a optional final argument called `globalId`; then call `executeMutation` with a `mutationKey`, `globalId`, and `mutationFn`. See `ExampleDataService` in this package for an example. - A `MutationKey` type is also available. - Add protected `cancelQueries` so subclasses can abort in-flight reads before a forced refresh ([#10454](#10454)) ### Changed - The payload for `:cacheUpdated` and `:cacheUpdated:${hash}` events now includes an `objectType` property, which is either "query" or "mutation" ([#9324](#9324)) - Bump `@metamask/utils` from `^11.12.0` to `^12.0.0` ([#10192](#10192)) - Add `uuid` `^11.1.1` as a dependency ([#9324](#9324)) - Bump `@tanstack/query-core` from `^5.62.16` to `^5.89.0` ([#9324](#9324)) - Bump `cockatiel` from `^3.1.2` to `^3.2.1` ([#10436](#10436)) - Bump `lodash-es` from `^4.17.21` to `^4.18.1` ([#10447](#10447)) ## @metamask/money-account-api-data-service ## [2.0.0] ### Added - Add optional `fresh` option to `fetchPositions` that cancels in-flight reads, fetches with a zero stale time, invalidates the result for subsequent reads, and sends `Cache-Control: no-cache` so the Money API skips its Nest response cache when supported ([#10455](#10455)) - Accept optional additive `musd_balance_updated_at` on the positions `balance` summary ([#10455](#10455)) - Accept vault metadata on each position returned by `fetchPositions`: `chain_id`, `vault_key`, `name`, `asset_symbol`, and `asset_decimals` ([#10500](#10500)) - Accept multi-asset balance fields `by_asset` and `total_balance_usd` on the positions `balance` summary, and export `AssetBalance` ([#10500](#10500)) ### Changed - **BREAKING:** `effective_apy` on a vault position is now `string | null`. `null` means the position has been invested for fewer than 28 days, and is distinct from a rate of zero ([#10500](#10500)) - Bump `@metamask/base-data-service` from `^2.0.0` to `^2.1.0` - Bump `@metamask/utils` from `^11.12.0` to `^12.0.0` ([#10192](#10192)) - Bump `@tanstack/query-core` from `^5.62.16` to `^5.89.0` ([#9324](#9324)) ## @metamask/money-account-balance-service ## [3.1.0] ### Added - Surface Money API freshness on `fetchBalanceWithFallback` results (`asOfBlock`, `asOfTimestamp`, `dataFreshness`, `indexerLagSeconds`, `musdBalanceUpdatedAt`) when `source` is `api` ([#10455](#10455)) - Add optional `FetchBalanceWithFallbackOptions` (`minBlock`, `fresh`): when the API `as_of_block` is behind `minBlock`, throw `MoneyAccountBalanceStaleError` and fall back to RPC without reporting a defect; `minBlock` implies a fresh positions read, and explicit `fresh` is also forwarded to `fetchPositions` ([#10455](#10455)) - Export `MoneyAccountBalanceStaleError` ([#10455](#10455)) ### Changed - Bump `@metamask/money-account-api-data-service` from `^1.0.0` to `^2.0.0` - Bump `@metamask/base-data-service` from `^2.0.0` to `^2.1.0` - Bump `@metamask/utils` from `^11.12.0` to `^12.0.0` ([#10192](#10192)) - Bump `@ethersproject/providers` from `^5.7.0` to `^5.8.0` ([#10482](#10482)) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > The release bundles Money balance/positions API behavior changes including a breaking type change and post-tx staleness fallback logic, even though this PR only cuts versions and dependency ranges. > > **Overview** > **Monorepo release 1294.0.0** that cuts new package versions and aligns dependents—there are no application source edits in this diff, only `package.json`, changelog, and lockfile updates. > > **Version cuts:** `@metamask/base-data-service` **2.1.0**, `@metamask/money-account-api-data-service` **2.0.0**, and `@metamask/money-account-balance-service` **3.1.0**, with `[Unreleased]` notes moved into those release sections. The root monorepo version moves **1293.0.0 → 1294.0.0**. > > **Dependency propagation:** Every listed consumer bumps `@metamask/base-data-service` from `^2.0.0` to `^2.1.0`. `@metamask/money-account-balance-service` picks up `@metamask/money-account-api-data-service` **^2.0.0**; `@metamask/subscription-controller` also bumps `@metamask/money-account-balance-service` to **^3.1.0**. > > **What ships in those releases (via changelog, not new code here):** `BaseDataService` gains mutation/query-cancel plumbing and richer cache events; Money API positions support forced refresh, richer balance/position shapes, and a **breaking** `effective_apy: string | null`; balance fallback exposes API freshness metadata and optional `minBlock` / `fresh` behavior with `MoneyAccountBalanceStaleError`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ee833b1. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
## @metamask/base-data-service ## [2.1.0] ### Added - Add `executeMutation` protected method to `BaseDataService` to allow for making server-state-mutating requests ([MetaMask#9324](MetaMask#9324)) - These kinds of requests are never retried, unlike queries. - To use this, create a method in your data service class which takes whatever arguments you need, plus a optional final argument called `globalId`; then call `executeMutation` with a `mutationKey`, `globalId`, and `mutationFn`. See `ExampleDataService` in this package for an example. - A `MutationKey` type is also available. - Add protected `cancelQueries` so subclasses can abort in-flight reads before a forced refresh ([MetaMask#10454](MetaMask#10454)) ### Changed - The payload for `:cacheUpdated` and `:cacheUpdated:${hash}` events now includes an `objectType` property, which is either "query" or "mutation" ([MetaMask#9324](MetaMask#9324)) - Bump `@metamask/utils` from `^11.12.0` to `^12.0.0` ([MetaMask#10192](MetaMask#10192)) - Add `uuid` `^11.1.1` as a dependency ([MetaMask#9324](MetaMask#9324)) - Bump `@tanstack/query-core` from `^5.62.16` to `^5.89.0` ([MetaMask#9324](MetaMask#9324)) - Bump `cockatiel` from `^3.1.2` to `^3.2.1` ([MetaMask#10436](MetaMask#10436)) - Bump `lodash-es` from `^4.17.21` to `^4.18.1` ([MetaMask#10447](MetaMask#10447)) ## @metamask/money-account-api-data-service ## [2.0.0] ### Added - Add optional `fresh` option to `fetchPositions` that cancels in-flight reads, fetches with a zero stale time, invalidates the result for subsequent reads, and sends `Cache-Control: no-cache` so the Money API skips its Nest response cache when supported ([MetaMask#10455](MetaMask#10455)) - Accept optional additive `musd_balance_updated_at` on the positions `balance` summary ([MetaMask#10455](MetaMask#10455)) - Accept vault metadata on each position returned by `fetchPositions`: `chain_id`, `vault_key`, `name`, `asset_symbol`, and `asset_decimals` ([MetaMask#10500](MetaMask#10500)) - Accept multi-asset balance fields `by_asset` and `total_balance_usd` on the positions `balance` summary, and export `AssetBalance` ([MetaMask#10500](MetaMask#10500)) ### Changed - **BREAKING:** `effective_apy` on a vault position is now `string | null`. `null` means the position has been invested for fewer than 28 days, and is distinct from a rate of zero ([MetaMask#10500](MetaMask#10500)) - Bump `@metamask/base-data-service` from `^2.0.0` to `^2.1.0` - Bump `@metamask/utils` from `^11.12.0` to `^12.0.0` ([MetaMask#10192](MetaMask#10192)) - Bump `@tanstack/query-core` from `^5.62.16` to `^5.89.0` ([MetaMask#9324](MetaMask#9324)) ## @metamask/money-account-balance-service ## [3.1.0] ### Added - Surface Money API freshness on `fetchBalanceWithFallback` results (`asOfBlock`, `asOfTimestamp`, `dataFreshness`, `indexerLagSeconds`, `musdBalanceUpdatedAt`) when `source` is `api` ([MetaMask#10455](MetaMask#10455)) - Add optional `FetchBalanceWithFallbackOptions` (`minBlock`, `fresh`): when the API `as_of_block` is behind `minBlock`, throw `MoneyAccountBalanceStaleError` and fall back to RPC without reporting a defect; `minBlock` implies a fresh positions read, and explicit `fresh` is also forwarded to `fetchPositions` ([MetaMask#10455](MetaMask#10455)) - Export `MoneyAccountBalanceStaleError` ([MetaMask#10455](MetaMask#10455)) ### Changed - Bump `@metamask/money-account-api-data-service` from `^1.0.0` to `^2.0.0` - Bump `@metamask/base-data-service` from `^2.0.0` to `^2.1.0` - Bump `@metamask/utils` from `^11.12.0` to `^12.0.0` ([MetaMask#10192](MetaMask#10192)) - Bump `@ethersproject/providers` from `^5.7.0` to `^5.8.0` ([MetaMask#10482](MetaMask#10482)) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > The release bundles Money balance/positions API behavior changes including a breaking type change and post-tx staleness fallback logic, even though this PR only cuts versions and dependency ranges. > > **Overview** > **Monorepo release 1294.0.0** that cuts new package versions and aligns dependents—there are no application source edits in this diff, only `package.json`, changelog, and lockfile updates. > > **Version cuts:** `@metamask/base-data-service` **2.1.0**, `@metamask/money-account-api-data-service` **2.0.0**, and `@metamask/money-account-balance-service` **3.1.0**, with `[Unreleased]` notes moved into those release sections. The root monorepo version moves **1293.0.0 → 1294.0.0**. > > **Dependency propagation:** Every listed consumer bumps `@metamask/base-data-service` from `^2.0.0` to `^2.1.0`. `@metamask/money-account-balance-service` picks up `@metamask/money-account-api-data-service` **^2.0.0**; `@metamask/subscription-controller` also bumps `@metamask/money-account-balance-service` to **^3.1.0**. > > **What ships in those releases (via changelog, not new code here):** `BaseDataService` gains mutation/query-cancel plumbing and richer cache events; Money API positions support forced refresh, richer balance/position shapes, and a **breaking** `effective_apy: string | null`; balance fallback exposes API freshness metadata and optional `minBlock` / `fresh` behavior with `MoneyAccountBalanceStaleError`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit ee833b1. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Explanation
Currently,
BaseDataServicehas afetchQuerymethod which is best used for making read-only requests ("queries" in TanStack Query parlance), but does not work as well for requests that change state on the server side ("mutations"). For instance, it makes sense for queries to be cached and retried, but not so much for mutations.This commit adds a separate method,
executeMutation, which accommodates mutations better. Besides the differences mentioned above, it also uses the mutation cache instead of the query cache.References
Closes https://consensyssoftware.atlassian.net/browse/WPC-1118.
Manual testing
I've created #10176, which converts
AuthenticatedUserStorageService.setAssetsWatchlistto useexecuteMutationunder the hood, and MetaMask/metamask-mobile#36083, which loads those changes into the mobile app. You can check out the mobile branch and run through the manual testing steps there to confirm thatexecuteMutationstill exhibits the same basic behavior asfetchQuery.Checklist
Note
Medium Risk
Changes shared cache/event contracts (
objectType, mutation sync) and introduces server-mutating paths; incorrect retry or hydration behavior could cause duplicate writes or stale UI state, though mutations are explicitly non-retried.Overview
Adds mutation support alongside existing query flows in
BaseDataServiceand@metamask/react-data-query, so write requests no longer go through query-style caching and retries.BaseDataServicegains a protectedexecuteMutationAPI (plus exportedMutationKey) that runs through TanStack’s mutation cache, optional Superstruct validation viaprocessMutationResponse, and no retry policy (only the circuit breaker runs the request). Mutations can carry aglobalId(default UUID) inmetato correlate with the UI client.:cacheUpdatedpayloads now includeobjectType: 'query' | 'mutation', and mutation cache lifecycle is wired into persistence, dehydration, anddestroy()(including clearing mutation GC timers).react-data-queryexposesuseMutation(retries off by default), routes data-service mutations through the messenger with the UI’sglobalId, and syncs service-side mutation state viahydrateMutationsonupdatedcache events only (so idleaddedevents do not clobber settled UI results).@tanstack/query-core(and@tanstack/react-query) are bumped to^5.89.0across dependent packages;uuidis added where mutation IDs are minted.Reviewed by Cursor Bugbot for commit 2bc6446. Bugbot is set up for automated code reviews on this repo. Configure here.