Yup: mixed.oneOf doesn't respect nullable

[fnky]

The mixed.oneOf doesn't accept null when nullable is added.

Example

const fruitsOrNone = yup.string().oneOf(['apple', 'banana']).nullable(true)

fruitsOrNone.validate(null) // => throws ValidationError: 'this must be one the following values: apple, banana'
fruitsOrNone.validate('apple') // => apple

Expected

It's expected to accept the value null when nullable is set on the schema.

[jquense]

yes this confusing but expected. you have a contradiction between your schema on one hand you are saying its can _only_ be 'apple' or 'banana' and on the other say it can also be null, the best way to handle this is to add null to your oneOf config

[aphillipo]

I strongly think that nullable() should add the null to the oneOf constraint automatically. Sometimes you are reusing the options passed to oneOf for your dropdowns (for example) but you don't want a null option field added.

I'm currently doing .oneOf([null].concat(RELATIONSHIPS), message) or whatever fields you have in there and it looks mega weird!

[nfantone]

yes this confusing but expected. you have a contradiction between your schema on one hand you are saying its can only be 'apple' or 'banana' and on the other say it can also be null

@jquense I could make the same argument for every schema definition that includes .nullable(). For instance, by using yup.number().nullable() you'd claim that "this is confusing" because "you have a contradiction" by saying that your value "can _only_ be a number" and "on the other side, it says it can also be null" (two completely different types in JS). The point is: _yes_, that is exactly what I'm saying it should happen.

This boils down to a matter of semantics and expressing intent. I honestly wouldn't find it confusing at all to read yup.string().oneOf(['apple', 'banana']).nullable() (so, "apple", "banana" and null are all fine) - in the same way, I wouldn't think yup.number().nullable() is contradictory or unexpected. _On the other hand_, reading something like:

yup.string().oneOf([ null, ...MY_VALUES ]);

...is bound to raise some eyebrows.

[jquense]

I realize that yes this can be confusing or a bit odd. Much of the choices in yup straddle that line, because a lot fo this is _is_ subjective and up to what you feel makes sense in terms of expectations. We can of course keep trying to hit the sweet spot fro the most people but somethings are always bound to raise eyebrows for some people. In this particular case I think the current behavior, while somewhat surprising, is the most flexible approach, and consistent with Joi's behavior so there is some precedent.

[rv4Flyver]

Btw, yup.number().nullable() does not work for me in case of {field: {label: 'Some', value: null }}:

yup.object().shape({
  // ...
  field: yup.object().shape({
    label: yup.string(),
    value: yup.number().nullable()
  }),
});

Is this also an expected behavior?

[davet-ifit]

@jquence Reading this thread helped me to understand the choices you made here. Perhaps we could update the docs for mixed.oneOf to clarify this behaviour?

[Philipp91]

Agree with @fnky @aphillipo and @nfantone.

the current behavior, while somewhat surprising, is the most flexible approach, and consistent with Joi's behavior

IIUC, yup.oneOf([noNulls, inHere]).nullable(true|false) currently does _not_ allow null values, no matter what I pass to nullable(..). That is, it's exactly equivalent to yup.oneOf([noNulls, inHere]). And the same is true if null is contained in the list of allowed values, then one wouldn't need nullable() on top anymore.

So firstly, I don't see how the current behavior is the most flexible approach. Unless I'm overlooking something, it makes nullable useless when combined with oneOf.

And secondly, it seems like changing the behavior won't hurt anyone. The way the current behavior (and apparently also Joi's behavior?) is, nobody would be combining these two. So these users (and consistently with Joi) can't be negatively affected by changing the combined behavior.

[juni0r]

I agree with @nfantone and @Philipp91. The current semantics are confusing and I don't think that consistency with Joi's behavior is a strong argument. If Joi's got it wrong, why not fix it?

[micktdj]

You can implement your own method with Yup.addMethod to handle this case :

Yup.addMethod(Yup.string, 'oneOfOptional', function(arr, message) {
  return Yup.mixed().test({
    message,
    name: 'oneOfOptional',
    exclusive: true,
    params: {},
    test(value) {
      return value == null ? true : arr.includes(value);
    },
  });
});

export default Yup.string().oneOfOptional;

Then, you can use it like this :

import * as Yup from 'yup';
import oneOfOptional from './oneOfOptional';

export default Yup.object().shape({
  value: oneOfOptional(['toto','foo','bar'])
});

Finally, it works :

schema.isValid("toto") // true
schema.isValid(null) // true
schema.isValid(undefined) // true
schema.isValid('42') // false

[Zhouzi]

It seems that adding null to the list of elements is not enough, it must also be nullable. Here's the final working solution:

const fruits = ["banana", "apple"];
const schema = string().oneOf([...fruits, null]).nullable();

It can be tested here: https://runkit.com/zhouzi/5f2d264f20ab43001a495276