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

or validator gives wrong error message #16

Open
banglashi opened this issue Aug 27, 2015 · 1 comment
Open

or validator gives wrong error message #16

banglashi opened this issue Aug 27, 2015 · 1 comment

Comments

@banglashi
Copy link

Hi @jagi

I added a or validator like you show in your example

Post.addValidator('title', Validators.or([
    Validators.null(),
    Validators.and([
      Validators.string(),
      Validators.minLength(5),
      Validators.maxLength(15)
    ])
  ]));

Problem is, if the field is filled in with less than 5 chars, I get following error message "The "title" field's value has to be null" Is this the intended behavior?

As I see it it should return the error message of minLength.

I created a repo for you to test here: https://github.com/banglashi/meteer-astronomy-validator-bug

Let me know.

@lukejagodzinski
Copy link
Owner

The problem is with validation order. The first validator it's going to execute is null and it's not null so why the error appears. The solution (for now) is to reverse the order:

Post.addValidator('title', Validators.or([
  Validators.and([
    Validators.string(),
    Validators.minLength(5),
    Validators.maxLength(15)
  ]),
  Validators.null()
]));

Now, everything should work.

Notice, that when working with forms and having validators order changed, there is no way to receive an error from the null validator. Because, even when the input field is empty it contains 0 length string, what causes validation to skip to the and validator. Of course, if you don't set any value on the field, then you would receive an error for the null validator.

Using this kind of nested validators is hard and error prone. Developers asked me to implement required attribute, so that's what I did for the next release. Now, you can forget about those awkward nested validators. However, I'm not removing nested validators, to be precise :).

In the new version your class will look like:

Post = Astronomy.Class({
  name: 'Post',
  collection: Posts,
  fields: {
    title: {
      type: 'string',
      required: false,
      validators: [
        Validators.string(),
        Validators.minLength(5),
        Validators.maxLength(15)
      ]
    }
  }
});

It will only be validated if the field's value is non empty.

One more important thing is that you may want to treat empty string from the form field as a null value (or lack of value). However, I think it's not the job of package to deal with it. I would rather recommend some check in the event.

var val = t.find('.js-title').value;
if (val.length) {
  t.post.title = t.find('.js-title').value;
}

However, I would not recommend doing so. Check for a null value is rather for a safety reason then for informing user that value can be null or a 5-15 characters long string.

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