query icon indicating copy to clipboard operation
query copied to clipboard

`onMutate` never actually runs synchronously

Open sleexyz opened this issue 10 months ago • 4 comments

onMutate never actually runs synchronously because the result of #mutationCache.config.onMutate?.() is always awaited beforehand

Diff that fixes this:

diff --git a/node_modules/@tanstack/query-core/build/modern/mutation.js b/node_modules/@tanstack/query-core/build/modern/mutation.js
index 1bc7990..efbddd6 100644
--- a/node_modules/@tanstack/query-core/build/modern/mutation.js
+++ b/node_modules/@tanstack/query-core/build/modern/mutation.js
@@ -82,10 +82,12 @@ var Mutation = class extends Removable {
     try {
       if (!restored) {
         this.#dispatch({ type: "pending", variables, isPaused });
-        await this.#mutationCache.config.onMutate?.(
-          variables,
-          this
-        );
+        if (this.#mutationCache.config.onMutate) {
+          await this.#mutationCache.config.onMutate(
+            variables,
+            this
+          );
+        }
         const context = await this.options.onMutate?.(variables);
         if (context !== this.state.context) {
           this.#dispatch({

(Diff generated via patch-package to patch @tanstack/[email protected])

sleexyz avatar Mar 01 '25 04:03 sleexyz

We guarantee that onMutate runs synchronously, because it can in itself return a promise. We also await the result of it, it’s in your diff:

const context = await this.options.onMutate?.(variables);

So, no, I don’t think onMutate is meant to be syncronous.

TkDodo avatar Mar 03 '25 15:03 TkDodo

Sure, the side-effects of onMutate could be asynchronous and be awaited. But my expectation is that the handler fired synchronously, esp since it's used for optimistic updates.

For example, onMutate is currently unusable for optimistic updates to dndkit drag and drop state, bc the optimistic update fires asynchronously: https://stackoverflow.com/questions/79168785/react-query-with-dnd-kit-item-goes-back-to-original-position-for-a-split-second

On Mon, Mar 3, 2025 at 10:19 AM Dominik Dorfmeister < @.***> wrote:

We guarantee that onMutate runs synchronously, because it can in itself return a promise. We also await the result of it, it’s in your diff:

const context = await this.options.onMutate?.(variables);

So, no, I don’t think onMutate is meant to be syncronous.

— Reply to this email directly, view it on GitHub https://github.com/TanStack/query/issues/8724#issuecomment-2694742206, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALPSUNVXD7AMHHVJZRXGZT2SRXHBAVCNFSM6AAAAABYDVBFAOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJUG42DEMRQGY . You are receiving this because you authored the thread.Message ID: @.***> [image: TkDodo]TkDodo left a comment (TanStack/query#8724) https://github.com/TanStack/query/issues/8724#issuecomment-2694742206

We guarantee that onMutate runs synchronously, because it can in itself return a promise. We also await the result of it, it’s in your diff:

const context = await this.options.onMutate?.(variables);

So, no, I don’t think onMutate is meant to be syncronous.

— Reply to this email directly, view it on GitHub https://github.com/TanStack/query/issues/8724#issuecomment-2694742206, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALPSUNVXD7AMHHVJZRXGZT2SRXHBAVCNFSM6AAAAABYDVBFAOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMOJUG42DEMRQGY . You are receiving this because you authored the thread.Message ID: @.***>

sleexyz avatar Mar 03 '25 15:03 sleexyz

okay I think the change in the PR doesn’t hurt, so feel free to file it. However, this then means that the behaviour changes depending on if you have an onMutate defined on the mutationCache.

So I really wouldn’t rely on it being synchronous, as this would change as soon as you add onMutate on the cache.

TkDodo avatar Mar 05 '25 15:03 TkDodo

@sleexyz I'm running into the same issue here, what did you do to fix the optimistic updates so that the onMutate handler is fired synchronously?

ayush-goyal avatar Jun 02 '25 19:06 ayush-goyal