[v3] Concurrent mode broken

[bbenezech]

Concurrent mode useQuery re-rendering too many times bug

To Reproduce
https://codesandbox.io/s/interesting-architecture-g4ojv
Press +1 a few times quickly and look at the number of renders.

Closely related to #1482

[boschni]

When I set staleTime to Infinity there is 1 render when using useState and 2 renders when using useDeferredValue. I don't know the specifics of useDeferredValue but this behavior seems ok to me?

[bbenezech]

Sorry I should have been more precise. Render count goes up to a thousand. Press the button twice with some delay between. Might require a few tries.

[bbenezech]

Updated codesandbox link, it was using v3.2.0 instead of latest react-query.

I humbly suggest you take your time with this bug, it looks really nasty. 😅

[boschni]

Maybe useDeferredValue should be use differently? This does seem to work: https://codesandbox.io/s/infallible-dewdney-9fkv9?file=/src/index.js . I'm not sure what the best practices are for concurrent mode as the feature is still experimental.

[bbenezech]

useDeferredValue works fine in my example.
But in some circumstances ReactQuery causes a massive number of rerenders that will bring real apps to their knees.
I am certain there is a bug in ReactQuery that must be fixed. IMO this is pretty serious.

[tannerlinsley]

Let’s keep in mind that CM is experimental still, so while it may be important and of high interest to those testing it out, it’s definitely not a serious issue or high priority.

I want to set expectations correctly that there is zero obligation for anyone, especially core contributors, to pour hours of work into this issue unless it is something that affects or interests you directly. We will continue to monitor this issue as CM solidifies and better patterns emerge around it.

[bbenezech]

@tannerlinsley to be clear when I say "must be fixed", I mean it solely from a "product" pov, as in "the bug comes from here and there is no work around", I do not imply any moral obligation for anyone here.
If I were, it would naturally include myself.
And when I say it is serious, I mean it looks like a design issue that will affect all users using concurrent mode, not an easy-fix around a particular use-case.
I am not a native speaker, I use a very direct style on purpose to prevent incomprehensions, mistakes and lengthinesses.
Sorry if I sound blunt.

[tannerlinsley]

You’re totally fine 👍 I triage conversations like this every day, so it’s always safer to set firm expectations, even if they are already being met :)

[boschni]

The reason why I'm suggesting that it might be because of how useDeferredValue is used, is that when the button is clicked twice in a certain timeframe, the deferred value seems to be alternating each render between the old value (0) and the new value (2). Which means that each render the query key is being changed, which is causing a re-render, which is then causing the query key to change, and so on. I don't know why the deferred value is alternating, but this should be out of the control of RQ. For now I can only guess that having useDeferredValue and a suspending useQuery on the same level might just not be a good combination. If the suspending hook is used a level deeper it does work properly.

[bbenezech]

Thanks @boschni ! My thoughts in random order:

• I think this use case (use of deferred value in the component it was created) is perfectly nominal. But maybe it triggers something else complicating the case ?
• I use the deferred value in my useQuery key deep in the tree and have the same issue : I get crazy re-renders when I switch key and useQuery triggers a suspense (not if the cache is hot since the partial v3.3.3 fix). The behavior was correct with 3.2.0
• If I remove the useQuery call or use a stable key, everything goes back to normal
• My uneducated guess is that it looks like useQuery re-renders with the "previous" key, somehow "invalidating" the work CR is doing, each time it switches to the other branch
• I had a very similar issue (my fault) when I left a component with deprecated lifecycle methods deep in the tree. It looks like React Concurrent can easily be put at fault.
• using keepPreviousData: true fixes the behaviour (no suspending when changing the key)

I think we could either try to revert the behaviour to latest working behaviour (3.2.0) by rollbacking/fixing the commit breaking it or we could try to understand further on what is happening inside React concurrent, maybe with some help of someone from react core ?

[tannerlinsley]

What's the status here?

[bbenezech]

@tannerlinsley https://codesandbox.io/s/wizardly-rubin-tx1hn
No changes AFAICT
Press +1 a few times, you'll get thousands of render very quickly.

[boschni]

Think I have narrowed it down to the following pattern: https://codesandbox.io/s/gallant-ritchie-o6wig?file=/src/index.js

Don't know the exact details, but my feeling is that React is rendering the component in multiple trees with different props. Because all trees share a single observer instance, the observer keeps on switching between queries and triggering state updates. This has probably been introduced by: #1449.

The main issue is that people expect the state returned from hooks like useQuery to immediately reflect the latest situation. Which means that if they pass in a different query key or enable/disable a query, they do not expect the state to be lagging behind. This is a reasonable expectation, but somewhat difficult to reconcile with the "renders should never have side effects" rule.

Currently the hooks are violating this rule because:

1. While rendering for the first time, a query is created in the cache if it didn't exist yet (=side effect).
2. While rendering, the observer options are updated and this triggers side effects like switching query, fetches and updating timers.

Either we'll need to find a clever way to work around the issue (like guessing what the next state would be), or we'll need to accept that the state can lag behind the options. Will dig into it a bit more.

[tannerlinsley]

Interesting advice from @gaearon here about "pure" rendering:

vercel/next.js#21932 (comment)