@wordpress/data: Handle more return type values from a mapSelect argument on useSelect #16669

nerrad · 2019-07-18T23:11:01Z

Description

See #16612 for original report. The useSelect hook uses isShallowEqualObjects for comparing whether the new value returned from the mapSelect callback is equal to the previous value. See

gutenberg/packages/data/src/components/use-select/index.js

Lines 132 to 134 in a894b35

    
           if ( isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) { 
        
           	return; 
        
           }

I added some unit tests that demonstrated (when run against original useSelect) if a mapSelect callback returns a number or boolean primitive type, it will not get handled as expected by the useSelect hook. Oddly enough, array and string types did get handled fine. The same tests pass with the work done in this branch.

This pull adds a new utility function isEqual (which is not exposed on a public api so should be okay to use naming wise) for handling all primitive types and doing the appropriate comparison. This is then implemented in useSelect.

Fixes #16612.

How has this been tested?

New unit tests add coverage for this, and existing unit tests pass just fine.
Any serious problems should be picked up by e2e tests as well.

Types of changes

This is a non-breaking change that simply fixes a problem with expectations (especially considering the current documentation) around what a mapSelect callback given as an argument on useSelect can return. It can now return a primitive value, an array, or an object.

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.

nerrad · 2019-07-18T23:12:00Z

@epiqueras would love it if you could review this!

packages/data/src/components/use-select/test/index.js

nerrad · 2019-07-22T13:25:13Z

I don't think I've had this much back and forth on tests before 😃 ! All good though.

epiqueras · 2019-07-22T21:32:52Z

Tests are not only for validating logic and guarding regressions. They serve as living documentation of our code for folks who are adventuring into parts of the codebase that are unknown to them. I don't think they should have different quality standards than our other code.

That said, the test is looking a lot better after the initial iterations. There are still a few things I would change like the unnecessary .mockImplementation call that could be replaced by an inline definition in the spy's declaration and the confusing naming of some functions that aren't actually spies as spies. But I wouldn't say that these are blockers and the code is much easier to read now.

Thanks for taking care of this PR 😄

nerrad · 2019-07-22T21:47:47Z

Tests are not only for validating logic and guarding regressions. They serve as living documentation of our code for folks who are adventuring into parts of the codebase that are unknown to them. I don't think they should have different quality standards than our other code.

I 100% agree. My comment wasn't a complaint. It was more of an observation of the thoroughness of your review (hence the smiley). I probably could've worded better though.

swissspidy · 2019-07-24T09:20:02Z

packages/data/src/components/use-select/test/index.js

