Infinite render loop with v4.29.{22,23}

[ddolcimascolo]

Describe the bug

Renovate is trying to update react-query from v4.29.19 to latest (this currently pulls v4.29.23) in our application monorepo containing multiple large React apps and a helper package. We have lots of tests entering infinite render loop, with the usual Maximum call stack exceeded error that React is throwing.

I can't give you more details yet, I'm opening the issue early to gather community feedback. I will be able to provide more information on Monday, but as I'm not expecting any breaking change in a patch release, I though it would be useful to keep you posted.

I'm not even sure this is a problem with react-query, it's just that this happens while upgrading this library.

Cheers,
David

Your minimal, reproducible example

To be done

Steps to reproduce

To be done

Expected behavior

Tests pass as with v4.29.19

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

Linux
JSDOM through Jest tests

Tanstack Query adapter

react-query

TanStack Query version

v4.29.23

TypeScript version

No response

Additional context

The problem is in unit tests only.

[iscekic]

I can confirm we're having a similar issue with next 13.4.10 and react-query 4.29.25 when using useInfiniteQuery.

Downgrading to 13.4.9 and 4.29.19 fixes the issue.

[TkDodo]

@iscekic can you show a reproduction please?

[TkDodo]

and, can you narrow down in which RQ version the issue first occurred ? We do one patch release per issue, so if you can tell me exactly which release between 4.29.19 and 4.29.25 is the first to break, I can look into it.

[iscekic]

I don't have a minimal repro, but I did manage to fix the issue using the latest version.

Seems like the culprit was the getNextPageParam fn, which didn't have a stable reference - adding useCallback fixed it.

[TkDodo]

react-query doesn't require getNextPageParam to have a stable reference though, so I'd still appreciate the reproduction

[ddolcimascolo]

Hi guys, same on my side after spending the morning trying to do a minimal reproduction in a codesandbox. We did have a non-stable function passed as an event handler in one of our components. Wrapping it in a useCallback fixed it.

I didn't manage to reproduce in isolation though, so I'd accept that the issue was on my side, though it's still strange that a patch release did introduce the behavior...

The Renovate MR is now merged, the issue can be closed I suppose. Thx for the help

[ddolcimascolo]

and, can you narrow down in which RQ version the issue first occurred ? We do one patch release per issue, so if you can tell me exactly which release between 4.29.19 and 4.29.25 is the first to break, I can look into it.

I've bissected and the culprit commit is 15dceab3 (PR #5573) introduced in 4.19.22

[TkDodo]

hmm, @incepter do you know why making getNextPageParam referentially stable would be required?

I didn't manage to reproduce in isolation though, so I'd accept that the issue was on my side, though it's still strange that a patch release did introduce the behavior...

@ddolcimascolo you're the second person to state that wrapping it in useCallback fixed the issue, so it does look like it's on our end. If that's the case, it should be reproducible in codesandbox

[incepter]

hmm, @incepter do you know why making getNextPageParam referentially stable would be required?

The commit didn't address the getNextPageParam direclty, and I was also unable to find anywhere where we compare previous and the new getNextPageParam

@ddolcimascolo you're the second person to state that wrapping it in useCallback fixed the issue, so it does look like it's on our end. If that's the case, it should be reproducible in codesandbox

I verified that tests are using inlined functions and are running correctly, a codesandbox reproduction will be highly appreciated

[ddolcimascolo]

hmm, @incepter do you know why making getNextPageParam referentially stable would be required?

I didn't manage to reproduce in isolation though, so I'd accept that the issue was on my side, though it's still strange that a patch release did introduce the behavior...

@ddolcimascolo you're the second person to state that wrapping it in useCallback fixed the issue, so it does look like it's on our end. If that's the case, it should be reproducible in codesandbox

Yeah, I've tried but no luck... I will try to again when I get some free time

[dawi]

We are (maybe) also running into this issue after upgrading from v4.29.19 to v4.29.25.

After upgrade, our playwright tests are running into timeouts in firefox. In chromium, everything still works fine.

The issues start if we use v4.29.21. Using v4.29.19 or v4.29.20 does not make any problems.

I also don't have a minimal repo at hand, but maybe this information can help to narrow down the cause of this issues a bit.

[TkDodo]

The issues start if we use v4.29.21. Using v4.29.19 or v4.29.20 does not make any problems.

hmm, there is nothing in .21 that should influence this. The potentially problematic fix was introduced in .22. Can you double check please @dawi ?

[incepter]

After upgrade, our playwright tests are running into timeouts in firefox. In chromium, everything still works fine.

That's intriguing

I also don't have a minimal repo at hand, but maybe this information can help to narrow down the cause of this issues a bit.

Can you please just copy a failing test and remove confidential parts, may be just the shape of the usage may hint us about the problematic.

Does the test pass when you wrap in a useCallback hook like what others suggested ?

[dawi]

@TkDodo You are right, sorry for the confusion, the problem starts with 22.

I also don't think that it is a firefox issue anymore. It just happens in firefox, because the tests in firefox are always a little bit slower, and together with the library update they are running in the timeout.

[lennartschoch]

We upgraded to version 23 yesterday and had to roll it back after it kept getting one of our apps into infinite render loops. I can confirm that the issue was the useInfiniteQuery hook, even when I removed our getNextPageParam function it kept re-setting the data field and as we were using that in a useEffect it caused a bunch of renders.

[GiftedMediaBJ]

Hi all,
We also run into this problem. We can conclude that the issue is introduced in 4.29.22
It is in every version above as well.

Our short-term solution was to remove 2 dependencies from the useEffect

[justin-kucerak]

I cannot speak to why wrapping getNextPageParam in useCallback affects this behavior.

However, I did notice that a deep comparison isn't being used in queryObserver line 837 which would unnecessarily return true, unless I am missing something. Seems like this could cause the QueryObserver class variable currentResult to change a lot which might explain the infinite loop.

[kevindice]

Same issue here

[TkDodo]

@GiftedMediaBJ @justin-kucerak @kevindice we're still waiting on either a standalone reproduction of the issue or a failing test case that we can run in isolation to debug the issue. Would be great if anyone could provide that please.

[Komoszek]

@TkDodo Here is my reproducible example https://codesandbox.io/s/confident-visvesvaraya-t8hzwk?file=/src/App.js. With "@tanstack/react-query": "4.29.22" infinite render loop happens and with "@tanstack/react-query": "4.29.19" it doesn't.

[sebelga]

I confirm that there is an infinite loop. Downgrading to 4.29.19 fixed the issue (I just had upgraded to 4.32.5).

[dawi]

In our application we don't get any infinite loop errors. The only issue is that our playwright tests are failing, because they run much slower. And I currently have no idea, how to tackle this issue.

with 4.29.19

with 4.32.5

[TkDodo]

@incepter I think it would be best if we reverted the PR that introduced the issue until we can find the root cause for it ?

[incepter]

@TkDodo of course go ahead, i ll look into it too.

[dawi]

No playwright test issues with this fix anymore. Thank you. :)

[sebelga]

Fix confirmed 👍 Thanks for handling this so quikly 🎉