Bug: useEffect not triggering on [deps] change

[Tosheen]

I stumbled upon this issue which initially I thought is React bug, but upon further investigation React Query seems like the next best candidate. Minimal project is created to demonstrate the problem. In card-status.js I registered useEffect which is watching for certain changes and it just displays the message whenever [deps] change. The problem is that one state change gets swallowed by useEffect. It can be seen if you click on a button and watch the console, useEffect is not triggered even though UI updates correctly aka rerender is triggered. I have found that the issue is caused by line 60 in the same file.

if that line is commented out, then useEffect triggers properly when button is clicked. Also if setState on line 60 is called with number for example then it also behaves correctly.

Link to code example:

codesandbox: https://codesandbox.io/s/frosty-pateu-bh0ul github repo: git@github.com:Tosheen/use-effect-bug.git

To Reproduce Steps to reproduce the behavior:

1. Open the console and click on the activate trial button.
2. useEffect message is not visible upon clicking, even though [dependency] changed and UI is rerendered.

Expected behavior useEffect should register new changes, since every other part of the UI does

Desktop (please complete the following information):

• Windows 10
• Browser: Chrome
• Version: 92.0.4515.159

Additional context React Query version: 3.21.1

[lgenzelis]

Wow, I've just spent half an our going over your example, and I cannot get what's going on. At this point I'd guess you found a bug on react, but it seems crazy. I definitely wanna be a part of this conversation 😮

[lgenzelis]

@Tosheen I made a fairly simpler version of your codesandbox that exhibits the same bug, in hope to get more people interested in this. I'm honestly perplexed.

Btw, I tried downloading the repo to my machine and building for production, but the bug is still there.

[omaralsoudanii]

in mutation returning Promise is async, the docs says to use mutateAsync.

the line you suggested I believe re-renders the component, but the hook already changed the state while the re-render happens. Also I noticed that the hook it self is inconsistent.

you are returning a status field, or a status field with another 2 fields

using async: https://codesandbox.io/s/inspiring-ride-cj35s

without changing anything except the dependency in useEffect (will work because of the mentioned above about inconsistency)

https://codesandbox.io/s/compassionate-currying-wr9g0

relevant docs: https://react-query.tanstack.com/guides/mutations#promises

I generally prefer to refactor your code and split the mutation from the useQuery() hook

[Tosheen]

@lgenzelis Yeah your example is definitely easier to follow. Thanks for that. Hopefully we will get to the bottom of this.

@omaralsoudanii About inconsistency, original hook is using typescript and so called Algebraic Data types, it sounds fancy or complex but in essence depending on the status flag I know exactly which options are available thanks to typescript and I dont see any problems with that. Probably hook wont be that good when it comes to publishing it as a package, but for internal usage it works great.

[Tosheen]

Also why would setCounter(null) or setDummy(0) trigger a rerender if the actual state hasnt changed? New state and initial state are the same so the theory about rerendering while the hook changed doesnt seem that possible, even though it could be due to the bug :)

[omaralsoudanii]

@Tosheen So did you check the examples I provided? Or did you stop when you read inconsistent?

I won't argue about the hook, but you can see in both links how the behaviour changed.

Regarding TS union types, I am not finding where I mentioned that, inconsistent is returning a promise in the useQuery hook.

And returning a regular object from the hook itself.

Otherwise what's your explanation for how it worked when I changed the dependency from status to the fancy complex word that you implied that ofcourse I don't know about it, the useEffect hook worked.

Actually ignore it, my bad for helping.

[Tosheen]

@omaralsoudanii I read your comment in detail, but the thing is there is nothing async in the example I provided nor in Igenzelis example so I dont think its relevant. I could be wrong of course. This just seems like a bug in general rather than improper usage.

I appreciate the help though.

[omaralsoudanii]

@Tosheen what i meant while it's not async using promise in mutation you need to use the function provided in the docs, the behaviour is mentioned there is similar to this.

This sections: https://react-query.tanstack.com/guides/mutations#mutation-side-effects https://react-query.tanstack.com/guides/mutations#promises

From the docs: When returning a promise in any of the callback functions it will first be awaited before the next callback is called

You might find that you want to trigger additional callbacks than the ones defined on useMutation when calling mutate. This can be used to trigger component-specific side effects. To do that, you can provide any of the same callback options to the mutate function after your mutation variable. Supported overrides include: onSuccess, onError and onSettled. Please keep in mind that those additional callbacks won't run if your component unmounts before the mutation finishes.

Use mutateAsync instead of mutate to get a promise which will resolve on success or throw on an error. This can for example be used to compose side effects.

