React: Bug: react-hooks/exhaustive-deps false postive when given undefined as deps

[sQVe]

There's currently a false positive for the react-hooks/exhaustive-deps rule as it does not accept undefined as dependency.

// Below gives => React Hook useMemo has a missing dependency: 'byId'. Either include it or remove the dependency array.
const allIds = useMemo(() => Object.keys(byId), undefined)

This is clearly noticeable together with TypeScript as it does not allow omitting the second deps parameter, with the following typings:

// allow undefined, but don't make it optional as that is very likely a mistake
function useMemo<T>(factory: () => T, deps: DependencyList | undefined): T;

Note here that deps must be either a DependencyList or undefined. It is not listed as optional.

React version: 17.0.1

Steps To Reproduce

1. Install "eslint-plugin-react-hooks": "^4.1.0" and extend it with 'plugin:react-hooks/recommended'
2. Write a hook that required a dependency list, input undefined.

The current behavior

Gives false positive when given undefined for deps.

The expected behavior

Should allow deps to be set to undefined.

[eps1lon]

Thanks for the report.

I frequently stumble over this and don't understand the problem https://github.com/DefinitelyTyped/DefinitelyTyped/pull/33220 tried to solve by allowing undefined.

It looks to me that this is "you're on your own"-territory so it's expected that you need to check for yourself whether undefined is ok.

In any case, the TypeScript types are not maintained by the React core team so they're not an argument for changing core packages.

/cc @Jessidhia

[sQVe]

@eps1lon As far as I understand it the typings only allow DependencyList | undefined as it indicates a clearer intent and would also catch potential bugs where the developer forgets to input deps.

In my opinion the defaults of the typings are more sensible than the settings for the exhaustive-deps rule. Why does exhaustive-deps allow omitting deps but not setting it to undefined?

[eps1lon]

Why does exhaustive-deps allow omitting deps but not setting it to undefined?

Gotcha, that makes more sense to me. That sounds reasonable to propose as a change if

useMemo(() => Object.keys(byId))
useMemo(() => Object.keys(byId), undefined)

would indeed not work the same statically.

[vkurchatkin]

Wha the use case for this? Why not omit useMemo?

[sarathps93]

Hi @eps1lon @sQVe is this issue up for grabs? I can give it a shot if you guys are not already working on this

[eps1lon]

Hi @eps1lon @sQVe is this issue up for grabs? I can give it a shot if you guys are not already working on this

Feel free to work on it. Please do let us know if you no longer work on the issue. If you're stuck you can share your progress so that others can help you.

[sarathps93]

Thanks @eps1lon. Is the suggested change of removing the warning for undefined only for useMemo or even for other hooks as well? I mean what about useCallback?

[sQVe]

@sarathps93 I would assume that this would be relevant for all hooks that use the deps api. It makes sense to allow to omit and set to undefined in all cases.

[sarathps93]

@eps1lon @sQVe I have raised a PR with a fix