Define useSelect dependencies properly

[youknowriad]

Related #19007

I was hoping that by fixing the dependencies isssues we have in useSelect, we'd notice impact on peformance but nothing :) (weird)

[youknowriad]

Were this missed before @jorgefilipecosta

[jorgefilipecosta]

Yes I missed them, thank you for the fix 👍

[ellatrix]

Looks like the there are spaces in the line above and tabs on this line.

[epiqueras]

This was hard to review. Can we add the lint rule to be safe?

[gziolo]

I was hoping that by fixing the dependencies isssues we have in useSelect, we'd notice impact on peformance but nothing :) (weird)

Yes, it should have some impact. I wrote unit tests to ensure that this memoization works in the first place. It seems like it does:

diff --git a/packages/data/src/components/use-select/test/index.js b/packages/data/src/components/use-select/test/index.js
index 1aa2ad0d7..5e6741d35 100644
--- a/packages/data/src/components/use-select/test/index.js
+++ b/packages/data/src/components/use-select/test/index.js
@@ -27,6 +27,86 @@ describe( 'useSelect', () => {
 		return <div>{ data.results }</div>;
 	};
 
+	it( 'does not memoize mapSelect when no deps passed', () => {
+		let renderedTimes = 0;
+		let calledTimes = 0;
+		const TestComponent = () => {
+			const { result } = useSelect( () => {
+				calledTimes++;
+				return {
+					result: calledTimes,
+				};
+			} );
+			renderedTimes++;
+			return <div>{ result }</div>;
+		};
+
+		let renderer;
+		act( () => {
+			renderer = TestRenderer.create(
+				<TestComponent keyName="foo" test={ 1 } />
+			);
+		} );
+
+		act( () => {
+			renderer.update(
+				<TestComponent keyName="foo" test={ 2 } />
+			);
+		} );
+
+		act( () => {
+			renderer.update(
+				<TestComponent keyName="foo" test={ 3 } />
+			);
+		} );
+
+		expect( renderedTimes ).toBe( 4 );
+		expect( calledTimes ).toBe( 5 );
+		expect( renderer.root.findByType( 'div' ).props ).toEqual( {
+			children: 5,
+		} );
+	} );
+
+	it( 'memoizes mapSelect when an empty array as deps passed', () => {
+		let renderedTimes = 0;
+		let calledTimes = 0;
+		const TestComponent = () => {
+			const { result } = useSelect( () => {
+				calledTimes++;
+				return {
+					result: calledTimes,
+				};
+			}, [] );
+			renderedTimes++;
+			return <div>{ result }</div>;
+		};
+
+		let renderer;
+		act( () => {
+			renderer = TestRenderer.create(
+				<TestComponent keyName="foo" test={ 1 } />
+			);
+		} );
+
+		act( () => {
+			renderer.update(
+				<TestComponent keyName="foo" test={ 2 } />
+			);
+		} );
+
+		act( () => {
+			renderer.update(
+				<TestComponent keyName="foo" test={ 3 } />
+			);
+		} );
+
+		expect( renderedTimes ).toBe( 4 );
+		expect( calledTimes ).toBe( 2 );
+		expect( renderer.root.findByType( 'div' ).props ).toEqual( {
+			children: 2,
+		} );
+	} );
+
 	it( 'passes the relevant data to the component', () => {
 		registry.registerStore( 'testStore', {
 			reducer: () => ( { foo: 'bar' } ),

An unrelated question to this PR:
Is it expected that useSelect is called more times than the render method?

[gziolo]

I couldn't find any docs about configuring ESLint rule react-hooks/exhaustive-deps in https://github.com/facebook/react/tree/master/packages/eslint-plugin-react-hooks#eslint-plugin-react-hooks but I found unit tests which do the following:

https://github.com/facebook/react/blob/9ac42dd074c42b66ecc0334b75200b1d2989f892/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js#L260

We should enable this rule in general and for useSelect in particular as discussed in other places. Well, assuming it's useful 🙃 /cc @aduth

[epiqueras]

Is it expected that useSelect is called more times than the render method?

Not useSelect, but the mapping function is called twice sometimes.

We should enable this rule in general and for useSelect in particular as discussed in other places.

Yes, we had it before, not sure why it was removed.

The reason this PR doesn't generate significant performance improvements is because most of the deps added weren't changing often, or were changing due to a store update, which would also trigger a mapSelect call. To see more significant improvements we will need more granular store subscriptions.