Skip to content
This repository has been archived by the owner on May 4, 2020. It is now read-only.

Keep validation result in state #2

Closed
ivan-kleshnin opened this issue Jan 23, 2015 · 11 comments
Closed

Keep validation result in state #2

ivan-kleshnin opened this issue Jan 23, 2015 · 11 comments
Assignees
Labels

Comments

@ivan-kleshnin
Copy link
Contributor

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 jurassix added the bug label Jan 23, 2015
@jurassix jurassix self-assigned this Jan 23, 2015
@jurassix
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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

@jurassix
Copy link
Owner

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
Copy link
Contributor Author

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

@jurassix
Copy link
Owner

Fixed

@ivan-kleshnin
Copy link
Contributor Author

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
Copy link
Contributor Author

@jurassix
Copy link
Owner

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 jurassix reopened this Jan 24, 2015
@jurassix
Copy link
Owner

fixed in version 3.0.0

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

@ivan-kleshnin
Copy link
Contributor Author

Wow, You're blazingly fast :)

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

No branches or pull requests

2 participants