Major refactoring

[ivan-kleshnin]

Please review this and let's discuss :turtle: I'm trying to address some lurking problems with current implementation.

1) eager vs lazy validation. I think this should be up to user to decide whether to revalidate or not. For now isValid may invoke validation, while getValidationMessages may not... this is too confusing and involved. In general, if something require comments, it's probably no so obvious.

So I put validation in validate and isValid ang getValidationMessages now just check / return against current validation state (this.errors). Mixin users should develop their own flows on top of this IMO.

2) Tests used shared semi-global mutable variables. This is antipattern. It's better to redeclare arguments in functions as it's more explicit, more obvious, and protected from bugs with mutable state.

3) There were some unnecessary tests which actually tests Joi and not mixin... I also restructured test sections to remove valid vs invalid distinction. When we mix valid and invalid input (and you did that in some cases) what section should it belong it? In my experience, It's inconvenient classification style so I replaced it.

4) Mix of expect and should style assertions. I played with them a bit and found that expect provide much more readable messages in some cases. Also they are more portable. So I replace should styled assertions with expect styled.

5) Inconsistent terminology in code (legacy artefacts like validations vs errors). Cleaned them up.

6) Inconsistent terminology in docs. Especially field when you mean form field and state key in the same sentence. It may be not obvious, but I put comparatively a lot of time in making docs clear in this version.

7) Other fixes, hopefully following best practices.

We may split this in chunks again, It's not "all or nothing" PR. I just wanted to implement everything at once, while I had free time to do this and to present the whole picture :)

[jurassix]

Thanks. Traveling today will review and let u know.

[jurassix]

Thanks for your contributions; glad to have such an active contributor on this project!

I will need some time to go over all the changes you've introduced, and examine the tests, etc. I have a couple of inline comments I'll make now that have stood out to me.

[jurassix]

Added lots of comments. Some may be non-starters, since I haven't tested or played with the pull request.

Thanks again for the pull request.

[ivan-kleshnin]

First. I am so confident with the code just because this PR is mostly intent declarations to discuss things :)

handleValidation

I've removed this one as, at the moment, it does nothing without flag which is off by default. Maybe with flagged behavior by default it might be useful. Though I'm not a fan of boolean flags. There is opinion that boolean flags are always a code smell and I'm rather agree. Boolean flag signs two methods are squashed in one. Anyway it's just "sugar" and doesn't change the whole picture. I basically wanted to minimize everything except core things.

So on Submit the user would call if(this.validate()){...}? React docs for setState: setState() does not immediately...

Yes. It seems a well-known issue: https://github.com/facebook/react/issues/122 (and there are many other related). To summarize, general opinion of people is something like:

React had to make this.state "pending state" (sync) and not "rendered state" (async) but it is hard to impossible to fix this in backward compatible way

Lets think about whether it's a problem or not. There is one workaround:

My current hack is to update this.state directly and call this.forceUpdate later.

The downside: it will bypass shouldComponentUpdate...

As React does not return a promise on this.setState it's basically impossible to glue this all together in a sane way. So, in my opionion, it should just be documented that users have to rely on this.validate returned result and not call this.isValid in the same method. It's better to return exact messages from this.validate instead of boolean back then. Because it's React behavior (like it or not), I think it's better to accept it and document it than to work-around. Documentation may also prevent other bugs for people not aware of it.

By the way, I don't think if (validate) will be a common use-case. validate will be called in action handlers, while isValid / getMessages will be called in render. They should not overlap often.

should we also allow validatorTypes to be undefined? I think it's more user friendly to not blow up with the addition of a mixin. Thoughts?

I don't have any strong opinion on this. Not enough experience to weigh. I just throw it to not forgot that such option exists :) Maybe it's better to suspend it for a while.

[ivan-kleshnin]

This will simply return a list of error messages for this key, I feel it should maintain the same results structure when provided a key or not. The below snippet code will maintain the structure of [key: [messages...]}

This is very interesting. As you've probably saw I refactored many things to ban undefined and to keep results and even keys consistent. This is the most prominent example:

validate: function(joiSchema, data, key) {
    ...
    var errors = this.format(Joi.validate(data, joiSchema, joiOptions));
    if (key === undefined) {
      Object.keys(joiSchema).forEach(function(key) { // <-------
        errors[key] = errors[key] || [];
      });
      Object.keys(data).forEach(function(key) { // <-------
        errors[key] = errors[key] || [];
      });
      return errors;
    } else {
      return errors[key] || [];
    }
  },

Functions with consistent arguments and return values are (more) composable and predictable (note for random visitor). I kept your approach to key argument untouched. Is it a good idea to always return {<key>: <errors>} structure independently of key argument? The downside is that client code became more complex:

