Add react-hooks/exhaustive-deps lint rule

[adamziel]

Description

As mentioned in the docs, when using hooks it's worth checking if deps array is exhaustive. This is done by react-hooks/exhaustive-dep eslint rule. This PR adds that rule. It immediately detected numerous violations, most of the time with a pretty good explanation:

62:2 warning The 'toggleEventBindings' function makes the dependencies of useEffect Hook (at line 54) change on every render. Move it inside the useEffect callback. Alternatively, wrap the 'toggleEventBindings' definition into its own useCallback() Hook react-hooks/exhaustive-deps

function ObserveTyping( { children, setTimeout: setSafeTimeout } ) {
	// ...
	useEffect( () => {
		toggleEventBindings( isTyping );
		return () => toggleEventBindings( false );
	}, [ isTyping ] );
	// ...
	function toggleEventBindings( isBound ) {

Let's use this PR as a place to discuss whether or not we should be using that rule.

How has this been tested?

Tested locally.

Types of changes

Non-breaking changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[aduth]

@youknowriad I seem to recall that you had some objections to this previously?

[gziolo]

There is PR where we @youknowriad added missing dependencies to the usage of custom hooks: #19044. I can’t find the discussion about the rule you propose but it might have something to do with giving false negatives for referenced pure functions and similar cases that the rule doesn’t process (or used to) properly.

[youknowriad]

I'm not against adding this rule but I'm pretty certain this breaks a lot of existing hooks, so we need to be careful when "fixing" the lint error to ensure the desired behavior is retained. Sometimes it means refactoring the code I believe.

[aduth]

Not sure if it's 'warn' for the purpose of initial discussion, but we shouldn't want to introduce this to the codebase as a warning. It will be ignored.

Related: #18458

[adamziel]

Closing for now. I am not going to pursue merging this PR for the time being and we can always reopen later.