I did exactly that in one of the examples and it worked. I think it boils down to this:

Please keep in mind that those additional callbacks won't run if your component unmounts before the mutation finishes.

[Tosheen]

@omaralsoudanii Exactly and I fully agree, in case I want to perform component specific use cases, like if mutation was successful then do some other things like fire analytics events etc, those events should not pollute the hook in any way. But what if I have a state which is hook specific, aka its available for every call site and which is independent from react query? In that case we are still left with this bug. Again I could be wrong, and there are many solutions to this problem, you have also provided a solution.

I am just curious to see if this is the actual bug or not.

[TkDodo]

is this similar to / related to this issue?

https://github.com/tannerlinsley/react-query/issues/1535

oddly enough, it also works in your example if you switch the order in onSuccess: The effects trigger correctly if you call setDummy(0) first 🤯

    onSuccess: (result) => {
      setDummy(0)
      queryClient.setQueryData("message-upgrade", result);
    }

[Tosheen]

I am not sure, re-render does happen cause its reflected in UI, but useEffect doesnt register new changes. Probably it is due to some optimizations but who knows. I couldnt reproduce this bug without react query though.

[omaralsoudanii]

@Tosheen The issue is using the setState function inside the onSuccess callback, if I understood what you're saying correctly then you want this functionality:

1- a custom hook where you can import from anywhere, which is independent from react query 2- however it does use react query to do some stuff 3- and you would like to have a react state to fire another events irrelevant to react query 4- and this state would be tied to the UI since you are adding it as a dependency in useEffect?

If this is correct, then useState needs to be in the upper level ( the caller of the custom hook) and do other logic based on what returned from the custom hook? and in useEffect you need to bind both status values from:

1- the local useState 2- the custom hook returned value

check this out if it's what you meant (I just moved useState to the render function and added both as dependencies):

https://codesandbox.io/s/snowy-water-mjwrb

there is also this from the docs: https://react-query.tanstack.com/examples/custom-hooks

[Tosheen]

@omaralsoudanii Yeah this could also work, its probably like a 4th workaround this bug :). Thanks

[TkDodo]

@omaralsoudanii I don't understand that last example. Why would you want to call setStatus in the useEffect? It will just make sure that status in state is the same as status returned from the custom hook, so it's superfluous, isn't it?

one use-case that I'm seeing for calling setState to a local state from a mutation / query is because you want to control certain things, like staleTime or refetchInterval from the response of your query, so you have to "sync" it back to local state:

