multiple updates outside of react transition when using suspense

[joshribakoff-sm]

Describe the bug

My team is developing an app where we have an endpoint that returns mocked out random data on each request. This made a subtle bug in react-query super obvious to us.

There is non determinism where react-query will sometimes send the query multiple times. Also, if you have a useTransition hook (React concurrent mode), the useQuery is updating the consuming component outside of the transition which is bad as it blocks the main thread.

I suspect this is partly because strict mode in React 18 unmounts and mounts the component, see reactwg/react-18#19

With the release of React 18, StrictMode gets an additional behavior that we call “strict effects” mode. When strict effects are enabled, React intentionally double-invokes effects (mount -> unmount -> mount) for newly mounted components. Like other strict mode behaviors, React will only do this for development builds.

I think the bug in react-query can happen even without strict mode enabled. I read your source code and when your user sets cacheTime to 0, under the hood you guys set it to 1. For "stale time" you set it to 1000. https://github.com/tannerlinsley/react-query/blob/b44c213b39a258486aff719ba87a5f7d8c57fab4/src/react/useBaseQuery.ts#L60-L66 which was done in this PR #2821

This implementation under the hood is concerning and I suspect it is related to the bug. Depending on whether React takes more or less time than the hard coded cache/stale times you've selected for us, it determines how many times the query will run and whether it will re-run outside of startTransition.

Your minimal, reproducible example

https://github.com/joshribakoff-sm/rq-bug

Steps to reproduce

• Upgrade to react 18 rc npm i [email protected] --force, npm i [email protected] --force
• Install react-query and suppress peer dependency error npm i react-query --legacy-peer-deps
• Verify strict mode is on (default in create react app) src/index.js
• Wrap the app in query client with suspense mode const queryClient = new QueryClient({suspense: true}), <QueryClientProvider client={queryClient}><App /></QueryClientProvider>
• Create a simple app that loads random data in a query, and triggers the query within a react transition:

function App() {
  const [isPending, startTransition] = useTransition();
  const result = useQuery(
    "foo",
    async () => {
     console.log('query running')
      await new Promise((res) => setTimeout(res, 1000));
      return () => "foo" + Math.random();
    },
    { suspense: true }
  );
  console.log(result);
  return (
    <div className="App">
      {JSON.stringify(result)}
      <button
        onClick={() => {
          startTransition(() => {
            result.refetch();
          });
        }}
      >
        fetch
      </button>
    </div>
  );
}

Expected behavior

I expect the app to suspend or start a transition, and run the query once. I expect never to actually render with "stale" data.

Instead, the query itself runs multiple times even on the initial page load (not deterministic). Sometimes the component is rendered as many as 5+ times. In a more complex example you can observe the updates are happing outside of the transition, which can also be verified in React profiler.

How often does this bug happen?

Often

Screenshots or Videos

No response

Platform

Mac, chrome

react-query version

3.34

TypeScript version

No response

Additional context

Suspense support honestly seems a little half baked in react-query, I would suggest to label your support for it as experimental (not just that react suspense itself is experimental).

Can we get a way to set a "resource" object like in the react docs example? https://codesandbox.io/s/vigilant-feynman-kpjy8w?file=/src/index.js

The resource object is set into state, and passed down. Only the child that calls resource.read() suspends. This avoids the mind bending complexity of cacheTime/staleTime race conditions when the parent which starts the query inevitably suspends and re-mounts.

When suspense is enabled, you still return to us a bunch of things like isLoading and isStale which is useless as React itself handles keeping stale results on screen now (transition) and loading states (suspense), and this seems to confuse developers who sometimes mix approaches that are incompatible. I would propose an entirely new hook built from the ground up just for the new paradigm, without any bloat. This new hook would never throw, instead it would return the resource object which would itself throw when one calls resource.read() [after passing it down the tree]

[TkDodo]

I suspect this is partly because strict mode in React 18 unmounts and mounts the component

yes, strict effect might result in double request in dev mode only.

react 18 is still not officially released, and react-query does not yet have support for it. To make our store work with 18, we need to switch to useSyncExernalStorage. This has been done in this draft PR:

• Feature/react 18 #3064

