DefinitelyTyped icon indicating copy to clipboard operation
DefinitelyTyped copied to clipboard

[yup] InferType should default to nonRequired()

Open hellos3b opened this issue 5 years ago • 15 comments

Issue When creating a schema with Yup, you don't get validation errors on validate if a property is missing; it's non-required by default.

When using InferType<>, it marks all properties as required by default unless explicitly noted as notRequired(), which leads to TS errors in vscode

Sample

import {object, string, InferType} from 'yup'

const schema = object({
  requiredString: string().required(),
  notRequiredString: string()
})

// Some method that takes schema json as an argument
const method = (json: InferType<typeof schema>) => {}

// Error: Property 'notRequiredString' is missing in type '{ requiredString: string; }' but required in type (...)
method({
  requiredString: "done"
})

Expected The above should show no error since notRequiredString isn't marked as required


  • Authors: @dhardtke @vtserman @MoretonBayRC @sseppola @YashdalfTheGray @vincentjames501 @robertbullen @sat0yu @deskoh @mauricedb @kalley

hellos3b avatar Mar 07 '20 00:03 hellos3b

I did consider doing this when inplementing InferType<T> but decided against it.

First of all The semantics of a required property in Yup is not the same as in TypeScript. For example a Yup.array().of(Yup.string()).required() will fail validation if you pass in an empty array. A required array should not be empty: empty arrays are also considered 'missing' values.

In general there isn't a one to one match between Yup and TypeScript concepts so this is never going to be perfect. Another example of that is a Yup.date() that will pass validation if you use the string "2020-02-14T07:52:25.495Z" if you don't call .strict()

mauricedb avatar Mar 07 '20 09:03 mauricedb

hey @mauricedb why is

export type InferType<T> = T extends Schema<infer P> ? InnerInferType< : never;

and not simply

export type InferType<T> = T extends Schema<infer P> ? P : never;

? what does really InnerInferType buy us? right now InferType seems broken, on the latest release.

const schema = object({ foo: string().required() })
InferType<typeof schema>

gives {foo?: string} instead of the correct {foo: string}

itajaja avatar Apr 27 '20 18:04 itajaja

@itajaja

I am seeing a type with {foo: string} not the optional ? indicator. Can you create a simple repro using CodeSandbox and create an issue with that? You can use https://codesandbox.io/s/stoic-wu-7p2xw?file=/src/index.ts as a starter.

InnerInferType<P> takes care of required/null or not props and secial cases array schemas.

mauricedb avatar Apr 28 '20 15:04 mauricedb

@mauricedb thanks for the starter, it took me a little bit to figure out. interestingly, it's the strict: true flag that makes a difference. when you change to strict: false, then you get foo? string. I am not sure why is that

itajaja avatar Apr 29 '20 03:04 itajaja

also, re "takes care of required/null" why not have

