Make eslint-plugin validate dependencies in useSelect and useSuspenseSelect hooks

[ecgan]

What?

Closes #49897. Builds on top of PR #24914 that was just merged recently in February 2023.

In this PR, we add useSelect and useSuspenseSelect as additional hooks in 'react-hooks/exhaustive-deps' eslint rule in eslint-plugin.

Why?

This helps us to declare all the dependencies when we use useSelect and useSuspenseSelect hooks. When we miss a dependency, there will be an eslint warning. It allows us to make use of the editor's quick fix command to quickly fix the dependency issue.

Testing Instructions

1. Find a useSelect hook with dependency. For example:

gutenberg/packages/block-library/src/search/edit.js

Lines 83 to 93 in 361f257

	const insertedInNavigationBlock = useSelect(
	( select ) => {
	const { getBlockParentsByBlockName, wasBlockJustInserted } =
	select( blockEditorStore );
	return (
	!! getBlockParentsByBlockName( clientId, 'core/navigation' )
	?.length && wasBlockJustInserted( clientId )
	);
	},
	[ clientId ]
	);

2. Add or remove an item in the dependency.
3. There should be an eslint warning. See example screenshot below with "missing dependency" warning when we remove the clientId in the dependency array:

5. Use the editor's quick fix command to automatically fix the warning.

[ecgan]

The Static Analysis GitHub Actions failed because it has an ESLint error with a useSelect usage with missing dependency: https://github.com/WordPress/gutenberg/actions/runs/4734770383/jobs/8404141142?pr=49900#step:5:606. The useSelect code usage was introduced in PR #23922.

It is an ESLint error, not a warning, because it is set to 'react-hooks/exhaustive-deps': 'error' for the components package in the following file:

gutenberg/.eslintrc.js

Lines 353 to 364 in 7b8395e

	{
	files: [
	// Components package.
	'packages/components/src/**/*.[tj]s?(x)',
	// Navigation block.
	'packages/block-library/src/navigation/**/*.[tj]s?(x)',
	],
	excludedFiles: [ ...developmentFiles ],
	rules: {
	'react-hooks/exhaustive-deps': 'error',
	},
	},

[Mamaduka]

The Static Analysis GitHub Actions failed because it has an ESLint error with a useSelect usage with missing dependency: https://github.com/WordPress/gutenberg/actions/runs/4734770383/jobs/8404141142?pr=49900#step:5:606. The useSelect code usage was introduced in PR #23922.

That's a very unusual use of the useSelect hook. I think the hook should just return __experimentalFetchLinkSuggestions, and then fetchLinkSuggestions should be implemented outside of the hook.

cc @WordPress/native-mobile

[ecgan]

I made an additional code change to fix the eslint error for now. See below.

[ecgan]

As per #23922 (comment), I have temporarily disabled eslint rule for this line to suppress eslint error for now.

[dcalhoun]

Disabling the linter error on mobile-specific violations has been done in the past. I think this makes sense in this case to unblock this PR. Thanks for the ping!

[ecgan]

Hi fellow reviewers @dcalhoun @gziolo @ntwb @nerrad @ajitbohra , may I have your review and approval on this PR please? 🙏

[nerrad]

Looks good to me. I don't see any new errors popping up when running linting JS locally and agree this should remain a warning for now in most contexts (although in general I think we should move towards this being an error for all hooks with dependency args at some point).

[dcalhoun]

LGTM. I did a quick test of the mobile Demo editor using an iPhone 14 Pro simulator, I did not encounter any issues. 🚀

Thanks again for involving the mobile team! 🙇🏻

[ecgan]

@nerrad @dcalhoun , thanks so much for the approval. Apparently I am not authorized to merge the PR (see screenshot below). Are you able to grant me the permission? I made a request in WordPress Slack to join the Gutenberg github team last week. Or perhaps you could help to merge this?

[nerrad]

@ecgan I just realized, one thing that is missing here is an update to the eslint plugin changelog. Will you submit a followup to take care of that please?

[ecgan]

@nerrad , sure, I'll take care of that. Thank you for checking on it. 🙂 🙏

[rowasc]

@nerrad @ecgan would it make sense to have a similar setup as the one used in the flaky test reporter to report open eslint warnings so that the transition time between introducing a "warn" rule and being able to enable "error" is potentially shorter (or at least easy to track)?

I'm not sure if it's worth it, but I can take a stab at it if this problem is something that comes up often enough and if tracking it would indeed be helpful (I'm not sure if it'd help or just be annoying :) )

[nerrad]

🤔 I think it depends on how many issues would get created - I see the flaky test-reported issues do get handled but there are also quite a few older ones still lingering around. Probably a good first step would be to see how many warnings exist in the codebase and if small enough, try to fix them in one (or a set of) PR(s).

[rowasc]

yea @nerrad - I got a little overwhelmed by warnings (106 by react-hooks/exhaustive-deps) so I figured it may be a big enough issue that it makes sense for it to be tracked, but I'm not familiar enough yet with how things are handled in Gutenberg to really know if that's "normal" or common enough. The downside of such a tool may be accidentally incentivizing "leaving warnings for later" as a behavior instead of incentivizing fixing them promptly.

edit: of course, right now we're more in the "leave it for later" situation since warnings are non-blocking for a merge.