useMutation's onSuccess promise never resolves due to setQueryData and refetchQuery edge case

[franleplant]

Describe the bug

I have created a very contrived example reproduction code that you can find here:
https://github.com/franleplant/react-query-unresolved-promises/blob/master/src/App.tsx

I say contrived because it doesn't represent real use case but I first found this error
in a closed source company app where, as you probably know, things started to get messy
and there were some interactions between mutation's onSuccess and refetchQueries.

To be a bit more concrete, when we save EntityA we refetch EntityA (because of the way our particular backend works) but since we weren't using exact: true it was causing it to refetch EntityA1 via a query that wasn't initialized, so by a semi complex interactions of the query of EntityA we also did setQueryData of EntityA1 which caused the problem I explain below.

(Sorry, it ended up not being that much more concrete)

I found this bug in react-query 1.x but I have noticed the same code in 2.x

Expected behavior

A mutation promise always resolves to something, either a value or an error.

Desktop (please complete the following information):

• OS: macos
• Browser all

Additional context

I have isolated the bug to an edge case interaction between queryCache.setQueryData and queryCache.refetchQuery

(extracted from the repro repo)

export function useSomeMutation() {
  return useMutation<void, undefined>(
    async () => {
      await delay();
    },
    {
      onSuccess: async () => {
        console.log("onSuccess start");
        queryCache.setQueryData("notInstantiated", "hello"); //BANG
        queryCache.setQueryData("notInstantiated2", "hello"); // BANG
        const a = queryCache.refetchQueries("notInstantiated", { force: true });
        const b = queryCache.refetchQueries("notInstantiated2", {
          force: true,
        });
        console.log("promises are never resolved", a, b);
        await Promise.all([a, b]);
        // This never reaches
        console.log("onSuccess end");
      },
    }
  );
}

Whenever you try to setQueryData on a query that hasn't been instantiated react-query gives the queryFn a value of new Promise(noop) which is a promise that never resolves.
So when you later do refetchQuery of that same query you get a promise that never returns and since useMutation waits for the onSuccess async function to resolve then it never resolves.

We are virtually saying this

onSuccess: () => new Promise(noop)

• check the 1.x code for setQueryData
• check the 1.x code for refetchQuery

This same default value of new Promise(noop) is used in 2.x branch.

for lib consumers: A workaround is to check that the query that you are trying to setData on has actually an instance

// check that there are current instances of `myQueryKey`
// i.e. some component has used it
if (queryCache.getQuery(myQueryKey)) {
  queryCache.setQueryData(myQueryKey, someData)
}

But I wander why we are giving a default value of a promise that never resolves. Can you we change it to Promise.resolve() ? It seems that the functionality will be maintained but in these sort of edge cases we wont get a promise that never resolves.

Let me know what you think @tannerlinsley and I can definitively help with this in both 1.x and 2.x branches.

[tannerlinsley]

Here maybe? https://github.com/tannerlinsley/react-query/blob/master/src/core/queryCache.js

[franleplant]

@tannerlinsley you mean here https://github.com/tannerlinsley/react-query/blob/master/src/core/queryCache.js#L226 ? for the 2.x branch (master) ?

[tannerlinsley]

Sorry, yes... no idea why the line numbers didn't paste there. We need to fix that line to not be a promise, but just useQuery(key, noop, ...)

[franleplant]

Right, yeah, since we are using await it seems that we might not need to force it to be a promise. I will try to make this patch tomorrow. Is it ok to make a PR for the 1.x branch too? We are still using it and will be for a while.

…

On Fri, 26 Jun 2020 at 21:55 Tanner Linsley ***@***.***> wrote: Sorry, yes... no idea why the line numbers didn't paste there. We need to fix that line to not be a promise, but just useQuery(key, noop, ...) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#639 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOEIJ4QXUJU7JLB2AE3ZKTRYU7RFANCNFSM4OJRDTZA> .

[tannerlinsley]

Yep if you can backport the fix that would be fine.

[franleplant]

@tannerlinsley done with two PRs sir.

I noticed we are using new Promise(noop) in one more place in both master and 1.x branches https://github.com/tannerlinsley/react-query/blob/master/src/core/queryCache.js#L504 , we should probably avoid creating never resolving promises altogether, tried to quickly fix it but a test started to fail. If you agree I can maybe submit a patch next week?

[tannerlinsley]

That one is actually good. It’s so await chains do not proceed for outdated queries.

[franleplant]

shouldn't that be a `Promise.reject("outdated query, do not follow")` type of error instead? Having never resolving promises seem dangerous way of flow control.

…

On Sun, Jun 28, 2020 at 12:27 AM Tanner Linsley ***@***.***> wrote: That one is actually good. It’s so await chains do not proceed for outdated queries. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#639 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOEIJYSYGZPZL7LND47O23RY22BRANCNFSM4OJRDTZA> .

[tannerlinsley]

Yeah we could do that. We could improve the cancelled error object and use that.

[tannerlinsley]

I've messed around with this a bit, and I think I'm still in the camp that the retry promise that doesn't fulfill from the document not being visibile should be a noop promise (so side-effects from the promise don't run at all). I'm still 100% on board with the other noops getting resolved like you've done though.

[franleplant]

Just a heads up so that you have them in mind

• noop promise is not a thing I've seen anywhere else, promises should resolve or reject some value, semantically the pending state should be a transitory state not a permanent state
• this is an async chain blocking statement, any place in your lib code or on a consumer app that somebody says await noopPromise will get forever blocked, which is counter intuitive and difficult to debug
• there might be performance concerns regarding having n promises that never resolve, we would need to investigate this further