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

Error checking in select API #4901

Closed
wants to merge 3 commits into from
Closed

Error checking in select API #4901

wants to merge 3 commits into from

Conversation

abotteram
Copy link
Contributor

@abotteram abotteram commented Feb 6, 2018

Description

Updated select function to print an error when the called selector does not exist.

The reason I created this PR is because I want to make sure that plugins using the Gutenberg API will not interupt the javascript execution as much as possible. I also believe all other public APIs should have some error checking.

How Has This Been Tested?

Created unit tests and tested the select API in the browser.
Also improved unit tests to reload the data/index.js module for each test, so all module globals are reset, ensuring a clean test environement.

Screenshots (jpeg or gifs if applicable):

Types of changes

Improvement.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@aduth
Copy link
Member

aduth commented Feb 6, 2018

Noting that I'd like to explore assigning selector functions into an object produced by the select function, rather than specifying them as a second string argument of the function, which conflicts with the changes being proposed here.

@abotteram
Copy link
Contributor Author

abotteram commented Feb 6, 2018

I wasn't aware that the current api interface was still open for changes. I would like to point out, although it isn't pretty, the changes I made to the tests will allow for more consistent testing.

console.error( `Invalid selector called, with name "${ selectorName }" for reducer "${ reducerKey }".` +
' Make sure the reducerName and selectorName are correct!' );
return undefined;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add the same check line 103 or maybe reuse the same function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

console.error = jest.fn( () => {} );

expect( select( 'reducer1', 'selector1' ) ).toBeUndefined();
expect( console.error ).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -100,10 +100,6 @@ export const query = ( mapSelectorsToProps ) => ( WrappedComponent ) => {
};

return connectWithStore( ( state, ownProps ) => {
const select = ( key, selectorName, ...args ) => {
return selectors[ key ][ selectorName ]( state[ key ], ...args );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just noticed that #4823 broke this line, because getState() is not called on state[ key ].

@gziolo
Copy link
Member

gziolo commented Mar 23, 2018

Is it still relevant? There were a few major changes in the data codebase. It should be updated with the latest changes from master or closed otherwise. Thanks!

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 23, 2018
@abotteram abotteram closed this Mar 23, 2018
@abotteram abotteram deleted the update/error-checking-in-select-api branch March 23, 2018 08:33
@gziolo
Copy link
Member

gziolo commented Mar 23, 2018

By the way, we discussed that it might be a good idea to add some mechanism to prevent components to explode when withSelect or withDispatch try to access a method that isn't there. I'm not sure how challenging it is with the current code for select and dispatch because they simply return an object :)

@aduth
Copy link
Member

aduth commented Mar 23, 2018

We could always add a try / catch around the call to mapStateToProps (should really be called mapSelectToProps) and mapDispatchToProps:

const nextMergeProps = mapStateToProps( select, props ) || {};

const propsToDispatchers = mapDispatchToProps( dispatch, props );

@gziolo
Copy link
Member

gziolo commented Mar 23, 2018

We could always add a try / catch around the call to mapStateToProps (should really be called mapSelectToProps) and mapDispatchToProps:

Yes, totally on board with the idea 💯 Yes, let's rename as suggested 👍

@gziolo
Copy link
Member

gziolo commented Mar 23, 2018

By the way it, would be a good idea to have a helper similar to deprecated which would help to standardize the hint for the plugin developer, including the name of the plugin which caused issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants