yup: mixed.oneOf doesn't respect nullable

Ha ha, imagine how many dev hours you’ve wasted just by being stubborn and not handling this case automatically inside the library code.

... DEV DAYS WASTED BECAUSE OF ONE OF
NOT BEING ABLE TO HANDLE NULLS PROPERLY

Next person in this issue can increment the counter.

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

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!

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.

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?

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.

I would add that yup claims to be composable thanks to every method returning a new yup. Because of this questionable decision, this good principle is broken.

// Some reusable yup
export const yesNoAnyYup = yup.mixed<YesNoAny>().oneOf(Object.values(YesNoAny));

// at usage time
yesNoAnyYup.nullable()
// Here there is nothing I can do unless I completely override the initial `oneOf()`, 
// which break the LoD. 

Pleeease do something about it 🙏

Here is the typing for @micktdj’s oneOfOptional

import type {
    MixedLocale, Ref
} from "yup";

declare module "yup" {
    interface StringSchema<T, C> {
        oneOfOptional<U extends T>(
            arrayOfValues: ReadonlyArray<U | Ref>,
            message?: MixedLocale["oneOf"],
        ): StringSchema<U | null | undefined, C>;
    }
}

…the best way to handle this is to add null to your oneOf config

Note that if you do this, you must put .nullable() before .oneOf() otherwise TypeScript will fail with:

error TS2322: Type 'null' is not assignable to type 'string | Reference<unknown> | undefined'.

162    ['a', 'b', null],

// This won't work
const schema = yup.string().oneOf(["a", "b", null]).nullable();

// This works
const schema = yup.string().nullable().oneOf(["a", "b", null]);

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

Due to this non-obvious behavior, I cannot make a reusable description of the field. In a perfect world, I could do this:

function consensus() {
    return string()
        .oneOf(consensuses, consensusError);
}

And use this with native means as field: consensus() or field: consensus().nullable().

In the current implementation, I need to do crutches with passing parameters to my description.

@jquense I think this issue should be open for further discussion.

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