React: [react-hooks/exhaustive-deps] False positive for nested dependencies in useEffect hook

[johnloven]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
The following code

const callback = useCallback(() => {
    console.log("CALLBACK");
}, []);

const wrapper = { callback };

useEffect(() => {
    wrapper.callback();
}, [wrapper.callback]);

gives the following react-hooks/exhaustive-deps warning

React Hook useEffect has a missing dependency: 'wrapper'. Either include it or remove the dependency array

If the useEffect is replaced by the following

useEffect(() => {
    const callbackReference = wrapper.callback;
    callbackReference();
}, [wrapper.callback]);

there are no warnings.

What is the expected behavior?
There should be no warnings in either case.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
react: 16.8.4
eslint-plugin-react-hooks: 1.6.0

[bartlangelaan]

This issue is especially cumbersome for people that are trying out React Hooks for the first time. That's because a context provider that provides a setter would already create a rerender loop.

I've created a CodeSandbox to illustrate the use case here:

I've tried to understand how the eslint rule works, but I'm not familiar with the eslint visitors so I've no clue where to look in the long file.

[miraage]

What is the expected behavior?
There should be no warnings in either case.

Must be warnings in both cases. You are actually using wrapper variable, so it must be added to deps list.

[marceloadsj]

@miraage I'm not sure about it!

This happen with functions, but not with values:

The first useEffect will run every time the props.someFunc changes.

If we put like [props, props.someFunc] or just [props], it will run every time any props change, cause react create a new props object every time some props change (shallow copy).

[miraage]

@marceloadsj eslint rule is seeking for variables used. But I totally agree with an idea that tools should help people by covering common patterns, e.g. this case.
Another case we can "complain" about is when we are 100% sure that a certain variable is a constant which will never change, yet eslint rules says "dependency is missing".
My understanding of this situation is the eslint rule would rather be too careful about dependencies, otherwise we could miss it.

[bartlangelaan]

Hey @miraage,

You're absolutely right. The eslint rule should report every variable that we use as missing when not included in the dependency array.

However, the wrapper variable isn't really used, only a method on that variable.

If we would do something with wrapper directly it should definitely be added to the dependency array. But in this case, we only use wrapper.callback and thus adding wrapper.callback to the dep array should suffice with being 'careful' enough.

Also, the use cases provided by @johnloven, @marceloadsj and me in this issue thread point out that this is something users can walk into very quickly.

Another case we can "complain" about is when we are 100% sure that a certain variable is a constant which will never change, yet eslint rules says "dependency is missing".

So, in my understanding, this is not about 'knowing that a variable is constant' but this is about 'knowing that a variable is not used'.
If a variable is known for being constant, it's fine to be added to the dependency array (it won't trigger extra executions of the effect).
But adding a variable to the deps array that isn't used will re-trigger the effect while that wasn't needed.

[miraage]

@bartlangelaan I totally understand your position. I believe the solution could be achieved by adjusting AST visitor(s) of the eslint rule

[bartlangelaan]

I did some research, and found why the ESLint plugin is currently configured like this.

In the code samples shared in this thread, it seems pretty obvious that the function itself should be included in the dependency array instead of the object it comes from. To state another example, see this component:

function MyComponent() {
  const myArray = useArray();
  useEffect(() => {
    myArray.addEntry('item');
  }, [myArray.add]); // Currently triggers a warning, but shouldn't
}

However, _if you use the result of the function_ it's more obvious that the object should be included in the dependency array.

For example:

