useEffect dependency array and ESLint exhaustive-deps rule

Will Jenkins picture Will Jenkins · Jul 10, 2019 · Viewed 14.2k times · Source

I have a component that looks like this:

const MyComponent = props => {
  const { checked, onChange, id } = props;
  const [isChecked, setChecked] = useState(false);

  useEffect(() => {
    onChange && onChange({ isChecked: !!checked, id });
    setChecked(checked);
  }, [checked]);

  const childProps = {
    id,
    isChecked
  };

  return <ChildComponent {...childProps} />;
};

The exhaustive-deps lint rule isn't happy:

React Hook useEffect has missing dependencies: id and onChange. Either include them or remove the dependency array. (react-hooks/exhaustive-deps)eslint

I know that id and onChange are not going to change, so adding them to the dependency array seems unnecessary. But the rule isn't a warning, it's a clear instruction to do something.

Is the ESLint rule:

1) Over-cautious and a bit dumb in this instance, so safe to ignore?

2) Highlighting best practice - i.e. to minimise unexpected bugs that might occur in future if, for instance, changes in parent components mean that id will change at some point in future?

3) Showing an actual/possible problem with the code as it currently is?

Answer

Dupocas picture Dupocas · Jul 10, 2019

Actually the rule is very straightforward: Either pass an array containing all dependencies, or don't pass anything. So I guess the rule isn't dumb, it just doesn't know if the dependencies are going to change or not. So yes, if you are passing an array of dependencies it should contain ALL dependencies, including those you know for a fact that will not change. Something like this will throw a warning:

useEffect(() => dispatch({ someAction }), [])

And to fix this you should pass dispatch as a dependency, even though it will never change:

useEffect(() => dispatch({ someAction }), [dispatch])

Don't disable the exhaustive deps rule, as mentioned here