Feel free to try out the codesandbox preview build (by depending on: "react-query": "https://pkg.csb.dev/tannerlinsley/react-query/commit/c2bef1ff/react-query") to see if that works better for you, I'd appreciate the feedback. Please note that it is based on v4 of react-query, which is alpha, so there are breaking changes. But you cannot expect a stable v3 that requires react17 to magically work with concurrent features of react18.

Suspense support honestly seems a little half baked in react-query, I would suggest to label your support for it as experimental (not just that react suspense itself is experimental).

The docs clearly state that:

NOTE: Suspense mode for React Query is experimental, same as Suspense for data fetching itself.

[joshribakoff-sm]

@TkDodo thanks for the quick reply, and thoughtful response

The docs clearly state that
NOTE: Suspense mode for React Query is experimental, same as Suspense for data fetching itself.

Yes that's fair, however the term "experimental" can mean different things, and it goes on to say:

should not be used in production unless you lock both your React and React Query versions to patch-level versions that are compatible with each other.

This is worded to suggest as long as I lock the versions, that things are "stable".

---

As for your linked PR about useSyncExternalStorage I'm not sure I understand how it is related, the PR linked does not have a description and the code changes are quite extensive.

---

strict effect might result in double request in dev mode only
might result in double request in dev mode only.

I suspect it's more than this, even without strict mode it happens. I just pushed another commit to my reproduction example removing strict mode, and the issue still persists.

Feel free to try out the codesandbox preview build (by depending on: "react-query": "https://pkg.csb.dev/tannerlinsley/react-query/commit/c2bef1ff/react-query") to see if that works better for you

The issue still happens, it should be fairly straightforward to clone my example and try whatever development versions you suspect may affect it, but I'm also happy to test it. What I don't get though is how we expect it to be "stable" with the mind bending issue of setting an arbitrary cache time. It seems to me that as long as it takes more than the hard coded delay in react-query, it will trigger the query again when the component mounts after suspending. It seems to me to be because of the hard coded delay plus the fact calling useQuery suspends instead of returning a resource that suspends when calling resource.read() (lower in the tree).

[joshribakoff-sm]

So I wanted to clarify I realized I misunderstood cacheTime and staleTime, but I read your blog post. The behavior I want is to only update data once, inside a transition. I tried setting staleTime to infinity, to tell it to "never consider my data stale". It didn't help. I have also tried to memoize the data, but it still updates numerous times

const variables = useMemo(() => {
    return {
      params: {
       // some application specific code here
      },
    };
  }, [validQueryParams]);

  const myQueryResult = useGetMyQuery(variables, {
    staleTime: Infinity,
    suspense: true
  });
  const myData = useMemo(() => {
    console.log('data changed,', JSON.stringify(myQueryResult.data));
    return myQueryResult.data;
  }, [JSON.stringify(myQueryResult.data)]);


  useEffect(() => {
    console.log('mount');
    return () => console.log('unmount');
  }, []);

I basically want to be able to click a button that changes the variables, have react query run and update the data once. No matter what I do it triggers the updates like 6x, unless I try to memoize the myQueryResult.data then it updates "only" twice.

If I turn suspense off, everything is fine but I lose the ability to show stale content during a transition. As soon as I turn on suspense the issue comes back, here is the order of the logs:

Initial load:

data changed
mount

Upon transition changing variables:

data changed
data changed

In fact Chrome devtools detects the JSON is unchanged and groups the logs together for me:

I hope this clarifies the issue better, thank you!

[TkDodo]

there is no need to JSON.stringify the data for the effect dependency, as data is stable:

  const myData = useMemo(() => {
    console.log('data changed,', JSON.stringify(myQueryResult.data));
    return myQueryResult.data;
  }, [myQueryResult.data]);

apart from logging, this doesn't do anything though.

If I turn suspense off, everything is fine but I lose the ability to show stale content during a transition.

That is what keepPreviousData: true is for.

I basically want to be able to click a button that changes the variables, have react query run and update the data once. No matter what I do it triggers the updates like 6x, unless I try to memoize the myQueryResult.data then it updates "only" twice.

you are mixing a lot of things in this issue, it's very hard for me to keep track. Is this now a suspense issue, or a memoization issue without suspense? A query runs as soon as it mounts. If you want to only run it when variables change, you need to initially disable it with the enabled prop. The data returned from react-query is stable and structurally shared, so as long as your queryFn doesn't return anything new, useMemo will not compute again. The component will re-render though because of the isFetching transition, unless you use tracked queries.

