useMutation returns a new object on every call

[ashleymercer]

Describe the bug

If I'm reading the code right, I think the useMutation function creates a new object on every call - so if this object is used directly in the dependency array for any further React hooks, it will trigger additional renders.

To Reproduce

function FooComponent() {
  const callback    = useCallback(() => Promise.resolve("done"), []);
  const mutation    = useMutation(callback);
  const mutationRef = useRef(mutation);

  useEffect(() => {
    if (mutationRef.current !== mutation) {
      console.log(`This should never get logged`);
    }
  })

  return <></>
}

Expected behavior

The log line should never fire: the arguments to useMutation never change so I would expect the same reference back each time (I think this is similar to how useQuery works - at least, I don't seem to have the same issue there).

Additional context
Looking at the current code would it be sufficient to memoise the returned object on currentResult and mutate? Or would that likely cause further problems?

[bjarkehs]

The object returned from useMutation always changes, but mutate at least never changes. I'm not sure it's expected behaviour that the object returned from useMutation never changes. I think most hooks work this way.

[TkDodo]

I think it’s expected. useQuery behaves the same now: the functions / objects on the returned object are referentially stable if you need to pass them to memoized components or add as dependency to an effect, but the actual object is not.

[thoo1]

relatively new to using this library, is there a reason for this behaviour? that the returned object is not stable but the values inside the object are?

[TkDodo]

It was introduced to make the library concurrent-mode safe. See here: #1775

Unfortunately I was not able to keep the referential integrity of the result object between renders because we do not know which optimistic result will eventually get committed.

it’s a minimal tradeoff :)

[thoo1]

makes sense, thanks!

[tschanz-pcv]

@TkDodo Thanks a lot for sharing this info. Maybe this is common knowledge among React devs, I certainly didn't expect this. Could you maybe add something to the docs regarding referential stability so it's clear what can be used in deps arrays e.g. for useEffect.

[TkDodo]

Sure, I’d accept a PR to the docs :)

generally, I’d say it’s good practice to keep your dependencies as narrow as possible, but I agree that we should alert users to this trade off.

[alesmenzel]

This has been a real gotcha, calling some API hundreds of times per second is not fun.
This is even more unfortunate because react hooks eslint plugin will warn users that they actually need tu put the whole mutation object into the dependency array, causing all the havoc.

Here is a minimal example of such busted behavior:

const SomeComponent = () => {
       const { isLoading, data } = useQuery('toggle-state', fetchToggleStateFromApi)
	const mutation = useMutation(changeToggleStateInApi)

	const handleChange = useCallback(
		(state) => {
			mutation.mutate(state)
		},
		// [mutation, row] // DO NOT DO THIS even if eslint recommends you to add "mutation" there
		// eslint-disable-next-line
		[mutation.mutate]
	)
	
	return (<Toggle value={data} disabled={isLoading} onChange={handleChange}>)
}

[TkDodo]

@alesmenzel destructuring can help here:

const { mutate } = useMutation(changeToggleStateInApi)

const handleChange = useCallback(
    (state) => {
        mutate(state)
    },
    [mutate]
)

taking this one step further, state => mutate(state) is the same as just mutate, so it would read:

const handleChange = useCallback(
    mutate,
    [mutate]
)

which in turn, is just mutate. Probably you simplified the example a bit :)

---

that being said, the <Toggle> component will likely only call onChange when the user clicks the toggle, not when the functional identity of handleChange changes.

I guess the only reason to stick useCallback onto handleChange is if the Toggle component is wrapped in React.memo and you want to avoid an "unnecessary render" of that component. I don't see how that can result in an api being called hundreds of times per second...

[jzxchiang1]

I'm running into a similar problem with eslint complaining about exhaustive deps.

See facebook/react#21624.

Does mutate ever access this? If not, then I think it's safe to pass mutation.mutate as a dep to useCallback. If not, then the eslint error is justified.

[TkDodo]

can you show a codesandbox example with the failing lint rule please?

[alesmenzel]

@TkDodo Here is a very small repro with both the correct and broken solutions (uncomment the correct one to see the eslint warning)
https://codesandbox.io/s/white-glitter-s6ujk?file=/src/BrokenExample.tsx

[TkDodo]

@alesmenzel I think this is a bug in the lint rule, which has been fixed already. I updated the sandbox to react-scripts 5.0 and the issue is gone: https://codesandbox.io/s/nifty-tdd-3gn9q

Not sure which version of the eslint-plugin is used under the hood, or which released fixed it :)