export interface StringSchemaConstructor {
    <T extends string | null | undefined = string | null | undefined>

instead of

export interface StringSchemaConstructor {
    <T extends string | null | undefined = string>

?and why do arrays need to be special cased? Thanks! 🙏

itajaja avatar Apr 29 '20 03:04 itajaja

@itajaja

Setting strictNullChecks: true or even better strict: true is a requirement when working with required.

I have an open PR on the documentation for Yup about this. See: https://github.com/jquense/yup/pull/805

mauricedb avatar Apr 29 '20 08:04 mauricedb

thanks! unclear why it requires strict, though, in principle. I am SURELY missing something, so thanks for your patience, but why can't it simply be something like

https://www.typescriptlang.org/play/#code/C4TwDgpgBAcglgGygXigVwHYBMIDM4YRZQA+UGaCCA3ALABQokUA8gEYBWKUAShAMYB7AE5YAPAGdgwggHMANFACGGEAD469BgWARhuJf2gBlfgAsIAWyViAKmqgBvBlFdRhEAI5o4HrAC4oAAoAShQHUwtrMQBRAA9+BDQcO0V4BDUNBgBfBgYmE2k5SKslbhLoqRkMWVJYRCzGcGh2DgFgCptbKAg43WwJVk4HVE67OvTGgqG2-g7zUoARPAI4YDhBDHHe-qxB1pGnFzcAbXwIBGICKABrCBBBXChbAF1AsdszuAusF8bszT5ZpQACSGFwelszTsh26OwgAygYwIEOEUAACg4APwYqCBQgANz0gPoOESSg8UFwmDmGwwUCqclC7yKNU6mjJCAp0GpGFpmyggk47W2fQRexmaiC50uEkCrXanWW+AwazpMJC8uFcw+jQYQgwUgZAEZuELZsAgs56G5lIFGTVQjkQvrNkaJAAmM3ay3W21Ke2s2ShAB0Hm8viITvo2RdWiazGMptQYNRUMgYgKjxNU2Bxi9KfBkOhWaensaQA

I don't fully understand the argument about "'' is not valid for string().required()". That to me is simply something that typescript cannot address and that's fine, but I don't understand how that impacts type definition, seems more like an unrelated point about typescript not being tailored for that kind of validation. Let me know if there is a better place that you want to move this conversation to!

itajaja avatar Apr 29 '20 13:04 itajaja

The point is that the Yup schemas serve different purposes. They are first and foremost intended for validation as that is Yup's main purpose. Being able to infer the TS type is nice because there is always a lot of duplication if you have to maintain validation schema and TS type seperately.

Because Yup was designed without TS usage in mind there is a mismatch between the two. InferType<T> tries make that easy but has to make some sacrifices here.

Not makign things optional by default is one. Using strict is another of those for many cases in otder for the TS compiler to see the difference between for example a string, null and undefined.

mauricedb avatar Apr 29 '20 13:04 mauricedb

can you clarify a little? like an example where you show the difference and how the design choice affects the types? or, in other words, what's "wrong" about that quick definition outlined above? unsure what you mean by "Not makign things optional by default is one."

my 2 cents, the way to look at this, is not that yup "was designed without TS in mind". even if yup was completely designed for TS, you still wouldn't be able to do dynamic validations over, say, empty strings, emails, regexes, number ranges, you name it. but that again wouldn't stop writing a mostly correct TS type, as far as typescript is capable of handling the definition, and Ts definitely can handle nulls and undefined values.

Trying to understand and have some fruitful conversation, not trying to shun your approach :)

itajaja avatar Apr 29 '20 13:04 itajaja

Sure no problem.

Lets conside the following simple case:

const personSchema = yup.object({ firstName: yup.string() });
const person = {};
console.log(personSchema.isValidSync(person));

In JavaScript this is perfectly valid and the call to isValidSync() returns true.

The same is true if I change the definition of person a bit:

const person = { firstName: undefined };

or

const person = { firstName: '' };

or

const person = { firstName: 'Maurice' };

In all these cases the call to isValidSync() still returns true.

This could be done in TypeScript with a type defintion like this:

type Person = {
  firstName?: string;
};

Now suppose we do not want the firstName property to be optional or allow for undefined. In TypeScript that would be simple, just remove the ? like this:

type Person = {
  firstName: string;
};

Now the firstName property needs to be some kind of string so this is valid:

const person: Person = { firstName: "" };

Now lets bring yup.InferType<T> into the mix to infer the person type.

If we use the same defaults and a yup.string() is optional the following would be fine (note it isn't and I will come back to that):

const personSchema = yup.object({ firstName: yup.string() });
type Person = yup.InferType<typeof personSchema>;

const person: Person = {};
console.log(personSchema.isValidSync(person));

The same is true if I change the definition of person like before:

const person: Person = { firstName: undefined };

or

const person: Person = { firstName: '' };

or

const person: Person = { firstName: 'Maurice' };

So far so good but suppose I want to make firstName a required property. So the last two compile without error in TypeScript but the first two don't. Sounds simple, just change the schema to:

const personSchema = yup.object({ firstName: yup.string().required() });

This infers to:

type Person = {
  firstName: string;
};

As far as TypeScript is concerned we have achieved our goal.

However the reason we started with Yup is because we wanted to use it for runtime validation. And adding the .required() makes the object with empty string firstName { firstName: '' } fail validation. Having a type that needs to be a string but allows for an empty sting is quite common so needs to be supported but .required() does a bit too mutch for us at runtime.

To get around that problem adding a yup.string() property without any additional settings is for TypeScript required but for Yup not. Do you want to make the property optional for TypeScript? Just add change the definition to:

const personSchema = yup.object({ firstName: yup.string().notRequired() });

For Yup this doesn't change anything, it implicitly wasn't required and now it explicitly isn't required and for TypeScript it isn't a required property either so can be left out or will accept undefined.

Is it perfect? No it isn't but I can't allign the two without making changes to Yup or TypeScript. And I am pretty sure that a PR with a breaking change like that would not be accepted by either. But assuming I could do so I would change Yup so that a yup.string() would not be valid with a missing or undefined property. You want to allow that then explicitly call yup.string().notRequired(). But as I said, that is very unlikely to happen as that would be big a breaking change.

HTH

mauricedb avatar Apr 29 '20 18:04 mauricedb

Thanks, but I'll be honest, I don't follow your reasoning, and again I don't see why InferType couldn't simply be export type InferType<T> = T extends Schema<infer P> ? P : never; maybe a counterexample of where that would break could help. especially I don't understand the point where yup.string() needs to result in string and not in string | undefined | null

itajaja avatar Apr 29 '20 18:04 itajaja

Right now the folowing is valid for TypeScript:

const personSchema = yup.object({
    firstName: yup.string().notRequired(),
});
type Person = yup.InferType<typeof personSchema>;
const person: Person = {};

Note that no firstName is provided as it isn't required.

With export type InferType<T> = T extends Schema<infer P> ? P : never; that is no longer valid and you would need to the person as { firstName: undefined } because the firstName property has to be added but can be explicilty made undefined.

HTH

mauricedb avatar Apr 29 '20 19:04 mauricedb

ok, I see. because const a: { foo: undefined } = {} is actual invalid, which is unfortunate. thanks for the patience. Looks like there is a very lengthy issue tracking this, where everyone agrees, but the change seems hairy to make

itajaja avatar Apr 30 '20 15:04 itajaja

I used this and it worked fine:

export type RemoveIndex<T> = {
  [K in keyof T as string extends K ? never : number extends K ? never : K]: T[K];
};

const validationSchema = yup.object().shape({
  username: yup.string().required(),
  password: yup.string().required(),
});

type SignInFormData = RemoveIndex<yup.InferType<typeof validationSchema>>;

abeninski avatar May 20 '22 14:05 abeninski

I also use heavy convoluted type to make type inference work with yup. I do feel like current InferType conflicts with how TypeScript works. Validation is for validation so it just needs to work in runtime. But typescript is for reading / type inference, so it just needs to solve that problem(even though type system mismatches, we just need to make it work, applying hack or sth), I do think this is an improvable case.

vlwkaos avatar Jun 10 '24 23:06 vlwkaos