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

Remove validations from Frig #103

Open
jbinto opened this issue May 2, 2016 · 2 comments
Open

Remove validations from Frig #103

jbinto opened this issue May 2, 2016 · 2 comments
Milestone

Comments

@jbinto
Copy link
Contributor

jbinto commented May 2, 2016

We would like to remove validations from Frig.

There are two types of errors/validations in Frig:

  • Errors set programatically in the <Form errors={}> attribute, typically server-side errors (but could be anything)
  • Field-level, client-side validation errors set automatically by Frig validations, a.k.a. "Frig validations"

As for "Frig validations", users don't have a lot of control over these. We provide three very basic validations, required, min, and max. There is no support for custom validations.

"Frig validations" run in a way that is inconsistent with the <Form errors={}> attribute.

When a field changes, we run these validations. If there is an error, we display the error message next to the field. <Submit> will simply refuse to call Form's onSubmit if there are any errors.

There is no way to programatically access or manipulate these field-level, client-side errors. They also don't cascade up to the <FormErrorList> object. A form may have 10 (client side) invalid fields, but Form's props.errors would be empty.


We propose removing validations entirely, requiring consumers of Frig to implement their own custom validation in the onChange and onSubmit handlers.

Pros:

  • Clearer, more explicit, less magic
  • Full control, e.g. users would be able to control whether the form validates on change, on submit, or both
  • Field-level errors could be cascaded up to the <FormErrorList/>

Cons:

  • More boilerplate
  • Loss of declarative validations, e.g. <Input type="number" min="10" max="20" />
@SpiritBreaker226
Copy link
Member

Loss of declarative validations, e.g. <Input type="number" min="10" max="20" />

Not necessary because the validation could still happen, on the theme side, where all of the components could have individual validation. For example, the Number component could have a min and max validation, on its value, then display an error using the themes error list component.

@jbinto
Copy link
Contributor Author

jbinto commented Jun 8, 2016

Some early (non-binding!) thoughts on how this could work:

Option 1:

  • Handle everything in Form.onSubmit (or Form.onChange), completely ad-hoc, with no mention of validation.
  • This would let us plug in other validation libraries.

Option 2:

  • Don't "remove" validations per se, instead turn them into pluggable hooks
  • Provide a validate hook on <Input> and <Form>
  • For <Input>, validate would have a default implementation (defaultProps) as () => true
  • For <Form>, validate would have a default implementation (defaultProps) that returns true if all inputs validate, and false if any input fails to validate
  • In the form's submit handler, we won't call user-provided onSubmit unless all validate hooks return true

More ideas re: option 2:

  • The form-level validate callback would have a signature of (data).
  • The input-level validate callback might have a signature like (value, data). Most field-level validation would be done simply on value, but in the case of interdependent fields (e.g. password confirm) you might need access to the entire tree.
  • Since validations are simple predicates, we could include a few of them out-of-the-box, e.g.

{
  presence: (v) => v != null,
  min: (v, min) => v > min,
  max: (v, max) => v < max,
  fizzbuzz: (v) => v % 5 == 0 && v % 3 == 0,
}

Potential challenges:

  • Where to store state? Currently validations hold on to component local state state, which is a bad thing.
  • Currently the way Frig works is via props.data and props.onChange. A parent component is responsible for storing data somewhere (component local state, Redux store, etc) and also for updating it when Frig calls onChange.
  • Frig also takes props.errors, which is currently intended for use with server-side errors.
    • We could add a top-level onError hook to <Form>, which behaves just like onChange
    • i.e. Whenever there is an error on the form, send a JSON of all errors in a way that is compatible with Form.props.errors
    • Now the parent component can re-render, sending a new props.errors. It would be the parent component's choice/responsibility to either merge with existing (e.g. server-side) errors, or to overwrite them.
  • Caveat: This makes the setup curve for Frig a little higher. Frig would be completely stateless, but that state has to go somewhere. It feels like we need something like frigging-redux to tie this all together.

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

No branches or pull requests

2 participants