function MyComponent() {
  const myArray = useArray();
  useEffect(() => {
    if (myArray.hasEntry('item') {
      console.log('Whoop!');
    }
  }, []);
}

Because the output of hasEntry() is probably based on the myArray contents, the linting rule should suggest that myArray is added as a dependency instead of myArray.hasEntry.

The linting rule is currently configured to always add the object to the deps array if a function from it is executed, but I believe it should only do that if the return value of the function is actually used.

I think I found where this logic is located in the ESLint plugin, and I'm currently working on a PR that changes the behaviour to include the function in the deps array as long as its return value isn't used.

Also, the following issue seems related: #15448

[gaearon]

No, this is not a bug. The way the rule works is intentional and correct behavior.

These two examples are not identical:

const callback = useCallback(() => {
    console.log("CALLBACK");
}, []);

const wrapper = { callback };

useEffect(() => {
    wrapper.callback();
}, [wrapper.callback]);

useEffect(() => {
    const callbackReference = wrapper.callback;
    callbackReference();
}, [wrapper.callback]);

In the first example, wrapper.callback() passes wrapper as this value to the callback. This is why it needs to be a dependency.

In the second example, callbackReference does not pass any this value to the callback. This is why wrapper does not need to be a dependency.

It's subtle, but if the callback read this, you'd get potentially stale values if you didn't follow the lint rule.

---

For the common case, the fix it just to destructure it before calling.

const callback = ...

useEffect(() => {
    callback();
}, [callback]);

[marceloadsj]

Hey @gaearon, nice explanation!

I think your example have just a small issue, altought the general idea is ok with function keyword instead an arrow fn:

const callbackFn = useCallback(function() {
    console.log("CALLBACK FN");
}, []);

const callbackArrow = useCallback(() => {
    console.log("CALLBACK ARROW");
}, []);

const wrapper = { callbackFn, callbackArrow };

The callbackArrow is an arrow fn, and it don't bind to this, so the this won't be the wrapper obj.
With the callbackFn, your approach will work as you said, the this will point to wrapper.

[donaldpipowitch]

Hi @gaearon,

thank you for your explanation. I just stumbled over this myself. It wasn't obvious to me why the ESLint rule behaves the way it does. I needed your explanation to understand that. I wondered if this rule can be relaxed a little bit? When I think about prettier it is very opinionated and formats mostly everything, but sometimes it respects manually added white space for example. I wonder if this rule could behave similar?

E.g. if someone writes this:

useEffect(() => {
    someObj.doSomething();
  }, []);

warn and --fix it to this (which would be your default recommendation):

useEffect(() => {
    someObj.doSomething();
  }, [someObj]);

But if someone writes the next example than allow it:

useEffect(() => {
    someObj.doSomething();
  }, [someObj.doSomething]);

// currently that would be --fix'ed to [someObj, someObj.doSomething]

I could also imagine a configuration (similar to the eqeqeq rule where you can set allow-null.)

If the method-case [someObj.doSomething] is _really_ problematic from the linter point of view, because it is ambiguous, it would be nice to not automatically --fix this and instead create a more descriptive error/warning which tells the developer to _either_ manually write [someObject] or to destruct it like const { doSomething } = someObj;. IMHO that would help a lot to understand the problem.

[LeopoldLerch]

Hi
I have this function:

const updateCanAddItems = useCallback((canAdd: boolean) => { setCanAddItems(canAdd); if (props.onCanAddItems) { props.onCanAddItems(canAdd); } }, [props.onCanAddItems]);

however, the exhaustive-deps-Rule complains and fixes it with changing to simply "props" in the dependencies, which of course results in updating the function on every render.

However, if i do like this, it is OK for the rule:

const updateCanAddItems = useCallback((canAdd: boolean) => { setCanAddItems(canAdd); const x = props.onCanAddItems; if (x) { x(canAdd); } }, [props.onCanAddItems]);

It is technically the same, but the second one is one more declaration of a variable (and in my opinione more difficult to understand why to do so.

Is this a bug or as designed?

[bartlangelaan]

Hey @LeopoldLerch,

while your example functions look the same, they are not.

To turn your functions into a runnable example:

const updateCanAddItems1 = (canAdd) => { 
  setCanAddItems(canAdd); 
  if (props.onCanAddItems) {
    props.onCanAddItems(canAdd);
  } 
};
const updateCanAddItems2 = (canAdd) => {
  setCanAddItems(canAdd); 
  const x = props.onCanAddItems; 
  if (x) { 
    x(canAdd); 
  } 
};

const props = {
  onCanAddItems: function() {
    console.log(this.someProperty);
  },
  someProperty: 'test',
}

// This logs 'test' to the console
updateCanAddItems1();
// This gives an error
updateCanAddItems2();

Because in the second example props is not passed to onCanAddItems as this it also does not have to be in the dependencies array. So, this is as designed.

You can try it out here: https://repl.it/@bartlangelaan/React-15924-example

[LeopoldLerch]

well, you are right with "this", but, as this is useful in most cases, in my opinion it is not in regards of the props-object passed to the functional component.

the functions in there won´t be related to the props-object itself in, lets say, most cases, if not all.

In my opinion there should be at least an exception for this rule in regards of the props-object, as i think the first example is the one, most developers will think of first. and autofix will immediately change the dependency´s to "props", what is far from what is inteded.

[marceloadsj]

@bartlangelaan the functions aren't equivalent on your example, but they can be if we go for:

const props = {
  onCanAddItems: () => {
    console.log(props.someProperty);
  },
  someProperty: 'test',
}

But, of course, eslint cannot know about it!

I think React team choose to be this way to avoid generating problems on those different scenarios, even knowing the downside of writing more boilerplate code to extract attrs from objects or so...