Skip to content

allow promise based custom validators utilize custom messages #71

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

morrelinko
Copy link

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

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

morrelinko commented Jun 28, 2016

@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');
  });
};

pending.push(processItemAsync(runner, validation, key, context).catch(addError(errors, key, validation)))
pending.push(processItemAsync(runner, validation, key, context)
.then(function(result) {
if (_.isBoolean(result) && result === false) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_.isBoolean is redundant here. result === false is suffient (since false is of course boolean).

@rhys-vdw
Copy link
Collaborator

rhys-vdw commented Jun 29, 2016

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.

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

Successfully merging this pull request may close these issues.

2 participants