[TkDodo]

okay weird, maybe it was just slow - the error is still there. sorry for the confusion. Seems like the only way to get around it is destructuring? I think it is safe to destruct mutate and mutateAsync from the result though.

[edzis]

Destruction does work but becomes verbose when you have multiple mutations in a single block and using multiple functions in other hooks.

To avoid rerunning my dependent hooks every frame I am wrapping the results in a forked version of https://github.com/kentcdodds/use-deep-compare-effect/blob/main/src/index.ts#L28-L43

But that is not enough to guard calls to mutate() or reset() - can not afford to call those when the state of the mutation changes. Opting for a local eslint override but with a comment.

  const closeModal = useCallback(() => {
    updateManyMutation.reset();
    createOneMutation.reset();
    updateOneMutation.reset();
    deleteOneMutation.reset();

    setGroupTemplate(null);
    setModal(null);
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [
    // exclude mutations - linter prevents listing only reset functions
    modal,
  ]);

[Harukisatoh]

I still don't understand why it wasn't possible to guarantee the referential integrity of the mutation object, could you elaborate more on this?

Just asking because in my point of view this would be a nice thing to have implemented in the lib itself. As @edzis pointed out, destructuring works but it's not a good option for certain cases.

[keanemind]

An alternative to destructuring is using Function.prototype.call (or bind or apply). This explicitly removes the function's dependency on this, so the ESLint rule can safely allow it. If mutate/mutateAsync actually used this in their implementations, then this would break those functions. But because they actually don't, we can use call/bind/apply as a way of declaring that fact to ESLint.

e.g.,

  const myMutation = useMutation(myAsyncFunction);

  useEffect(() => {
    myMutation.mutateAsync.call(undefined, "first argument");
  }, [myMutation.mutateAsync]);

[ardalan-nhd]

@TkDodo Can we do something like below?

const mutation = useMutation(mutationFn) // or useQuery, etc.

useEffect(() => {
  // effect
}, [...Object.values(mutation), ...otherDeps])

It seems to work as expected, since you mentioned:

the functions / objects on the returned object are referentially stable

But I don't know if that's a good thing to do or not.

[orchidoris]

I run into this issue myself and I'm here to summarize my findings, so maybe they can be useful for others.

Below I describe two solution. I prefer the 2nd solution where it's possible.

The issue

// DEFINITION
import axios from 'axios';
import { useMutation } from '@tanstack/react-query';

const client = axios.create({ baseURL: PATH_BASE });
export const search = async (request: SearchRequest) => (await client.post<SearchResult[]>('/api/search', request)).data;
export const useSearch = () => useMutation({ mutationFn: search });

// USAGE
export const SearchPage = () => {
  const searchResult = useSearch();

  // ⚠️ Here ESLint adds searchResult into dependencies, so this useEffect(...) runs indefinite amount of times
  useEffect(() => searchResult.mutate({}), [searchResult]);

  return <>
    <SearchBar onChange={searchResult.mutate} />
    <SearchTable data={searchResult.data} />
  </>;

Solution 1: Put mutate into a separate variable

One way to go about it is move mutate into a separate variable, so ESList would expect only mutate in dependencies which doesn't change unlike mutationResult:

// USAGE
export const SearchPage = () => {
  const { mutate: search, ...searchResult } = useSearch();
  useEffect(() => search({}), [search]); // ⭐ Now ESList suggests safe dependencies

  return <>
    <SearchBar onChange={search} />
    <SearchTable data={searchResult.data} />
  </>;
}

Solution 2: Use useQuery instead of useMutation when POST request requires initial load

Before running into this issue I didn't realize that useQuery can be used with POST request.

I prefer this approach where it's possible.

// DEFINITION
import axios from 'axios';
import { useQuery } from '@tanstack/react-query';

const client = axios.create({ baseURL: PATH_BASE });
export const search = async (request: SearchRequest) => (await client.post<SearchResult[]>('/api/search', request)).data;
export const useSearch = (request: SearchRequest) =>
  useQuery({ 
    queryKey: ['search', JSON.stringify(request)],
    queryFn: () => search(request),
  });

// USAGE
export const SearchPage = () => {
  const [searchRequest, setSearchRequest] = useState<SearchRequest>({});
  const { data: searchData } = useSearch(searchRequest); // ⭐ Automatical initial request, no need for useEffect now

  return <>
    <SearchBar onChange={setSearchRequest} />
    <SearchTable data={searchData} />
  </>;