String required and min

[orokos]

Having an odd issue, version 0.10.1 with react-formal.

Using both required() and min() on a string input so that we get a different message for each. Expected is if the length is 0, should show required, otherwise if it is less than min then show min.

If you enter the field and leave without typing, it will have no value set and validates as {message: "Enter something", type: "required"} but if you go back and type something but erase it so that you have the empty string, you get {message: "Enter something", type: "min"}. The min message is something else entirely.

For some reason this appears to change the way required() is working and interferes with the min() check. Depending on the order that they are defined in the schema, you will get just the one but not the other. Probably to do with being exclusive and now being the same "type"?

[jquense]

I'm not quite sure from your explanation but I think the issue is the underlying value. is probably undefined or null the first time whereas its and empty string "" the second time around. min/max ignore undefined/null values but do operate on empty strings.

specifically tho, the string "required" is implemented as min(1) for empty string values: https://github.com/jquense/yup/blob/master/src/string.js#L31-L33 which is what's causing the differences in type on the error.

I'm not sure if this is a bug or working as intended...the type prop was added later, so this is a bit unintended, but I am not sure its wrong. What exactly about this behavior is causing issues for you? It's helpful to know the use case before I decide to change something :)

[orokos]

Yeah, that would explain the behaviour.

The problem is that you get the failure on required as type "min" which seems to break if you have an actual min. This is similar to another issue where an empty string is not considered absent, and it gets a little odd when you have form inputs. If you edit a form input and then delete everything, you get an empty string, and unless the field is required() it would be useful to allow that. Having rules that are mandatory to meet but only if a value is provided are common. An optional email field, setting a password with a minimum length but only if one provided, etc.

This is how it is handled in other input validators I have worked with.

It also is inconsistent for the error type the way it is implemented. The min(1) when a string is present (empty string or not) means that you get the type change I mentioned. That means I can't tell if this is my required() validation failing or the min() validation failing, since the required() one can result in type "required" but also "min" and on an html input both states look the same.

Thanks.

[jquense]

I think I agree with you on this...or at least the current behavior is inconsistent, and the empty string check should be done in terms of min(), since that would override other min()s checks further up the schema chain. I'll see if I can fix it soon.

In terms of whether min/max should also ignore empty strings, that I am a bit more torn about. The current behavior is consistent with how number().min() works for example, where 0 is not an empty or absent value. Generally I think that clearly signaling empty values with null or undefined ofters more flexibility and clarity when using custom or complex input components. You do bring up a good point about normal form inputs, though. Empty strings in those cases are almost always "absent" values, and not a "short" value or something. That being said, that problem I think properly belongs to the form validation library in this case (react-formal) and not the schema library, which has more than form validation to consider in its use cases.

[orokos]

I did the following to coerce things to be consistent with form inputs (treat '' as undefined)

    first: yup.string().trim().transform(value => value === '' ? undefined : value)
      .required(`What's your first name?`)
      .min(ValidationConstants.PROFILE.minNameLength, `This has to be at least ${ValidationConstants.PROFILE.minNameLength} characters long.` )
      .max(ValidationConstants.PROFILE.maxNameLength, `You can't exceed ${ValidationConstants.PROFILE.maxNameLength} characters long here.`),

This appears to give behaviour that a user would expect, the transform() seems to work fine but is rather ugly. Now if I drop the required() off the field then I do NOT trigger the min() which was my original expectation. I can understand not treating it the same, since this isn't strictly for form inputs, but it seems rather cumbersome for something that is likely validating forms. :)

Is this something that would be better to address in react-formal?

Thoughts?

[jquense]

Is this something that would be better to address in react-formal?

Yes, Specifically the <Input> component should coerce empty strings to undefined when the type is string and the value is an empty string. I would suggest that null is a better option but then validations running in strict will fail for empty values unless they are marked as nullable() which actually may make the most sense...It depends on what the behavior of the other "typed" inputs is I think

[orokos]

We are going to give this a shot:

            mapValue={(value) => {
              let val = (type === 'password') ? value : value.trim();
              return val === '' ? undefined : val;
            }}

We moved the trim().transform() from the schema to the input control. Now the schema is just validating, no transformations, which should hopefully work out.

Potential problem is that if we use the schema on the back-end, someone could submit direct a field with just spaces and it would pass, where on the front-end it would not. Thinking that we would have to trim() the necessary fields first, outside of the schema, to process things correctly. Not as elegant. Hmmmm.

[jquense]

So if you feel comfortable with min always ignoring empty strings I would just change the default min behavior:

yup.addMethod(yup.string, 'min', function (min, msg) {
  return this.test({
    name: 'min',
    exclusive: true,
    message: msg,
    test: value => !value || value.length >= min
  })
})

[jquense]

That does require that I fix the above bug tho ^^^

[sudhirdhumal289]

I'm still seeing this issue with release "^0.26.10".

Used above mentioned snippet and now its working but core library has this issue.