Feature Request: Exclude individual dependencies from exhaustive-deps rule

[0livare]

Currently, the react-hooks/exhaustive-deps is all or nothing. I leave the rule turned on (for a particular set of dependencies), or I disable it. Clearly this rule is beneficial in preventing bugs, but there are also definitely times when a dependency needs to be purposefully excluded from a dependency array.

When those situations come up, the rule then has to be disabled for ALL of the dependencies

// eslint-disable-next-line react-hooks/exhaustive-deps

So now if someone comes along later and adds an additional dependency to this effect (or memo etc.), there is no linting rule there to notify them of the need to add that dependency to the dependency array.

One approach to accomplishing this might be by using block comments. For example, if I have three dependencies to an effect, foo, bar, and baz, I can disable the dependency check for bar only, and leave the dependency checking in place for all other dependencies.

useEffect(() => {
  foo()
  bar()
  baz()
}, [foo, /*bar*/, baz])
//          ^
// No linting error because I have explicitly stated that I wish to disable dependency checking for `bar`

[lunaruan]

Hey! Thanks for posting about this feature request! From our experience, disabling the dependency check or leaving dependencies out of the dependency array can lead to stale closure bugs that are really hard to find. What is a specific case where you need this feature? We've found that most cases can be rewritten so that you no longer need to exclude the dependency.

[0livare]

Hey @lunaruan, thanks for getting back to me so quickly! I can definitely provide the example that prompted this feature request.

const store = useStore() // Some arbitrary redux style store
const {data, isFetched} = useApiQuery() // Some API call wrapped in a hook

// I want this effect to run exactly once to set an initial value that is displayed 
// to the user.  If `data` changes at some point when the user is on this screen, I
// don't want the initial state that was already shown to the user to change.  Even
// more concerning would be if that update were to override changes already made by
// the user.
useEffect(() => {
  if (!isFetched) return
  store.update({foo: data.foo})
}, [isFetched, store, /*data*/])

If there is a better way to handle this situation, I would love to understand how this code could be improved!

However, although I do understand that the React team's position on this is that disabling this check can lead to bugs (I've experienced that first hand), I would argue that you and I couldn't possibly brainstorm every scenario where disabling it could be necessary or beneficial. It is already of course possible to disable the rule entirely, but my entire goal here is to avoid having to do that because I do really understand the value that this dependency check provides.

In my head, this is similar to dangerouslySetInnerHTML, in that it is possible for the feature to simultaneously exist and for use of it to be discouraged.

[lunaruan]

Hey! Thanks for the code example!

We believe that there are two reasons why people might use an effect:

• Reactive effects - these keep your react state in sync with some external system and always need to rerun in response to changes to any of their dependencies. These effects are why we believe that it's important to prevent people from excluding dependencies from the dependency array.
• Event effects - these effects are run when something happened in your app. (ex. an event cause a component to render). For these effects, we recommend you don't use dependency arrays at all, and instead use refs.

Your example seems to be an event effect. We would recommend you rewrite this as follows:

const shouldUpdateStore = useRef(true);
useEffect(() => {
  if (shouldUpdateStore.current && isFetched) {
    store.update({foo: data.foo});
   shouldUpdateStore.current = false;
  }
});

In general, for event effects, we believe that often it's possible to eliminate them entirely by moving the code up to the event that triggered the effect.

I agree that we couldn't possibly brainstorm every scenario, but from our experience, we've seen that almost every case where developers want to exclude individual dependencies or remove the exhaustive-deps rule can actually be rewritten in a better way.

Closing this issue. Feel free to reopen if you have any followup questions!

[0livare]

Thanks for your response, I had not thought to use a ref in that way and I'll give that a shot!

I do feel though that the approach here of putting the onus on the developer to "do better" is a little oversimplified.

In my experience, most developers most of the time are in a rush to meet some deadline. There is unfortunately not often time to do something the perfect way, especially when that involves a more complex approach, as I would argue this case does.

Not providing an escape hatch to disable a single dependency will undoubtedly lead to more eslint-disable-line's than would have to exist without it, which will unfortunately lead to more bugs when the code is edited again next time.

[0livare]

Additionally, creating a ref exclusively for the purpose of making an effect not re-run when a particular dependency changes is such an indirect way to solve the problem.

I feel that the intent of the code you're writing by introducing this extra ref is really hazy. When someone else looks at this code a few weeks or months, how can they be certain that the only purpose of the ref is to ignore the dependency? Should I now add a code comment in addition to the ref?

p.s. This issue is essentially a duplicate of #20067

[ipsips]

Fix this:

OK:

const [value, setValue] = useState();
useCallback(() => {
  setValue();
}, []);

not OK:

const useDebouncedState = (initialValue, delay = 50) => {
  const [value, setValue] = useState(initialValue);
  const [debouncedValue, setDebouncedValue] = useState(initialValue);

  useEffect(() => {
    const handler = window.setTimeout(() => setDebouncedValue(value), delay);
    return () => window.clearTimeout(handler);
  }, [value, delay]);

  return [debouncedValue, setValue];
};

const [value, setValue] = useDebouncedState();
useCallback(() => {
  setValue();
}, []); // <= warning: React Hook useCallback has missing dependencies: 'setValue'. Either include them or remove the dependency array  react-hooks/exhaustive-deps

this eslint config does not help:

'react-hooks/exhaustive-deps': [
  'warn',
  {
    additionalHooks: 'useDebouncedState',
  },
],