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

[franleplant]

feel free to resolve this bug :)

[tannerlinsley]

I'll figure something out. It might not be as simple as rejecting it though. I'll have to add in a lot of checks for such an error and just to ignore it.

[franleplant]

I’ll also try to take a look

…

On Sun, 28 Jun 2020 at 18:12 Tanner Linsley ***@***.***> wrote: I'll figure something out. It might not be as simple as rejecting it though. I'll have to add in a lot of checks for such an error and just to ignore it. — 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/AAOEIJZNBYXGMYT7NHZFIETRY6W4ZANCNFSM4OJRDTZA> .

[tannerlinsley]

The original problem for this issue was resolved, so I'm going to close this. Feel free to open another one if you encounter issues for the other promise noop, or feel free to open a PR with any ideas to get rid of it.

[franleplant]

Thank you! Have a good one!

…

On Sun, 28 Jun 2020 at 20:00 Tanner Linsley ***@***.***> wrote: Closed #639 <#639>. — 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/AAOEIJ7QAN6P6IG3USYKTW3RY7DRFANCNFSM4OJRDTZA> .