Incorrectly works in Concurrent mode with Suspense

[vkurchatkin]

Describe the bug

When query key changes, all the subsequent queries become inactive immediately and never revalidate

To Reproduce
Steps to reproduce the behavior:

1. Go to https://codesandbox.io/s/dreamy-pond-5y7fd?file=/src/App.tsx
2. Open dev tools, there is one stale query (correct)
3. Press "Next"
4. There are two stale queries in dev tools (incorrect)

Expected behavior
There should be one inactive and one stale query

[flybayer]

We are experiencing this bug in Blitz apps too.

From my investigation, this broke in 2.7.0.

@tannerlinsley I know you are working on v3, so I'm curious if this might be fixed in 2.x or not?

[vkurchatkin]

To be quite honest, I took a look at useQuery implementation and it quite obviously violates numerous React rules which makes it completely incompatible with Concurrent Mode

[tannerlinsley]

Well, saying that it’s wrong without saying how it where is kinda useless, right? Can you offer a bit more context?

[vkurchatkin]

Sure, sorry for that. Here problematic parts with comments:

export function useBaseQuery<TResult, TError>(
  queryKey: QueryKey,
  config?: QueryConfig<TResult, TError>
): QueryResultBase<TResult, TError> {
  const [, rerender] = React.useReducer(c => c + 1, 0) // <-- This is technically not a problem on its own
                                                                                           // but is a good indicator of one. If rendering a component without
                                                                                           // changing anything (props or state) does something, that usually
                                                                                          // means that external mutable data is read in render, which is
                                                                                          // incompatible with CM
  const isMounted = useIsMounted()
  const cache = useQueryCache()
  const contextConfig = useContextConfig()
  const errorResetBoundary = useErrorResetBoundary()

  // Get resolved config
  const resolvedConfig = getResolvedQueryConfig(
    cache,
    queryKey,
    contextConfig,
    config
  )

  // Create query observer
  const observerRef = React.useRef<QueryObserver<TResult, TError>>()
  const firstRender = !observerRef.current
  const observer = observerRef.current || new QueryObserver(resolvedConfig)
  observerRef.current = observer

  // Subscribe to the observer
  React.useEffect(() => {
    errorResetBoundary.clearReset()
    return observer.subscribe(() => {
      if (isMounted()) {
        rerender()
      }
    })
  }, [isMounted, observer, rerender, errorResetBoundary])

  // Update config
  if (!firstRender) {
    observer.updateConfig(resolvedConfig) // <-- this is the main suspect, a side effect in render
  }

  const result = observer.getCurrentResult() // <-- if this reads from external mutable state, then it is problematic

  // Handle suspense
  if (resolvedConfig.suspense || resolvedConfig.useErrorBoundary) {
    const query = observer.getCurrentQuery() // <-- if this reads from external mutable state, then it is problematic

    if (
      result.isError &&
      !errorResetBoundary.isReset() &&
      query.state.throwInErrorBoundary
    ) {
      throw result.error
    }

    if (
      resolvedConfig.enabled &&
      resolvedConfig.suspense &&
      !result.isSuccess
    ) {
      errorResetBoundary.clearReset() // <-- a bunch of side effects, could cause trouble
      const unsubscribe = observer.subscribe() 
      throw observer.fetch().finally(unsubscribe)
    }
  }

  return result
}

Here is a bit about CM that Dan Abramov himself explained to me. In CM one component could be attempted to be rendered concurrently with different props or state. The most recent render doesn't necessarily represents something that is about to be on the screen, it might not be ever committed!

Dan called this "Old world" and "New world". When a component receives new props, React attempts to render the "New world" with this props applied. But it might not happen immediately, the render might be suspended. While this happens, the the "Old world" is still valid and is on the screen. User can interact with it in any way and can even make the pending render unnecessary.

What happens in useQuery is that observer.updateConfig(resolvedConfig) based on new props is called in render, which make the "Old world" invalid. The observer has been mutated too soon. While the new query is pending, the "Old world" is broken.

[vkurchatkin]

The solution to all of this is also simple: no mutation, useState instead.

This is what I have tried:

