ember-changeset-validations icon indicating copy to clipboard operation
ember-changeset-validations copied to clipboard

[rfc]: Externalize schema validation

Open snewcomer opened this issue 2 years ago • 4 comments

RFC

One broad goal we have is to potentially deprecate the use of ember-changeset-validations. Having both ember-changeset and ember-changeset-validations presents some confusion. If we went forward, ember-changeset would be the single library we would use to validate our data.

Currently, at a high level, the changeset API for validations is rather unintuitive and verbose.

e.g. ember-changeset-validations README

This addon updates the changeset helper by taking in a validation map as a 2nd argument (instead of a validator function).

Do we need this? Here is an example of the migration.

import lookupValidator from 'ember-changeset-validations';

const EmployeeValidations = {
  email: validateFormat({ type: 'email' }),
  password: [
    validateLength({ min: 8 }),
    validatePasswordStrength({ minScore: 80 })
  ],
  passwordConfirmation: validateConfirmation({ on: 'password' })
};

const changeset = new Changeset(this.model, lookupValidator(EmployeeValidations), EmployeeValidations);

Nominally, this could look like the following...

let schema = yup.object().shape({
  email: yup.string().email().required(),
  password: yup.string().required().min(8),
  passwordConfirmation: yup.string()
     .oneOf([yup.ref('password'), null], 'Passwords must match')
});

const changeset = new Changeset(this.model, schema);

As long as schema adhered to the interface we defined (something like isValid and/or validate methods), then we can detect errors in the current in progress changeset and error appropriately.

https://github.com/jquense/yup

Upsides

  • Less confusion in the changeset ecosystem
  • Simpler API

Downsides

  • New API to learn.
  • Another library to optionally install that supersedes our relatively simple validations provided - https://github.com/poteto/ember-changeset-validations/tree/master/addon/validators

ref https://github.com/validated-changeset/validated-changeset/issues/166

snewcomer avatar Mar 12 '22 04:03 snewcomer

One problem I'm seeing with yup is that is only returns one error for the schema

let schema = yup.object().shape({
  name: yup.string(),
  email: yup.string().email().required(),
  age: yup.number().min(18),
});
await schema.validate({ name: 'jimmy', age: 11 });

// ValidationError: age must be greater than or equal to 18

Table stakes would be all errors...for form rendering for example.

snewcomer avatar Mar 13 '22 14:03 snewcomer

Well I guess validationAt would work if we iterate over each key. Just need to make sure our string key a position format is the same. However, this would require us to discover all the keys in question (might be arbitrarily nested, arrays, etc)

snewcomer avatar Mar 13 '22 14:03 snewcomer

Ok yup abortEarly: false would give us errors. Would require user configuration.

Another option instead of internalizing validation and settings errors dependent on the result of validate, we could instead require the user to push in the errors. e.g. changeset.validate will call their ValidatorClass.validate function and give them errors they can specifically add to the changeset.

const errors = await changeset.validate();
errors.forEach((e) => changeset.addError(e));

That would give them the flexibility needed and decouples changeset from managing not only validations but creating errors as well.

snewcomer avatar Mar 14 '22 15:03 snewcomer

https://github.com/validated-changeset/validated-changeset/issues/166#issuecomment-1073159453

Will provide a new changeset. Something like SchemaChangeset or ValidatedChangeset and avoid deprecating this package for now.

snewcomer avatar Mar 20 '22 03:03 snewcomer