React: Bug: eslint(react-hooks/exhaustive-deps) wrong deps with optional chained function calls

[AviVahl]

React version: 16.13.1

Steps To Reproduce

Every time optional chaining is used with a function call, the function itself is detected by the rule.

// users is optional...
const someContext = {
    users: Math.random() > 0.5 ? [{name: 'test'}] : undefined,
};

const filteredUsers = useMemo(() => {
    return someContext.users?.filter((u) => u.name.startsWith('a'));
}, [someContext.users]);

The current behavior

v4.0.1 and above of eslint-plugin-react-hooks tells me to change the dep to someContext.users?.filter.

The expected behavior

Allow me to keep using someContext.users. I can imagine scenarios where I would want the looked up symbol, but that's probably less common than depending on the uniqueness of the instance.

Observations

Probably regressed due to https://github.com/facebook/react/pull/18820, but I'm not sure.

[DavidHenri008]

I also have this behavior since upgrading to "eslint-plugin-react-hooks": "4.0.2", from version 4.0.0.

In my case:

useEffect(() => {
    const newSelectedVersion = globalPackage?.version || globalPackage?.versions[0].version;
    setSelectedVersion(newSelectedVersion);
  }, [globalPackage]);

Produce the following warning message:

React Hook useEffect has missing dependencies: 'globalPackage?.version' and 'globalPackage?.versions'. Either include them or remove the dependency array  react-hooks/exhaustive-deps

[lynxtaa]

I have similar problem

const userEditable = useMemo(
  () =>
    !formValues?.nodeIds ||
    formValues.nodeIds.every(({ id }) => id),
  [formValues?.nodeIds],
)

Error: React Hook useMemo has a missing dependency: 'formValues.nodeIds'. Either include it or remove the dependency array react-hooks/exhaustive-deps. Autofix rewrites it as:

const userEditable = useMemo(
  () =>
    !formValues?.nodeIds ||
    formValues.nodeIds.every(({ id }) => id),
  [formValues.nodeIds, formValues?.nodeIds],
)

It will throw in runtime because formValues may be undefined

[ZiiMakc]

It also complains about data?.user.theme

warning React Hook useEffect has a missing dependency: 'data.user.theme'. Either include it or remove the dependency array react-hooks/exhaustive-deps

[AviVahl]

Guys, every time it complains about the looked up field which is not a function call, the new behavior is probably more correct.

In the examples about, if the memoized value depends on formValues?.nodeIds (@lynxtaa example) or on data?.user.theme (@ReTWi example), the lint rule has no way of knowing if formValues or data are immutable, so it compares the inner value directly (nodeIds and theme).

@lynxtaa the expression could have been just formValues?.nodeIds?.every(({id}) => id), and then the dep should have been: formValues?.nodeIds, but that's where the bug occurs in the lint (current version suggests formValues?.nodeIds?.every).

[lynxtaa]

@lynxtaa the expression could have been just formValues?.nodeIds?.every(({id}) => id), and then the dep should have been: formValues?.nodeIds, but that's where the bug occurs in the lint (current version suggests formValues?.nodeIds?.every).

In case of !formValues?.nodeIds || formValues.nodeIds.every(({ id }) => id) the result will be true when formValues is undefined. So just formValues?.nodeIds?.every(({id}) => id) is not a valid substitution

[AviVahl]

Oh my bad. formValues?.nodeIds?.every(({id}) => id) ?? true

[AviVahl]

@jcestibariz see my comment from before. The correctness of depending on the actual field being tested for (foo?.bar, in your example) vs. depending on the whole instance (foo) could be argued either way.

foo?.bar covers both the immutable and mutable cases and is probably going to be a "more correct" guess.

This issue is about functions being targeted for deps, which is incorrect in most cases.

[yanneves]

Relates to #18985, should now be resolved in [email protected]

[AviVahl]

thanks for the fix!

[AviVahl]

Re-opening. Bug still occurs:

import React, { useMemo } from 'react';

export const Comp = ({ text }) => {
  const sliced = useMemo(() => text?.slice(0, 5), [text]);
  // ...
};

Plugin suggests changing text to text?.slice.

[dmarkow]

Still happening here too. The fix seems to only work for useEffect. useMemo and useCallback are still triggering the same warnings:

useEffect(() => {
  console.log(a?.b);
}, [a]) // No errors

var c = useMemo(() => {
  return a?.b;
}, [a]) // React Hook useMemo has an unnecessary dependency: 'a'. Either exclude it or remove the dependency array

If I remove [a] in the useMemo version, it then tells me I need to add a?.b as a dependency.

[yanneves]

Interesting, it's not obvious from the plugin code that useMemo or useCallback would be treated differently when matching their dependencies - I'll see if I can apply it to these also, and anywhere else it might not be working

[gaearon]

Happy to keep taking fixes :-)

[vadzim]

still happening in [email protected]

[AviVahl]

Will probably be fixed by https://github.com/facebook/react/pull/19062 (which is waiting for a merge/release).

[AviVahl]

Should be fixed by [email protected]

[gaearon]

Remaining optional chaining problems should be fixed in [email protected]. If you experience some problem with optional chaining after 4.0.6, please file a new issue. Thanks.