Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: react-hooks/exhaustive-deps not recognizing dependencies when accessed via optional chaining #19061

Closed
fredvollmer opened this issue Jun 1, 2020 · 2 comments
Labels
Component: ESLint Rules Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@fredvollmer
Copy link
Contributor

fredvollmer commented Jun 1, 2020

This is, I believe, an extension of the issue reported in #18985.

While the PR that resolved that issue fixed the lint rule for the useEffect hook, it seems to remain an issue for useCallback and useMemo.

When the optional chaining operator (?.) is used to access a path inside a useMemo or useCallback hook, and a prefix of that path is listed in the dependencies array, eslint-plugin-react-hooks reports an unexpected "missing dependency" warning. Replacing ?. with . produces the expected behaviour: the warning is no longer reported.

In many cases, adding the complete path, rather than a prefix, to the dependencies array is the correct answer. However, in some cases, the complete path might point to a prototype method of an object, in which case only a prefix of the path should be listed. (Hopefully the example below makes this more clear.)

React version: 16.13.1
eslint-plugin-react-hooks version: 4.0.4

Steps To Reproduce

 import React, {useEffect} from 'react';

 export function MyComponent(props) {
   useCallback(() => {
     console.log(props.foo?.toString());
   }, [props.foo]);
    
    return null;
  }
  • It doesn't matter what foo, represents, other than it has a prototype method, toString().
  • We don't want to reference the prototype method of foo (props.foo?.toString) in the dependencies array because our function depends on props.foo, not the prototype of props.foo.

The current behavior

The following warning is reported:

React Hook useCallback has an unnecessary dependency: 'props.foo'. Either exclude it or remove the dependency array  react-hooks/exhaustive-deps
  • Note that if I list props.foo?.toString instead of props.foo in the dependencies, no warning is reported. However, this is not acceptable as the callback depends on props.foo, not the prototype of props.foo.
  • If I remove the optional chaining operator from the above code (so that the body of the callback reads console.log(props.foo?.toString());) then no warning is reported, as expected.

The expected behavior

No warning should be reported.

@fredvollmer fredvollmer added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jun 1, 2020
fredvollmer added a commit to fredvollmer/react that referenced this issue Jun 1, 2020
fredvollmer added a commit to fredvollmer/react that referenced this issue Jun 1, 2020
fredvollmer added a commit to fredvollmer/react that referenced this issue Jun 1, 2020
fredvollmer added a commit to fredvollmer/react that referenced this issue Jun 1, 2020
josejulio added a commit to josejulio/policies-ui-frontend that referenced this issue Jun 15, 2020
@darkbasic
Copy link

It used to work so I think it's a regression.

gaearon pushed a commit that referenced this issue Jun 30, 2020
…ing prototype method inside useCallback and useMemo #19061 (#19062)

* fix(eslint-plugin-react-hooks): Support optional chaining when accessing prototype method #19061

* run prettier

* Add fix for #19043
@gaearon
Copy link
Collaborator

gaearon commented Jun 30, 2020

Fixed by #19062

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ESLint Rules Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

4 participants