jurassix / react-validation-mixin

Simple validation mixin (HoC) for React.
MIT License
283 stars 38 forks source link

Keep validation result in state #2

Closed ivan-kleshnin closed 9 years ago

ivan-kleshnin commented 9 years ago

Currently, validation may be called multiple times with the same data. For example, isValid may be called on submit and getValidationMessages(<field-name>) may be called n times for each field to get and render error messages.

isValid: function(field) {
  return ValidationFactory.isValid(this.validate(), field);
}

getValidationMessages: function(field) {
  return ValidationFactory.getValidationMessages(this.validate(), field);
},

Instead of returning validation messages it's probably better to keep them in state. This link may help: Reference implementation

jurassix commented 9 years ago

Thanks for the report.

Yup, I've been playing with different options to correct the current performance overhead of validating single fields verse validating all fields. I like the idea of caching the result set; I don't want to override any of the component lifecycle methods unless absolutely necessary.

ivan-kleshnin commented 9 years ago

I don't want to override any of the component lifecycle methods unless absolutely necessary.

I'ts common scenario to provide self-validation (in terms of required properties, not that validation) in componentDidMount, for example. I saw this use-case a lot from trusted React sources. React mixin's methods don't technically override each other, they chain. That's cool. I think mixins which implement "component lifecycle methods" are totally sane and ok.

If you just cache result set with some memoizing it will be hard or impossible to purge. So I'm afraid messing with form state is unavoidable.

jurassix commented 9 years ago

I didn't know that mixin's chained; good information.

jurassix commented 9 years ago

This is resolved and published to npm in version 2.1.0

I'm maintaining a cache of validation results and invalidating that cache automatically during the ComponentWillUpdate lifecycle. I've also exposed a manual clearValidations() api if you need to force a revalidation during a render cycle.

Validations are also lazily initialized; we only validate on the first call to isValid or getValidationsMessages. This now ensures only the first call has a processing overhead and all subsequent render calls are pulling from cache

ivan-kleshnin commented 9 years ago

Ah, ok. I always look into Releases tab and it somehow misses last ones. Final tag it shows is v2.0.2 :disappointed: I think you should give it a look.

jurassix commented 9 years ago

Fixed

ivan-kleshnin commented 9 years ago

I think I have a strong argument for this.state usage.

Besides of validation error messages there are other errors. Like "email is already used". Techically, it's not a validation error. And Joi or other validation libraries can't help with that, as such errors are app specific. But, as developer, you'll probably want to show them right there along with validation errors. It's quite possible to add secondary error messages. But it violates single source of truth principle for UI elements. So we'd be happy to have an option to merge custom error messages (obtained from app specific processes) with a react-validation-mixin validation messages.

If library uses this.state.errors it's simple. Just push whatever you want. If it's cached private var – it's both hard and hacky.

I think your consideration in avoiding state usage is to prevent potential conflicts. Keys with the same names may override which should lead to debugging hell... Well, luckily no. React is smart enough to prevent this. See http://stackoverflow.com/questions/23872374/is-initialstate-in-a-mixin-merged-with-initialstate-in-a-component

Yes, it's possible to merge the state from component A and the state declared in mixin M used by A if the states don't share keys. If they share keys, the Error "Invariant Violation: mergeObjectsWithNoDuplicateKeys()" will be thrown. PS: using React.js 0.9.0.


It's also worth mentioning that propTypes is also merged.

ivan-kleshnin commented 9 years ago

That's awesomely described: https://github.com/facebook/react/blob/0c6bee049efb63585fb88c995de788cefc18b789/src/core/ReactCompositeComponent.js#L47

jurassix commented 9 years ago

Thanks for the argument and research. I agree with you on using state to store the errors, caching on 'this' didn't seem right for React. I'll correct this in the next release.

jurassix commented 9 years ago

fixed in version 3.0.0

validation errors are now stored on the components state: this.state.errors

ivan-kleshnin commented 9 years ago

Wow, You're blazingly fast :)