const [ staleTime, setStaleTime] = useState(0)
useQuery(key, fn, { staleTime, onSuccess: data => setStaleTime(data.validUntil) }

I find it very weird that this seems to affect the triggering of useEffects, depending on if you call setState with the value it already contains or not, and apparently also the ordering seems to be important?

[omaralsoudanii]

@TkDodo

That's what I've been trying to say, my example is redundant and pointless. It's just to prove that there is some race condition with useEffect and the custom hook.

You said your self the ordering seems important, in fact take the original example and swap the setState to be before mutate and the example will work.

Remove the whole local state from code and it works.

I suspect that the logic of react query inside the custom hook is not in sync with the component itself ( otherwise how the UI changes but useEffect doesn't trigger?)

When I swapped mutate with mutateAsync then useEffect triggered.

The thing is if you carefully look at useQuery and useMessagingUpgrade you find 2 different types.

useQuery returns Promise.resolve for status value

useMessagingUpgrade returns a regular object for status

useMessagingUpgrade returns Promise.resolve incase of mutation

UseMessagingUpgrade returns a regular object after the mutation happens.

Promise.resolve is async Regular object is sync

Go ahead and enable the debugger locally and you will see that the local state is not in sync with the custom hook due to mixing sync and async returns.

My previous example using mutateAsync triggered useEffect (but it is still not the correct solution)

The last example your confused about, I made it on purpose to prove that the custom hook is not in sync with the component, by using setState inside useEffect I am forcing it to be a dependency.

Even if you see my previous comment the docs state the same thing.

[lgenzelis]

I'm 99% sure the bug is in react rather than in react-query. If the bug were that the component is not re-rendering, I would bet for react-query. But here the component does re-render, but useEffect doesn't run, even though its dependencies change.

Reading through the comments, I feel we're tackling this the wrong way. This has nothing to do with custom hooks or with mutate vs mutateAsync. To demonstrate, here is an even simpler version of the bug, without using mutations at all. Click the Rerender button repeatedly, and you'll see the bug in action.

The problem is that I can't replicate this issue without using react-query. I cloned the repo and tried to understand what setQueryData is exactly doing, but no luck so far 😓 If we can surgically extract the piece of related code and still replicate the issue, we may open a ticket in the repo for react.

tl;dr

Please check this sandbox

[omaralsoudanii]

@TkDodo @lgenzelis @Tosheen

I give up, solve this mystery ( used the sandbox from @lgenzelis ) useEffect runs again when you re-focus window ONLY when the value is true.

Check this recording, and pay attention to the status value from clicking, and the console when refocus based on the status value.

https://user-images.githubusercontent.com/7079173/132187687-db0bfcf5-cfb1-4b51-b121-17009c46f5b4.mp4

Also the note here could be relevant: https://reactjs.org/docs/hooks-reference.html#conditionally-firing-an-effect

[Tosheen]

@lgenzelis I am trying to replicate the bug on your latest example but it seems that effect function is triggering correctly on every button click.

What am I missing? Anyhow your approach seems most reasonable.

[lgenzelis]

@omaralsoudanii unluckily I'm too intrigued to give up at this point! 😂

@Tosheen those logs correspond to the rerenders (which always happen correctly). But useEffect is not running after any of the renders, that's the bug. Comment out setDummy(0); and you'll note the difference 🙂

[mckelveygreg]

So this was happening with us as well.
I was not able to narrow it down more than if our user query was used in a React Query mutation, then the user query could update once, but after it was invalidated and the user was signed out (no jwt, so the user query would return null), then the user query would no longer trigger any useEffect deps!

We are pretty stumped 🤷

[codal-hkrishnani]

Any update on this one, in my case sometimes useEffect is triggering sometimes its not.

[TkDodo]

so the somewhat good news is that I tried out the simple reproduction of this issue on the react 18 branch, where we useSyncExternalStorage, and the issue does not occur there:

• https://codesandbox.io/s/useeffect-bug-react-18-ni2sr

now it still occurs if we downgrade to react17, where the uSES shim is used, and just upgrading to react18 doesn't solve it either.

so it seems to be a combination of both things:

• something in react
• something in react-query on how we setup subscriptions currently in an effect, or something how we do batching.

I guess the only thing that I can tell you now is that it seems to work with 18, even in sync mode, when we migrate to uSES. Not sure when that will happen though.

[dmitryKochergin]

Hi! I was really interested to figure this out. @lgenzelis Here's your version of code, but without react-query: https://codesandbox.io/s/react-or-react-query-bug-version-2-forked-fnebv?file=/src/example.js

I just noticed, that useQuery gets result from observer.getOptimisticResult and it's actually not a part of react state. Thus, it's the same as if I move value out of render function and change it as a variable. And I cannot reproduce it with react 18, so I guess it's really some kind of bug in react 17.

[TkDodo]

very interesting - thank you @dmitryKochergin . Do you think it would make sense to open issue for react itself with that reproduction?

[lgenzelis]

@dmitryKochergin you're awesome! 😁😁 I spent some time myself trying to get react query out of the equation but I had failed. It's really cool to have such a small reproduction for this bug.

Here's a new version of the sandbox, where I completely removed react query, and upgraded react and react-dom to v18.0.0-rc.

@dmitryKochergin , you said you couldn't reproduce it in react 18, but I'm guessing you went straight to the new API (ReactDOM.createRoot and root.render). Using the new API the bug does indeed disappear, but it persists in the legacy API (ReactDOM.render).

Since you're the one that got it to this point, dmitry, I'd say you get the honor to report it to react 😄

Also, @TkDodo , I'd say we can safely close this issue here, as it's clearly not a problem with react query.

[dmitryKochergin]

@TkDodo @lgenzelis thanks 😄 I also found something else, and I guess it can explain current behaviour: https://github.com/facebook/react/issues/23055#issuecomment-1003712895

If you update a State Hook to the same value as the current state, React will bail out without rendering the children or firing effects.

So, in our case we have dummy and forceRerender state variables. After calling rerender function, dummy updates with the same value, and React bails out without rendering the children or firing effects. I suppose that somewhere here the effect should've been scheduled, but it didn't because of that react behaviour. BUT, the render function was actually called and useEffect received updated value in deps (you can see this by checking how many times console.log was called in render). And then on the next tick forceRerender causes another update, but this time it doesn't schedule the effect because it has the same deps (value) that were captured in the previous render function call.

I'll create an issue in react repo to check if I'm right with this explanation and to clarify how and why this behaviour has changed in react 18 with the new root api.