fix(react): Avoid using stale deps from closure when executing query in useQuery

[nathan-knight]

Summary

Currently if you change pause it does not recreate executeQuery so it ends up using stale values from deps. In order to ensure that it uses the freshest values without adding another case that can trigger re-renders or break memoization, I made it use state[2] instead with resolves the issue. If this is undesirable I can change it to just add args.pause as a dependency of the useCallback which I think will also fix the issue.

Set of changes

• useQuery now uses state[2] instead of deps within executeQuery

[kitten]

Could you provide a unit test or a sandbox/code sample of when this fails specifically please? I haven't worked on this for a while, but I don't really feel at ease with changing code that can affect a combination of React edge cases.

Not your fault of course, but basically, with strict mode, non-strict mode / older versions, pre- and post-concurrent having been added, and suspense, there's a lot of cases that I'd like to test, if you have a case that previously caused issues

[kitten]

I just found #1982 which does the exact and literal opposite change. So, I'm sorry, but I'll have to ask for a proper bug report here 😅 I want to avoid a situation where we fix one issue and introduce and regress an old one. There's also a lot of manual tests I'll have to do on old sandboxes, so while I don't doubt you had a specific issue in mind, I'll have to assume that this regresses on a separate issue for now

[nathan-knight]

I just found #1982 which does the exact and literal opposite change. So, I'm sorry, but I'll have to ask for a proper bug report here 😅 I want to avoid a situation where we fix one issue and introduce and regress an old one. There's also a lot of manual tests I'll have to do on old sandboxes, so while I don't doubt you had a specific issue in mind, I'll have to assume that this regresses on a separate issue for now

Ha I see, I'll consider an alternative strategy that doesn't regress on the past issue (but I'll need to figure out how to reproduce the old issue first).

Here's a sample that reproduces the bug I was encountering: https://codesandbox.io/p/sandbox/ecstatic-clarke-tyrf3m Steps:

1. Unpause it
2. Refetching will no longer refetch

[kitten]

I'll have to look at this tomorrow in more detail.

Looking at the change, I don't actually know anymore why the change over from state[2] to deps works.

I also don't know why its dependencies contain getSnapshot still, which is obsolete.

Overall, your change should be correct, although, I'm not sure whether state[2] also then needs to go into deps. Basically, the thinking is, the intention of the code is to bypass the in-render setState via executeQuery, hence, state[2] should be correct, as long as we also update the dependencies.

We could then just test against the old reproduction in the old issue. But I agree that your change back to the old implementation on the surface makes sense. I'm just not sure if the actual issue is that useCallback's dependencies array is wrong, in the current implementation, in the one before, and in this PR still 🤔

I'd have to test all changes against the reproduction of yours and the old issue, I suppose

[kitten]

This is superseded by #3323. I tested this exact change against this PR's and the prior issue's reproduction and am still testing it against some older edge cases.