errorsForKey = this.validate(...key)
vs
errorsForKey = this.validate(...key)[key]

And the same logic should be applied to getValidationMessages than. I may be wrong, but as the second pattern is going to spread in many places I think we don't get any benefit from this exact sort of "normalizing" (code just lurks to client, not disappear).

[ivan-kleshnin]

There is second counter-argument. Imagine a scenario where user calls console.log(errors) down below the validate call and wonders why validators don't work. He's observing {key1: [], key2: [], key3: [errors]} structure and not {key1: [], key2: [errors], key3: [errors]} he's expecting. And he just forgot about that little key argument he put above.

[ivan-kleshnin]

However everything above do not invalidate your criticism. Return result is still rather complex & inconsistent at the same time. There is another opportunity to consider. Splitting validate, isValid, getValidationMessages to keyed and non-keyed variants.

// implies mixin is only about forms (limits us forward)
this.validateForm()     -> {<key>: <errors>}
this.validateField(key) -> [<errors>]
vs 
// seems ok, maybe a bit too generic?
this.validateAll() 
this.validateOne(key) || this.validateSingle(key)
vs
// seems ok. Underlines we validate state. Good or not?
this.validateState() 
this.validateStateKey(key) 

I replaced field argument with key to forward handle other use-cases. Mixin may possibly be used to validate objects coming from backend (3rd party servers etc.), e.g. not forms & fields at all. And implied distinction between "field as component" vs "field as fieldname" is confusing.

[jurassix]

Haven't forgot about you ;) I'm at ReactJS conference this week, it's a blast! Will pick this up either over the weekend or next week.

[ivan-kleshnin]

Good luck! :thumbsup:

[jurassix]

Thanks again for you work on this project. The code is looking good!

I modified this pull request and merged it into master as part of a minor release: 3.1.0

I am keeping the original validate implementation, and have added back in the handleValidation method (but is simpler), to cut down on developer boilerplate. I also am keeping isValid the original impl, so we can validate the entire form for the user if no key is provided and report of the forms validity. I've updated the README to not mention 'lazy' vs 'eager' impl; I think those were good suggestions, and were confusing. All your tests and code clean up were used whole sale. I've also added a new API validatorData that allows the developer to specify where the data is coming from; this allows me to validate props, state, and a combination of the two. This is useful in FLUX and other use cases where state is not being used to manage data changes.

Thanks again for the contributions.

[jurassix]

I added a demo to the README.md

The remaining open item to discuss is the behavior of isValid

In my example component, I would only call isValid() with zero args when I want to know the validity of the entire form; to block a submit. But I also think that it may be confusing to the end user that a zero arg call is fundamentally different than a single arg call; but my example should guide them how to use this lib.

Do you have different examples than the ones I have provided? Or a different flow/behavior that isn't exemplified in the demo ?

[ivan-kleshnin]

Hi! Thanks for the response. I'm out of work until Saturday. I'll give it a look then.

[ivan-kleshnin]

I reviewed your changes. Everything looks great, you really pushed everything one more step forward. The only thing I still concerned is, yeah, isValid().

Regarding your signup example. This should be symmetrical IMO:

onBlur calls handleValidation('email') (handleValidation is confusing name, see below) handleValidation calls validate('email')

onSubmit calls handleSubmit() handleSubmit calls isValid() which calls validate() -- current or handleSubmit calls validate() -- should be imo

Transition to simple isValid will change client code like this:

handleSubmit: function(event) {
  event.preventDefault();
  if (this.isValid()) {
    this.setState({
      feedback: 'Form is valid send to action creator'
    });
  } else {
    this.setState({
      feedback: 'Form is invalid'
    });
  }
},

handleSubmit: function(event) {
  event.preventDefault();
  this.validate();
  // "feedback" stuff should evaluated in `render`
}

same length, less magic. And, I believe, you'll want to ship your version of handleSubmit at some point of time with handleValidation -> handleUnfocus

[ivan-kleshnin]

I imagine library code like this:

handleUnfocusFor: function(key) {
  return function handleUnfocus(event) {
    event.preventDefault();
    this.validate(key);
  }.bind(this);
},

handleReset: function(event) {
  event.preventDefault();
  this.setState(this.getInitialState());
},

handleSubmit: function(event) {
  event.preventDefault();
  this.validate();
},

And client code like this:

<div className='form-group'>
  WAS: <h3>{this.state.feedback}</h3>
  NOW: {this.isValid() ? '': <h3>Form is not valid</h3>}
</div>

Messages (design / content aspect) also belong to render now, which is good.