+				'undefined',
+				[ undefined, 42 ],
+			],
+		].forEach(


Why not use the each([...]).test approach that Jest supports natively?

Honestly, I just didn't think to use it :). It does look like the test could be switched to use it here but is it functionally any different? Are there some advantages to using it here?

Different reporting I think, and better ESLint support.

Interesting, good points and I'll give a go at updating the tests to use the built-in test conditions api in jest. It's also something I'm willing to do in a separate pull if this fix is needed to move #16460 along.

implemented in e2f6c28

epiqueras · 2019-07-24T10:10:38Z

I 100% agree. My comment wasn't a complaint. It was more of an observation of the thoroughness of your review (hence the smiley). I probably could've worded better though.

I didn't assume it was, I try to not make assumptions like that, because so much tone and context is lost through text. But I figured it wouldn't hurt to make my approach as transparent as possible 😄

aduth · 2019-07-24T14:41:24Z

packages/data/src/components/use-select/utils.js

+ */
+import isShallowEqual from '@wordpress/is-shallow-equal';
+
+const isEqual = ( valueA, valueB ) => {


Might be worth a brief JSDoc to explain the purpose we're seeking to accomplish with this function.

When I see isEqual, I think of a deep comparison. Might be good to specify what kind of equality this checks for.

Agreed to both points, I'll try and get this added today sometime.

We should just use the default export of the shallow equality package.

import isShallowEqual from '@wordpress/is-shallow-equal';

I am pretty sure it does the same thing.

doh ya, tests pass with just isShallowEqual... I'll update and great catch!

We should just use the default export of the shallow equality package.

Implemented in 642d672

We should just use the default export of the shallow equality package.

import isShallowEqual from '@wordpress/is-shallow-equal';

I am pretty sure it does the same thing.

It's true and I think it's fine to ship with it as-is (especially with tests to guard against regressions), and while technically it's the specified behavior, it's probably not exceedingly obvious that in code here we're intentionally anticipating to handle non-object/array values. It could be worth a clarifying comment to that end.

Aside: isShallowEqual is documented as requiring the arguments be passed:

gutenberg/packages/is-shallow-equal/index.js

Lines 15 to 16 in fcbdd20

* @param {(Array|Object)} a First object or array to compare.

* @param {(Array|Object)} b Second object or array to compare.

But it's tested to handle others:

gutenberg/packages/is-shallow-equal/test/index.js

Lines 136 to 147 in fcbdd20

it( 'falls back to strict equality when one of arguments is non-object-like', () => {

expect( isShallowEqual( undefined, {} ) ).toBe( false );

expect( isShallowEqual( undefined, [] ) ).toBe( false );

expect( isShallowEqual( undefined, undefined ) ).toBe( true );

expect( isShallowEqual( {}, null ) ).toBe( false );

expect( isShallowEqual( [], {} ) ).toBe( false );

expect( isShallowEqual( null, [] ) ).toBe( false );

expect( isShallowEqual( null, null ) ).toBe( true );

expect( isShallowEqual( 1, true ) ).toBe( false );

expect( isShallowEqual( true, true ) ).toBe( true );

expect( isShallowEqual( null, undefined ) ).toBe( false );

} );

It should probably allow any type.

Also adds tests covering the following return types from a selector map: - string, - array, - number, - boolean

fix spelling Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>

…llow-equal

- use native jest each api - improve naming of variables - simplify mocking and test setup.

…ment on useSelect (#16669)

nerrad added [Type] Bug An existing feature does not function as intended [Package] Data /packages/data labels Jul 18, 2019

nerrad requested review from aduth and youknowriad as code owners July 18, 2019 23:11

nerrad self-assigned this Jul 18, 2019

nerrad requested a review from epiqueras July 18, 2019 23:11

epiqueras reviewed Jul 19, 2019

View reviewed changes

packages/data/src/components/use-select/test/index.js Outdated Show resolved Hide resolved

packages/data/src/components/use-select/test/index.js Show resolved Hide resolved

nerrad requested a review from epiqueras July 19, 2019 13:23

epiqueras reviewed Jul 20, 2019

View reviewed changes

nerrad requested a review from epiqueras July 20, 2019 16:32

youknowriad mentioned this pull request Jul 22, 2019

Typewriter experience #16460

Merged

5 tasks

swissspidy reviewed Jul 24, 2019

View reviewed changes

aduth reviewed Jul 24, 2019

View reviewed changes

nerrad and others added 9 commits July 26, 2019 20:03

handle primitive return values from map select

4e58b2c

Also adds tests covering the following return types from a selector map: - string, - array, - number, - boolean

remove unnecessary mocks

962d61b

update tests to test object/null/undefined

914e863

modify test to conver null <=> undefined

4df3300

fix comment block

e95ffd1

Update packages/data/src/components/use-select/test/index.js

5a7e70d

fix spelling Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>

remove unneccessary concatenation

83e417b

remove unneccessary argument

e0920c5

remove unnecessary props/dependencies and rename argument

1169f9d

nerrad force-pushed the BUG/accept-more-return-types-for-use-select branch from f3fe2e0 to 1169f9d Compare July 27, 2019 00:04

nerrad added 2 commits July 26, 2019 22:04

replace unnecessary new util function with default export from is-sha…

642d672

…llow-equal

improve tests

e2f6c28

- use native jest each api - improve naming of variables - simplify mocking and test setup.

aduth approved these changes Jul 29, 2019

View reviewed changes

swissspidy changed the title ~~@wordpress/data: Handle more return type values from a mapSelect argument on useSelect (fixes #16612)~~ @wordpress/data: Handle more return type values from a mapSelect argument on useSelect Jul 31, 2019

swissspidy mentioned this pull request Jul 31, 2019

SandBox not re-rendered after HTML changes #16831

Closed

youknowriad merged commit ee3a375 into master Aug 1, 2019

youknowriad deleted the BUG/accept-more-return-types-for-use-select branch August 1, 2019 09:08

youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019

gziolo pushed a commit that referenced this pull request Aug 29, 2019

@wordpress/data: Handle more return type values from a mapSelect argu…

9570b2f

…ment on useSelect (#16669)

gziolo pushed a commit that referenced this pull request Aug 29, 2019

@wordpress/data: Handle more return type values from a mapSelect argu…

e1e483f

…ment on useSelect (#16669)

snyk-io bot mentioned this pull request Aug 13, 2024

[Snyk] Upgrade @wordpress/element from 5.28.0 to 6.3.0 Playgirlkaybraz11/wp-xeet#8

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@wordpress/data: Handle more return type values from a mapSelect argument on useSelect #16669

@wordpress/data: Handle more return type values from a mapSelect argument on useSelect #16669

nerrad commented Jul 18, 2019 •

edited by swissspidy

Loading

nerrad commented Jul 18, 2019

nerrad commented Jul 22, 2019

epiqueras commented Jul 22, 2019

nerrad commented Jul 22, 2019

swissspidy Jul 24, 2019

nerrad Jul 24, 2019

swissspidy Jul 24, 2019

nerrad Jul 24, 2019

nerrad Jul 27, 2019

epiqueras commented Jul 24, 2019

aduth Jul 24, 2019

ellatrix Jul 25, 2019

nerrad Jul 26, 2019

epiqueras Jul 26, 2019

nerrad Jul 27, 2019

nerrad Jul 27, 2019

aduth Jul 29, 2019

	if ( isShallowEqualObjects( latestMapOutput.current, newMapOutput ) ) {
	return;
	}

	* @param {(Array\|Object)} a First object or array to compare.
	* @param {(Array\|Object)} b Second object or array to compare.

	it( 'falls back to strict equality when one of arguments is non-object-like', () => {
	expect( isShallowEqual( undefined, {} ) ).toBe( false );
	expect( isShallowEqual( undefined, [] ) ).toBe( false );
	expect( isShallowEqual( undefined, undefined ) ).toBe( true );
	expect( isShallowEqual( {}, null ) ).toBe( false );
	expect( isShallowEqual( [], {} ) ).toBe( false );
	expect( isShallowEqual( null, [] ) ).toBe( false );
	expect( isShallowEqual( null, null ) ).toBe( true );
	expect( isShallowEqual( 1, true ) ).toBe( false );
	expect( isShallowEqual( true, true ) ).toBe( true );
	expect( isShallowEqual( null, undefined ) ).toBe( false );
	} );

@wordpress/data: Handle more return type values from a mapSelect argument on useSelect #16669

@wordpress/data: Handle more return type values from a mapSelect argument on useSelect #16669

Conversation

nerrad commented Jul 18, 2019 • edited by swissspidy Loading

Description

How has this been tested?

Types of changes

Checklist:

nerrad commented Jul 18, 2019

nerrad commented Jul 22, 2019

epiqueras commented Jul 22, 2019

nerrad commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epiqueras commented Jul 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nerrad commented Jul 18, 2019 •

edited by swissspidy

Loading