tgriesser / checkit

simple, flexible validations for node and the browser
MIT License
223 stars 53 forks source link

allow promise based custom validators utilize custom messages #71

Open morrelinko opened 8 years ago

morrelinko commented 8 years ago

Currently the only way to trigger a custom validation failure that utilizes "promises" is to explicitly throw new Error() with a message and this does not play well on the off chance you want to utilize custom messages or localized error messages. The code below demonstrates how its currently done.

Checkit.Validator.prototype.unused = function() {
  return knex(table).where(column, '=', val)
    .andWhere('id', '<>', this._target.id)
    .then(function(resp) {
      if (resp.length > 0) {
        throw new Error('The ' + table + '.' + column + ' field is already in use.');
      }
    });
};

The changes made allows you to do this

Checkit.i18n.en.messages.unsued = '{{label}}: {{var_1}} is already in use.';

Checkit.Validator.prototype.unused = function() {
  return knex(table).where(column, '=', val)
    .andWhere('id', '<>', this._target.id)
    .then(function(resp) {
      if (resp.length > 0) {
        return false; // Changes
      }
    });
};

Returning a falsey value will either use custom rule object message or localized message (default behavior without promises).

rhys-vdw commented 8 years ago

this does not play well on the off chance you want to utilize custom messages or localized error messages

Could you explain the problem?

morrelinko commented 8 years ago

@rhys-vdws if you return a promise in a custom validation rule, the only way to cause a failure is to throw an Error() object and you have to hard code an error message which makes it impossible to utilize localization in this scenario.

Take this setup for instance. Returning a falsey value will cause a validation failure and use the 'unused' message defined in i18n.

Checkit.i18n.en.messages.unsued = '{{label}}: {{var_1}} is already in use.';

Checkit.Validator.prototype.unused = function() {
  return false;
};

But when you return a promise, the only way to cause a failure is to trigger an exception. Notice how its you can't use the i18n message since you have to hard code the error message.

Checkit.i18n.en.messages.unsued = '{{label}}: {{var_1}} is already in use.';

Checkit.Validator.prototype.unused = function() {
  return Promise.resolve(true).then(function() {
    throw new Error('Failure message');
  });
};
rhys-vdw commented 8 years ago

Sounds good @morrelinko, thanks for the explanation.

  1. Address that line note.
  2. Add at least 1 test covering this use case.
  3. Update documentation to explain that you can return a Promise resolving to false.
  4. Add a new heading at the top of "change log" called "next release" with this change documented.

Then I merge immediately and do an npm release with the changes.