So please, let's start over, from zero. And please provide a codesandbox reproduction, that is so much easier to run for me than a repo. Thanks.

[joshribakoff-sm]

there is no need to JSON.stringify the data for the effect dependency, as data is stable

While I generally agree, the point here is that my component is mounting twice. Once when it initially suspends, and again when the data resolves. I added the stringify just to be really sure. Although the string passed to useMemo is equal, the memo call fires more than once.

That is what keepPreviousData: true is for.

Thanks, that's helpful. Ideally we'd like to use React transitions to keep the stale data on screen, as there are numerous other benefits.

you are mixing a lot of things in this issue, it's very hard for me to keep track. Is this now a suspense issue, or a memoization issue without suspense?

I don't know, to be honest. I've tried to make the example as minimal as possible, it's only a dozen or so lines of code. I created another example with just React, without react-query and there does seem to be some issue or confusion with React itself, I filed an issue here facebook/react#24155, perhaps the issue I'm seeing with react-query is related? I don't know.

A query runs as soon as it mounts.

I expect it to run 1x not 2x (or really 6x)

If you want to only run it when variables change, you need to initially disable it with the enabled prop

I don't want to disable the query from running on mount. I just want it to run initially 1x, and 1x when the variables change. I never want react-query to trigger an update on it's own (without wrapping in startTransition at least) just to tell me data is "stale" or deliver data that is referentially equal to the previous data, which is what it's doing now.

The data returned from react-query is stable and structurally shared, so as long as your queryFn doesn't return anything new, useMemo will not compute again

If that's the case, it's possible this is all due to an issue in React itself? I don't know. I expected it to be stable, but I'm confused on why my component is being updated 6x without memoization, and 2x with memoization. I expect it to only update 1x in both cases.

The component will re-render though because of the isFetching transition, unless you use tracked queries.

I guess this is the crux of my feedback. react-query is a really great library, but it seems to be doing a lot of updates related to things that aren't strictly needed with the new react, with suspense we have no use for the isFetching flag, so delivering us updates when all that changed was this flag is undesirable, it causes updates to happen outside of the transitions we changed the variables in, hence the attempt to overly memoize things to remove the superfluous updates.

Thanks for letting me know about tracked queries, that's something I was not aware of. Given that I can replicate similar issues in React itself, I'd speculate it's not going to help. I'm already memoizing the data part of the returned object, and that updates 2x. I guess this is most likely an issue in React itself.

So please, let's start over, from zero. And please provide a codesandbox reproduction, that is so much easier to run for me than a repo.

I'm happy to answer any questions, and my intent here is to be as helpful as possible, but I have already spent a lot of time creating a minimal reproduction as the issue template asked. I agree the issue is confusing, but I have made it as simple as possible. I've got a basic minimal query, and upon loading the page the query runs multiple times instead of once. I think it's reasonable to wait on your end until we rule out any bug in Reacts end here facebook/react#24155 -- if it turns out this is a bug in React, any discussion about react-query is obviously moot :) If having this issue open on your end for an experimental feature is not helpful, please feel free to close it. I'm grateful for the help you've provided and hopeful that things will work more smoothly in the near future as React team clarifies the new features more.

[TkDodo]

okay, I debugged your example a little bit. The issue arises also in react 17, so it has nothing to do with concurrent mode at least. It's also independent of cacheTime - it's because you set staleTime: 0.

Depending on whether React takes more or less time than the hard coded cache/stale times you've selected for us, it determines how many times the query will run

while what we do in the code might seem arbitrary, it is not. staleTime only starts to kick in after the query is done fetching, so even if you fetch takes 5 seconds, a staleTime of 1 second is just fine.

you can try that out by leaving out the staleTime parameter, and increase your setTimeout to 5 seconds - it will still only fetch one.

the default staleTime: 0 works in normal, non-suspend mode because we mount our observer, which kicks off the fetching, and it stays mounted. However, with suspense, the component mounts, and then unmounts, because the promise is thrown to the suspense boundary. Then, when the fetching finishes, the component mounts again. Due to refetchOnMount, you'll now get a refetch because the query data that you have is stale and, from the viewpoint of react-query, a new observer mounts. But it's the same one that we had before - it just got unmounted by react.

