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

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

Merged
merged 11 commits into from Aug 1, 2019

Conversation

@nerrad
Copy link
Contributor

commented Jul 18, 2019

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

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 nerrad requested review from aduth and youknowriad as code owners Jul 18, 2019

@nerrad nerrad self-assigned this Jul 18, 2019

@nerrad nerrad requested a review from epiqueras Jul 18, 2019

@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@epiqueras would love it if you could review this!

@nerrad nerrad requested a review from epiqueras Jul 19, 2019

@nerrad nerrad requested a review from epiqueras Jul 20, 2019

@youknowriad youknowriad referenced this pull request Jul 22, 2019
5 of 5 tasks complete
@nerrad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

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

@epiqueras

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

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.

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

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 24, 2019

Member

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

This comment has been minimized.

Copy link
@nerrad

nerrad Jul 24, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@swissspidy

swissspidy Jul 24, 2019

Member

Different reporting I think, and better ESLint support.

This comment has been minimized.

Copy link
@nerrad

nerrad Jul 24, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@nerrad

nerrad Jul 27, 2019

Author Contributor

implemented in e2f6c28

@epiqueras

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

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 😄

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

const isEqual = ( valueA, valueB ) => {

This comment has been minimized.

Copy link
@aduth

aduth Jul 24, 2019

Member

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

This comment has been minimized.

Copy link
@ellatrix

ellatrix Jul 25, 2019

Member

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

This comment has been minimized.

Copy link
@nerrad

nerrad Jul 26, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Jul 26, 2019

Contributor

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.

This comment has been minimized.

Copy link
@nerrad

nerrad Jul 27, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@nerrad

nerrad Jul 27, 2019

Author Contributor

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

Implemented in 642d672

This comment has been minimized.

Copy link
@aduth

aduth Jul 29, 2019

Member

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:

* @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:

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.

nerrad and others added 9 commits Jul 18, 2019
handle primitive return values from map select
Also adds tests covering the following return types from a selector map:
- string,
- array,
- number,
- boolean
Update packages/data/src/components/use-select/test/index.js
fix spelling

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

@nerrad nerrad force-pushed the BUG/accept-more-return-types-for-use-select branch from f3fe2e0 to 1169f9d Jul 27, 2019

nerrad added 2 commits Jul 27, 2019
improve tests
- use native jest each api
- improve naming of variables
- simplify mocking and test setup.
@aduth
aduth approved these changes Jul 29, 2019

@swissspidy 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

@youknowriad youknowriad merged commit ee3a375 into master Aug 1, 2019

1 of 4 checks passed

Milestone It Milestone It
Details
Milestone It Milestone It
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad deleted the BUG/accept-more-return-types-for-use-select branch Aug 1, 2019

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

gziolo added a commit that referenced this pull request Aug 29, 2019
gziolo added a commit that referenced this pull request Aug 29, 2019
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.