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

Some changes on validators #297

Closed
wants to merge 12 commits into from
Closed

Conversation

diegonvs
Copy link
Contributor

@diegonvs diegonvs commented Nov 14, 2017

shapeOf:

Instead returns a simple message containing a constraint in a shape validation, now returns an validation error that property in shape returns.

If shapeOf catches a error, validator() function can be called returning a error that others validators inside shapeOf have returned.

Example:

Error: Invalid state passed to 'icon.a'. Expected type 'object', but received type 'number'. Passed to 'ClayButton’.

arrayOf:

Instead return a simple constraint now returns a error showing that a index in array validator returns.

I’ve modified validateArrayItems() function that emits arrayOf error message, this function now returns a validator call in the same way of shapeOf and i’ve appended a string for show the error message on the error’s index.

Example:

Error: Invalid state passed to 'values'. Validator for values[2] says: "Error: Invalid state passed to 'values'. Expected type 'string', but received type 'number'. Passed to 'ClayDropdown'. " Passed to 'ClayDropdown'. 

'Warning':

I've removed warning string fragments to compose a message more clean instead of:

Warning: Error: Warning: Invalid state passed to 'label'. Expected type 'string', but received type 'number'. Passed to 'ClayBadge'. 

After changes will be like:

Error: Invalid state passed to 'label'. Expected type 'string', but received type 'number'. Passed to 'ClayBadge'. 

Some fixes:

On solving these problems, i’ve noticed that shapeOf validation doesn’t verify shape(argument of function), causing that developer only receive a feedback of bad using this function on instantiate a new instance of metal. I had solved these problem validating arguments before executing them like, but i think this is not appropriated handling for devs.

I’m searching for enhance this handling. What do you think guys?

@diegonvs diegonvs changed the title SF template literals Some changes on validators Nov 14, 2017
@diegonvs diegonvs changed the base branch from master to develop November 14, 2017 21:53
@diegonvs
Copy link
Contributor Author

/cc @jbalsas @Robert-Frampton

@@ -61,6 +63,10 @@ const validators = {
* @return {!function()}
*/
objectOf: function(validator) {
const validatorResult = validators.func(validator);

Choose a reason for hiding this comment

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

Hey @diegonvs

I noticed that in arrayOf and objectOf you validate the args outside the maybe function, but in shapeOf it's done inside the maybe function. This will result in two different behaviors.

With how it is currently, validators.arrayOf(2) will return an error, but validators.shapeOf(2) will not.

I'm pretty sure we want the behavior of validators.shapeOf, since returning an error immediately when using the Config helpers won't actually print the error. For example,

const config = Config.arrayOf(2); // TypeError: Cannot read property 'validator' of undefined

Copy link

@brunobasto brunobasto Nov 24, 2017

Choose a reason for hiding this comment

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

Hey @Robert-Frampton,

One thing confusing about that validation in particular is that it's a different scenario than the others. Validations inside the maybe will only get thrown once the user of your component is trying to pass an invalid argument to a components state property.

The error thrown by

MyComponent.STATE = {
    items: Config.arrayOf(2);
}

on the other hand, is more targeted towards component developers instead of component users. And should probably be thrown upon component definition (as soon as possible) instead of waiting for component instantiation.

I'm pretty sure we want the behavior of validators.shapeOf, since returning an error immediately when using the Config helpers won't actually print the error.

One alternative would be throwing an error right away instead if returning it.

Choose a reason for hiding this comment

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

I think throwing the error immediately makes more sense to me. In that case I'm fine with it, though I would like to change the error message that is thrown. The message returned from validators.func is confusing when you don't pass the additional name and context args to it.

Choose a reason for hiding this comment

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

@diegonvs, do you have time to make those changes? If not I can do it, just trying to get ready for a release later this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @Robert-Frampton, i'm currently working on domData get/set functions these days. Probably this feature will enter on this week release :) You can take those changes in validators?

@robframpton
Copy link

Resent at #315

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants