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

Data: Fix wp.data.query backwards compatibility with props #5220

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 23, 2018

Related: #5137

This pull request seeks to resolve an issue where the wp.data.query backwards-compatibility added with the introduction of wp.data.withSelect does not correctly pass props. In the course of revisions to #5137, it was overlooked to use the final signature of ( select, props ) on withSelect. No errors occurred only because none of the core components had used the ownProps argument of the query higher-order component.

Testing instructions:

Ensure unit tests pass:

npm run test-unit

As of #5215, no core components use wp.data.query, so this cannot be easily tested in the browser without manual changes.

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Feb 23, 2018
registerReducer( 'reactReducer', () => ( { reactKey: 'reactState' } ) );
registerSelectors( 'reactReducer', {
reactSelector: ( state, key ) => state[ key ],
function cases( withSelectImpl, extraAssertions = noop ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the tests themselves have changed, just putting in a function to allow testing both withSelect and query separately with same expectations (except console.warn in the latter).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name is unclear to me :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name is unclear to me :)

I broke my own rule of no abbreviations 😄 It's a semi-common convention for "Implementation". I've unabbreviated it.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@aduth aduth merged commit 6b47489 into master Mar 2, 2018
@aduth aduth deleted the fix/query-deprecated branch March 2, 2018 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants