[Feature Request] exhaustive-deps linting rule: allow custom hooks to specify the index of their callback effect

[ReillyBova]

Background

Currently, the react-hooks/exhaustive-deps linting rule allows developers to lint custom hooks. E.g.:

{
  "rules": {
    // ...
    "react-hooks/exhaustive-deps": ["warn", {
      "additionalHooks": "(useMyCustomHook|useMyOtherCustomHook)"
    }]
  }
}

While the official documentation for this configuration states "We suggest to use this option very sparingly, if at all", there are nonetheless certain custom hook cases that necessitate such an approach. Consider the following scenario: (1) a callback needs to hook into an external system (thus requiring an effect), (2) the callback has dependencies on state / props and needs to reexecute when those change, (3) executing that callback can be arbitrarily expensive, so re-executing every render is not an option.

Motivation

To meet this challenge, we have two options: (1) treat the callback reference as sufficiently reactive (by this I mean, referentially stable unless a dependency has changed) — in other words, we assume the user of the hook is passing in a callback that was defined via useCallback, with the appropriate dependencies; (2) accept dependencies in the custom hook itself, wrap the function in a useEvent / useEventHandler style hook for referential stability, apply the dependencies to whatever effect the function is then executed within.

If we go with option (1) we have an obvious shortcoming — there's no way to ensure the user is wrapping their callback in useCallback. From personal experience on a large industry project with lots of developers, this is just not a viable option. Option (2) on the other hand meets all our needs, and via the additionalHooks config, we can ensure correctness by linting for exhaustive dependencies.

The Problem

Great! We've met our challenge with a robust solution and easy to use solution, however...... what if our hook accepts a third, or fourth arg? Well, the current implementation of exhaustive-deps only allows for the [callback, deps] args to occupy arg0 and arg1 of our custom hook (e.g., useCustomHook(effectCallback, effectDeps)). While we can still get everything to "work", this because more than a minor headache if a hypothetical arg2 we want to add contains some of the most important contextual information. This pain is particularly acute because callbacks are (preferably) defined inline within hooks, and so they'll have a tendency push any succeeding args pretty far down, making them almost seem irrelevant.

An example from React

While this is ultimately an issue of style, the resulting requirement forces custom hooks that need the exhaustive deps rule into a very awkward argument pattern. It seems I'm not alone, as React's own useImperativeHandle seems to agree in the preferred structure for hooks that accept effectful callbacks with deps. In the case of useImperativeHandle, the most important, unique, and concise value — the ref — is passed first, and only then followed by the more verbose and less information-dense callback + deps args.

The Proposal

Allowing developers to add their custom hooks to the linting tool is genuinely useful, but it does come with the significant caveat that the effect and deps have to be used as the first two args. I propose either of the following revisions:

1. The additionalHooks rule be modified to alternatively accept an object of the form { [callbackArgIndex: number]: customHookRegex }. For instance, adding a rule for useImperativeHandle would look like

   "additionalHooks": { 1: "useImperativeHandle" }"

2. Instead, the behavior of the rule should be altered to assume that the callback and the dependencies occupy the final two arguments of any given custom hook. This pattern seems consistent with how all native hooks with callback-deps patterns are defined, and so it seems to be a better "default assumption" over the current approach.

If this proposal is not moved forward with, I highly recommend that the documentation for the linting rule at least be updated to clarify that the linted callback and dependencies must occupy the first two arguments of any custom hook. Mine was failing silently and it took a deep dive into the implementation to understand what was going on!

[lojjic]

Implemented with a slightly different syntax in https://github.com/facebook/react/pull/24344