isLoading isn't safe to use in a hook's dependency array because it comes from a mutable object, not state

[ericbiewener]

In React Query v2.5.6 (and likely all of v2), isLoading was actual component state. This appears to have changed in v3, and it continues to not be React component state in v4 as well. This is true for both useQuery and useMutation.

I was surprised to see when upgrading that some of our useEffect hooks that had [isLoading] in the dependency array were not always executing even when that value changed. This led me to this explanation from Dan Abramov, where he states that essentially anything that is not a prop or state isn't guaranteed to ensure that a useEffect hook fires because React could bail on the update. I'd argue that this is a major gap in React's documentation, and it requires developers to know the implementation details of every value they put into a dependency array, which strikes me as unreasonably burdensome.

Curious to know the maintainers' and community's thoughts on this.

[ericbiewener]

Looking into this further, I see that this CodeSandbox repro I put together actually has a lint warning from the react-hooks/exhaustive-deps rule. Never noticed that before! Also, a more careful reading of the React docs do suggest this consequence. Here they note that situations exist where React will bail from the state update, and the useEffect section just a little lower down states:

By default, effects run after every completed render

Emphasis mine. In the case where React bails on the state update, the render is not completed.

Unfortunately, the eslint warning pictured below will not appear for isLoading since that lint rule doesn't know that is coming from a ref.

[ericbiewener]

Just going to continue talking to myself here :)

I see that in RQ3, the useQuery notifyOnChangeProps option was created as part of optimizing which property changes would trigger re-renders. I don't see a similar option for mutations. Perhaps that is a gap in functionality?

[TkDodo]

isLoading comes from an external store, and we use useSyncExternalStore for that in v4.

I see that in RQ3, the useQuery notifyOnChangeProps option was created as part of optimizing which property changes would trigger re-renders. I don't see a similar option for mutations. Perhaps that is a gap in functionality?

mutations don't have that property, yes. They also very likely don't need it.

Unfortunately, the eslint warning pictured below will not appear for isLoading since that lint rule doesn't know that is coming from a ref.

refs are mutable objects from react, this is totally different to how our store works.

[ericbiewener]

mutations don't have that property, yes. They also very likely don't need it.

I think this whole thread comes down to discussing this point (the rest was me finding my way through the docs & implementation details!). In my app, we've got a Submit button that we want to disable while a mutation is in flight. In order to correctly update this UI, it seems that we need to create our own isLoading state separate from useMutation, and then call setIsLoading(true) in the mutation options onMutate hook, and setIsLoading(false) in onSettled. This certainly works. But it is also less convenient than being able to depend on the isLoading property from useMutation. So I believe this is a feature request :)

[TkDodo]

I don't quite follow. We do the same and it looks like this:

const SubmitButton = () => {
  const { mutate, isLoading } = useMutation(...)

  return <Button onClick={() => mutate()} disabled={isLoading}>Submit!</Button>
}

[ericbiewener]

Agreed, I can't create a simple repro, so perhaps I jumped to conclusions about what the actual cause of the failing test was in my repo.

Is it ultimately guaranteed that when the mutation's status changes, there is a corresponding state update that will cause the component to re-render?

[TkDodo]

Is it ultimately guaranteed that when the mutation's status changes, there is a corresponding state update that will cause the component to re-render?

yes.