Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define useSelect dependencies properly #19044

Merged
merged 1 commit into from Dec 11, 2019
Merged

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 10, 2019

Related #19007

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

@@ -45,7 +45,7 @@ export function __experimentalUseGradient( {
customGradient: attributes[ customGradientAttribute ],
gradients: getSettings().gradients,
};
}, [ clientId ] );
}, [ clientId, gradientAttribute, customGradientAttribute ] );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 10, 2019

Author Contributor

Were this missed before @jorgefilipecosta

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 12, 2019

Member

Yes I missed them, thank you for the fix 👍

@@ -548,7 +548,8 @@ function Button( { onClick, children } ) {
const SaleButton = ( { children } ) => {
const { stockNumber } = useSelect(
( select ) => select( 'my-shop' ).getStockNumber()
( select ) => select( 'my-shop' ).getStockNumber(),

This comment has been minimized.

Copy link
@ellatrix

ellatrix Dec 10, 2019

Member

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

@youknowriad youknowriad force-pushed the update/memoize-use-selecct branch from 4798369 to 7804572 Dec 10, 2019
Copy link
Contributor

epiqueras left a comment

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

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 11, 2019

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

This comment has been minimized.

Copy link
Member

gziolo commented Dec 11, 2019

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

@gziolo
gziolo approved these changes Dec 11, 2019
@youknowriad youknowriad merged commit ae1eccd into master Dec 11, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the update/memoize-use-selecct branch Dec 11, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 11, 2019

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.

@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.