Public API confusion

[vfonic]

Hi @aldeed,

Thanks for the great lib! I love it and I use it together with vazco/uniforms

I noticed that there are some things that were a little bit confusing to me. I just wanted to point it out, maybe it could be addressed in the next version.

1. Validation is a bit confusing. All fields are required by default. If you add optional: true, that makes the field optional. All the other validation-related properties do the opposite, if you specify it, it does some kind of validation, like max: 255 or custom(). I think required: true would feel better.
2. max: 255 validates max length for strings or the actual value for numeric field types. I'd keep max for numeric fields, but I'd change max to maxLength for strings just to avoid confusion. I prefer clarity (for reading) over brevity.
3. custom() is a property you define on the "root" of the field properties that it's referring to. For example:

  host: {
    type: String,
    optional: true,
    max: 255,
    custom() {
      // ....
    }
  }

Since it's just called custom, it's confusing whether it's customType or customValidation or something else. If it was nested under validation like so:

  host: {
    type: String,
    validation: {
      optional: true,
      max: 255,
      custom() {
        // ....
      }
    }
  }

...maybe it would be clearer.

4. Speaking of custom, it's also strange to have to have custom method for validation and any other validation. I'd expect to just have custom method take care of validation for this field. But in the current API, you have to do this:

  host: {
    type: String,
    // this property
    optional: true,
    custom() {
      // ....
    }
  }

5. It would be easier if I could define validation message right here, just like I can define label. Here's how I define validation message (that is only used in one place, for one field) for my custom validation right now:

  host: {
    // ...
  }
  port: {
    type: Number,
    min: 0,
    max: 65535,
    custom() {
      return this.value >= 0 && this.value <= 65535 ? undefined : 'Connection.port';
    },
    optional: true,
  },

Then, I have completely separate file (because it has to be loaded before any schemas are defined) where I say:

SimpleSchema.setDefaultMessages({
  messages: {
    en: {
      'Connection.port': 'Port must be between 0 and 65535'
    },
  },
});

It would be way easier if I could just return the error message as string:

  host: {
    // ...
  }
  port: {
    type: Number,
    min: 0,
    max: 65535,
    custom() {
      return this.value >= 0 && this.value <= 65535 ? undefined : 'Port must be between 0 and 65535';
    },
    optional: true,
  },

To sum it all up, here's how new API schema might look like:

const ConnectionSchema = new SimpleSchema({
  ...CommonSchemaFieds, // I think this should be possible in JS language for custom objects
  host: {
    type: String,
    customValidation(fieldValue, allFieldValues) {
      if (!URLChecker.valid(fieldValue)) {
        return 'Host must be a valid URL';
      }
    },
  }
  port: {
    type: Number,
    min: 0,
    max: 65535,
    validationError: 'Port must be between 0 and 65535',
  },
});

Thanks for reading through this! And thanks for the great library! I understand it took a lot of your time to write it and then to release v2 as well. It helps me a lot to speed up my development. ❤️ The above comments are just my opinion. Maybe I'm completely wrong. Feel free to ignore any comment you disagree with.

[aldeed]

Thanks for this write-up @vfonic

1. It's been debated before. I prefer required-by-default because it's stricter. If you forget required: true, you may not realize it for a while. But if you forget optional: true, you'll probably get errors right away to remind you. That said, those who prefer the required prop can have it. It's already supported; you just have to opt in: https://github.com/aldeed/simple-schema-js#required
2. Agree. It's just an old issue that I've never had time to address. I'd accept a PR to add maxLength and minLength for strings and deprecate (but keep working for now) max and min for strings. They can be removed in the next major version release.
3. True. I'm not sure about adding the validation layer since it would be a pretty big change, but I'd accept a PR to add customValidation as a synonym for custom and deprecate custom.
4. You are asking for a way to make custom run instead of built-in validation rather than in addition to it? I see what you mean. It's actually probably pretty easy since the lib just concats all validators and runs them. If we add a schema constructor option for this, it could be done with even more flexibility if it's something like: { validatorOrder: ["built-in", "custom-schema", "custom-global"] }. (That's the order in which they are currently run.) Doing the option this way would allow you to skip one or more types of validation or change the order in which they run. "built-in" could be expanded to list each built-in validator separately instead.
5. It may not be documented, but I believe it may work to have your custom function return { type: 'badPort', message: 'Port must be between 0 and 65535' }; If message is found on validation error objects, it uses that rather than the normal flow. However, I think it might make sense to support a validationMessages prop on individual fields, which could be merged into the global and schema-level messages.

[vfonic]

Thanks for the detailed reply! :)

1. Thanks! I missed this! I use it everywhere now. :) Is there a way to specify this as a "global" behavior through SimpleSchema.extendOptions or similar? You're right, people tend to forget to add required: true. However, I've mostly seen required as opposed to optional in other libraries. Here are some examples: https://www.npmjs.com/package/joi https://guides.rubyonrails.org/active_record_validations.html#presence https://www.npmjs.com/package/vee-validate Database validation would be: NOT NULL (aka required) Here's one other library I've found that has required by default (like SimpleSchema2 does): https://express-validator.github.io/docs/

2./3. I'll try to find some time to make couple of PRs.

4. Yeah, I think of custom as the only validation function when it's defined. But maybe it's better to leave it the way it is. If you don't specify any other validation properties, it will be the only validation function.

5. I'll have a look. Thanks!