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

[rfc]: Externalize schema validation #342

Open
snewcomer opened this issue Mar 12, 2022 · 4 comments
Open

[rfc]: Externalize schema validation #342

snewcomer opened this issue Mar 12, 2022 · 4 comments

Comments

@snewcomer
Copy link
Collaborator

snewcomer commented Mar 12, 2022

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

ref adopted-ember-addons/validated-changeset#166

@snewcomer
Copy link
Collaborator Author

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

snewcomer commented Mar 13, 2022

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

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

snewcomer commented Mar 20, 2022

adopted-ember-addons/validated-changeset#166 (comment)

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

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

No branches or pull requests

1 participant