Bug: react-hooks/exhaustive-deps false positive with direct function calls on an object?

[joealden]

eslint-plugin-react-hooks version: 4.2.0

---

When calling a function within useEffect where the function exists directly on an object and the function is included in the effect's dependency array, the react-hooks/exhaustive-dep ESLint rule is triggered. I'm wondering whether this is a false positive, or if there is actually a valid reason for this ESLint rule to be triggered in this scenario.

Please let me know if this has already been answered/explained elsewhere; I tried searching for this problem and couldn't find anything about it!

It's also worth noting that I found this out when trying to call react-query useMutation mutateAsync within a useEffect call. As the return value of useMutation is not referentially stable (TanStack/query#1858), including the whole object in the dependency array isn't feasible, as it causes the effect to fire more than it should. However, as mentioned in that linked issue, the "functions / objects on the returned object are referentially stable", which means that including x.mutateAsync (where x is assigned the return value of the useMutation call) in the dependency array means that the effect fires only when it actually needs to/should do. However when trying to do so, react-hooks/exhaustive-dep returns the warning mentioned.

I understand that I could get around this warning by destructuring the mutateAsync off of the returned object before using it in useEffect, but that would be worse DX wise in my opinion, and I'd like to avoid it if possible.

Steps To Reproduce

1. Call a function within useEffect where the function exists directly on an object and include the function in the effect's dependency array.
2. See the ESLint warning coming from react-hooks/exhaustive-dep.

Link to code example: https://codesandbox.io/s/bold-fast-smo6g?file=/src/App.js

The above example shows the following scenarios (to show that the behavior feels inconsistent and potentially wrong):

1. Calling a function within useEffect where the function exists directly on an object (the case in question).
2. Calling a function within useEffect where the function did exist on an object, but was called from a different variable.
3. Referencing another value (in this case a number) within useEffect where the value exists directly on an object.

The current behavior

The react-hooks/exhaustive-dep rule returns the following warning (in the case in the code example above):

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

The expected behavior

No warning comes from react-hooks/exhaustive-dep, or an explanation is given by the rule as to why this is a problem?

[nmain]

This is because of the this parameter. If test is replaced with a new object that has the same fn value as the old one, then a deps list of [test.fn] won't trigger an effect rerun, but because fn is called with the test object in the this parameter, fn could have different behavior depending on other changed contents of test.

[joealden]

@nmain good point, and thanks for explaining that, but I'm now wondering the following:

1. Is this actually intentional (as in, has the ESLint rule be written to explicitly trigger in this case)? If so, like I mentioned above, I personally think it might help to have more detailed messaging for these kinds of JS "edge cases".
2. I personally don't directly use this in any of the code I write (due to these kinds of implicit dependency problems), so I think there's a very high chance I'd never run into this case where an effect doesn't fire when it likely should. Of course I am only one developer, but I'd presume that most developers using React also rarely reach for APIs designed around this usage, especially in the context of it being needed in an effects dependency array? If this is the case, then should this be enforced as it is (without a way to disable it)?

[nmain]

I believe this is intentional. See it here mentioned as an aside in a comment: https://github.com/facebook/react/blob/master/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L890

I agree that this is not commonly used in the style of programming that functional components use. And in your specific case, fn is an arrow function so the lint rule is being overly cautious and there is no this issue.

[joealden]

I believe this is intentional. See it here mentioned as an aside in a comment [...]

Nice find! I looks like that block of code is specifically targeting props access, which means that an extraWarning doesn't get set in the case I've outlined in this issue (which as I've suggested, better messaging could make this a lot less confusing for those that don't think about the potential implications of this usage).

And in your specific case, fn is an arrow function so the lint rule is being overly cautious and there is no this issue.

Good point, I think it'd be great if rule could be improved so that it only applies the warning when fn is a "regular function", and not when it is an arrow function (as I presume the AST exposed by ESLint differentiates between function types).

[tylersayshi]

Could this warning be one that is optionally enabled? It seems to me that it would be less confusing to users if they could opt in and out of the warning. This would also lend itself to a nice place to document the edge case and why it is a provided warning.

[jzxchiang1]

+1, just ran into this and it's slightly confusing.

[eps1lon]

Duplicate of #16265