index fe217ca..488a495 100644
--- a/src/react/useBaseQuery.ts
+++ b/src/react/useBaseQuery.ts
@@ -12,7 +12,6 @@ export function useBaseQuery<TResult, TError>(
   queryKey: QueryKey,
   config?: QueryConfig<TResult, TError>
 ): QueryResultBase<TResult, TError> {
-  const [, rerender] = React.useReducer(c => c + 1, 0)
   const isMounted = useIsMounted()
   const cache = useQueryCache()
   const contextConfig = useContextConfig()
@@ -26,28 +25,25 @@ export function useBaseQuery<TResult, TError>(
     config
   )
 
+  React.useEffect(() => {
+    setObserver(new QueryObserver(resolvedConfig))
+  }, [resolvedConfig.queryHash])
+
   // Create query observer
-  const observerRef = React.useRef<QueryObserver<TResult, TError>>()
-  const firstRender = !observerRef.current
-  const observer = observerRef.current || new QueryObserver(resolvedConfig)
-  observerRef.current = observer
+  const [observer, setObserver] = React.useState<
+    QueryObserver<TResult, TError>
+  >(() => new QueryObserver(resolvedConfig))
+  const [result, setResult] = React.useState(() => observer.getCurrentResult())
 
   // Subscribe to the observer
   React.useEffect(() => {
     errorResetBoundary.clearReset()
     return observer.subscribe(() => {
       if (isMounted()) {
-        rerender()
+        setResult(observer.getCurrentResult())
       }
     })
-  }, [isMounted, observer, rerender, errorResetBoundary])
-
-  // Update config
-  if (!firstRender) {
-    observer.updateConfig(resolvedConfig)
-  }
-
-  const result = observer.getCurrentResult()
+  }, [isMounted, observer, errorResetBoundary])
 
   // Handle suspense
   if (resolvedConfig.suspense || resolvedConfig.useErrorBoundary) {

This fixes the problem in principle, but it is required to figure out all the cases, where new QueryObserver has to be created.

Main rules are:

• side effects only in useEffect/useLayoutEffect
• no reading directly from external mutable state, instead put it into useState, then React manages multiple versions correctly

[vkurchatkin]

Here is the issue with a relevant discussion: facebook/react#19473

[tannerlinsley]

Does this fix cause any tests to fail or any unexpected functionality?

[dburles]

I found this issue after experiencing something similar with seeing inactive queries, except simply where queries will mount inactive if they suspend. I can also reproduce this issue with @vkurchatkin's reproduction for this original issue.

To reproduce, refresh the page (within the sandbox) a few times, the query will fetch but be marked as inactive once the query has loaded. Clicking on the page will fire the focus refetch and then the query becomes stale (as it should have been initially).

It seems like it's a race condition as sometimes it's inactive, sometimes it's stale. This issue has certainly has caused some strange bugs.

[vkurchatkin]

Does this fix cause any tests to fail or any unexpected functionality?

@tannerlinsley I have not tried it, but this is definitely not a complete fix. This is the key bit:

 React.useEffect(() => {
    setObserver(new QueryObserver(resolvedConfig))
 }, [resolvedConfig.queryHash])

Instead of updating the observer we have to recreate it, but what deps to use? I've used queryHash in order to verify that it works when you change the query key, but it definitely needs more that.

[tannerlinsley]

Say we do this, and it becomes cm safe, how exactly would a suspense trigger know to use the latest configuration passed to the component?

[boschni]

Seems to be working correctly in V3: https://codesandbox.io/s/mutable-dream-7fubu

[vimcaw]

@boschni Yeah it is really working, but I still encounter a bug.

Please have a look at this demo: https://codesandbox.io/s/react-query-v3-bug-reproduction-b3b8u?file=/src/App.js

After you click the "Switch Key" button, that switches the query key, during data is fetching,  the data which return from useQuery is undefiend, and don't suspend the component, that will break much real-world code.

In suspense mode on react-query v2, we can safely use data without worrying about it being undefined:

function UserInfo({ id }) {
  const { data } = useQuery(`/user/${id}`);
  
  return <div>{data.name}</div>
}

I think the correct behavior should be: during fetching, either suspend the component or return the last requested result instead of undefined. It is best to provide some config to control which of the above two behaviors is wanted.

I fix this locally by applying a patch (using patch-package):

diff --git a/node_modules/react-query/es/react/useBaseQuery.js b/node_modules/react-query/es/react/useBaseQuery.js
index 7332b67..a393c7e 100644
--- a/node_modules/react-query/es/react/useBaseQuery.js
+++ b/node_modules/react-query/es/react/useBaseQuery.js
@@ -51,7 +51,7 @@ export function useBaseQuery(options, Observer) {
       throw currentResult.error;
     }
 
-    if (observer.options.suspense && !observer.hasListeners() && observer.willLoadOnMount()) {
+    if (observer.options.suspense && observer.willLoadOnMount()) {
       errorResetBoundary.clearReset();
       var unsubscribe = observer.subscribe();
       throw observer.refetch().finally(unsubscribe);

It will suspend the component during refetching after the query key changes, that is what I want.

[boschni]

Hi @vimcaw! Yes I think suspense should indeed also be triggered when switching to a new query. Fixed in 3.2.0-beta.22

[dburles]

Hey @boschni I was curious to see whether the inactive bug I mentioned earlier had also been fixed in the latest beta, however it seems that react-query-devtools is incompatible. Is there some other way I can dig into the cache?

[boschni]

Hi @dburles! Yes that should be fixed. The devtools still need to be updated but you can click the "Log" button in the previous playground I posted to check how many observers the queries have.

[boschni]

Is anyone still having trouble with suspense in v3?

[flybayer]

I haven't had a chance to try v3 yet