CODEWITHSUNDEEP
reactjsreact-hooks
luke.belliveau
luke.belliveau

Reputation: 105

Obeying react-hooks/exhaustive-deps leads to infinite loops and/or lots of useCallback()

My app has a userService that exposes a useUserService hook with API functions such as getUser, getUsers, etc. I use a Hook for this because the API calls require information from my Session state, which is provided by a React Context Provider.

Providing the getUsers functions to a useEffect call makes the react-hooks/exhaustive-deps eslint rule unhappy, because it wants the getUsers function listed as a dep - however, listing it as a dep causes the effect to run in an infinite loop, because each time the component is re-rendered, it re-runs the useUserService hook, which recreates the getUsers function.

This can be remedied by wrapping the functions in useCallback, but then the useCallback dependency array runs into a similar lint rule. I figure I must be doing something wrong here, because I can't imagine I'm supposed to wrap every single one of these functions in useCallback().

I've recreated the issue in Codesandbox.

1: Encounter eslint warning: https://codesandbox.io/s/usecallback-lint-part-1-76bcf?file=/src/useFetch.ts

2: Remedy eslint warning by sprinkling useCallback in, leading to another eslint warning: https://codesandbox.io/s/usecallback-lint-part-2-uwhhf?file=/src/App.js

3: Remedy that eslint rule by going deeper: https://codesandbox.io/s/usecallback-lint-part-3-6wfwj?file=/src/apiService.ts

Everything works completely fine if I just ignore the lint warning... but should I?

Upvotes: 7

Views: 2571

Answers (1)

Dan Abramov
Dan Abramov

Reputation: 268235

If you want to keep the exact API and constraints you've already chosen, that is the canonical solution — you need to ensure that everything "all the way down" has useCallback or useMemo around it.

So this is unfortunately correct:

I can't imagine I'm supposed to wrap every single one of these functions in useCallback()

There is a concrete practical reason for it. If something at the very bottom of the chain changes (in your example, it would be the "session state from React's context" you're referring to), you somehow need this change to propagate to the effects. Since otherwise they'd keep using stale context.

However, my general suggestion would be to try to avoid building APIs like this in the first place. In particular, building an API like useFetch that takes an arbitrary function as a callback, and then calling it in effect, poses all sorts of questions. Like what do you want to happen if that function closes over some state that changes? What if the consumer passes an inline function that's always "different"? If you only respected the initial function, would you be OK with the code being buggy when you're getting cond ? fn1 : fn2 as an argument?

So, generally speaking, I would strongly discourage building a helper like this, and instead rethink the API so that you don't need to inject a "way to fetch" into a data fetching function, and instead that data fetching function knows how to fetch by itself.

TLDR: a custom Hook taking a function that is then needed inside of an effect is often unnecessarily generic and leads to complications like this.

For a concrete example of how you could write this differently:

  1. First, write down your API functions at top level. Not as Hooks — just plain top-level functions. Whatever they need (including "session context") should be in their arguments.
// api.js

export function fetchUser(session, userId) {
  return axios(...)
}
  1. Create Hooks to get any data they need from the Context.
function useSession() {
  return useContext(Session)
}
  1. Compose these things together.
function useFetch(callApi) {
  const session = useSession()
  useEffect(() => {
    callApi(session).then(...)
    // ...
  }, [callApi, session])
}

And

import * as api from './api'

function MyComponent() {
  const data = useFetch(api.fetchUser)
}

Here, api.fetchUser never "changes" so you don't have any useCallback at all.

Edit: I realized I skipped passing the arguments through, like userId. You could add an args array to useFetch that only takes serializable values, and use JSON.stringify(args) in your dependencies. You'd still have to disable the rule but crucially you're complying with the spirit of the rule — dependencies are specified. Which is pretty different from disabling it for functions, which leads to subtle bugs.

Upvotes: 9

Related Questions