that's why we set the 1s staleTime. I think we just didn't account for the fact that someone might pass in staleTime: 0 manually, because that's the default anyways.

What we should do is change that code to take a minimum of 1second staleTime for suspense instead I think.

https://github.com/tannerlinsley/react-query/blob/808c80131ccd8353656e8b5ecfbc2198284aab67/src/reactjs/useBaseQuery.ts#L56-L62

[joshribakoff-sm]

It sounds like you've isolated the bug pretty good, thank you for doing that investigation!

staleTime only starts to kick in after the query is done fetching

I had assumed as much. I guess I was theorizing that maybe there is a race condition between the query becoming stale and how long it takes react to render (after the query is done fetching). I'm just guessing, though.

What we should do is change that code to take a minimum of 1second staleTime for suspense instead I think.

I see how you could have missed this because I was not super concise in my communication, but in this comment I had mentioned I tried changing staleTime to Infinity and the problem still occurs. It's possible I was mistaken.

Thank you again for looking into my issue report! Maybe another possible solution is disable refetchOnMount in suspense mode?

[TkDodo]

I see how you could have missed this because I was not super concise in my communication, but in #3432 (comment) I had mentioned I tried changing staleTime to Infinity and the problem still occurs. It's possible I was mistaken.

I just trie out your example repo, with react18 and react17, where staleTime is set to Infinity and it definitely only fetches once.

[joshribakoff-sm]

Yes, I can confirm the same upon double checking. However, I see the component is still updated multiple times outside of the transition, but if I add on the useMemo workaround I was referring to it seems to finally do what I want:

  const result = useQuery(
    "foo",
    mockFetcher,
    { suspense: true, staleTime: Infinity, cacheTime: 0 }
  );
  const data = useMemo(()=>{
    console.log('data changed', result.data)
    return result.data
  }, [result.data])

  console.log('result changed', result);

When i hit the button to do the React 18 transition, I get result changed 4x (unexpected), but I now get data changed only 1x, as expected. Still seems like there is behavior that is unexpected from my perspective, but I'll concede most of the issue was due to misunderstanding of staleTime

EDIT looks like I still get the data changed 2x unexpectedly in my real app. Let me narrow down the difference when comparing to the minimal reproduction example.

[joshribakoff-sm]

Ok I updated the example.

In my real project, I have variables being passed into the query, which are stored in React state inside the same component calling useQuery. When the component suspends, I also update the URL in my real app. This causes the variables to be recomputed, which delivers an update to my component again.

In the minimal reproduction example, I tried to create a similar bug by adding:

const [variables, setVariables] = useState(() => Math.random());

I guess because we're doing "fetch on render", each time the component suspends the state is lost. Therefore, the state initializer runs when it goes to render again after having previously suspended. If the state is initialized differently than the previous render that suspended, it triggers the side effect again, which suspends again. Since I've used Math.random() here you can see it loops infinitely. Pretty much exactly the issue I discussed with Dan here facebook/react#24155

They're saying that for now one of the only "supported" patterns is to create the promise above the suspense boundary, and store it in state (and not memo). Since we're creating it within the suspense boundary here, both state and memo are blown away due to how suspense works. If it were created above the suspense boundary, at least state should work I believe.

I guess one workaround could be to create my variables above the suspense boundary, but that doesn't seem ideal.

[joshribakoff]

It also worth mentioning in my real app these variables in the URL don’t actually change between the two renders, but since I consume the URL in useMemo and my memoization is lost when suspending, react-query is given variables object that is equal by value but not reference. In the real app it triggers “data changed” only 2x, as compared to the minimal repro which logs “data changed” infinitely, but in both cases the general idea is that “fetch on render” is error prone in general in react.

[joshribakoff-sm]

It is working now, the only action item on your end is to perhaps consider instructing your users to set staleTime to a high enough value (if not Infinity), and if they are going to pass variables into their query they'll likely need to construct those variables above the suspense boundary, which isn't too unreasonable. They'll also need to likely apply memoization to the data that is returned, or fiddle with the various settings of react-query to otherwise prevent updates when data has not changed. I also had to heavily memoize a lot of the components with React.memo, but I was able to get things stable with these workarounds.

Thanks for your help in narrowing this down!