Missing generics in create() methods

[chasecaleb]

For example, see string.ts:

yup/src/string.ts

Lines 26 to 34 in 3ca0ebf

	export function create() {
	return new StringSchema();
	}
	export default class StringSchema<
	TType extends Maybe<string> = string | undefined,
	TContext extends AnyObject = AnyObject,
	TOut extends TType = TType
	> extends BaseSchema<TType, TContext, TOut> {

The generated string.d.ts ends up with a signature of, which makes the generic impossible to constrain:
export declare function create(): StringSchema<string | undefined, Record<string, any>, string | undefined>;

The solution is to make the create() function generic, e.g.:

export function create< 
   TType extends Maybe<string> = string | undefined, 
   TContext extends AnyObject = AnyObject, 
   TOut extends TType = TType 
 >() { 
   return new StringSchema<TType, TContext, TOut>(); 
}

Based on quickly skimming your codebase (I may have missed some), it looks like this applies to the following:

• yup/src/Reference.ts

  Line 13 in 3ca0ebf

  export function create(key: string, options?: ReferenceOptions) {

• yup/src/boolean.ts

  Line 8 in 3ca0ebf

  export function create() {

• yup/src/date.ts

  Line 16 in 3ca0ebf

  export function create() {

• yup/src/number.ts

  Line 10 in 3ca0ebf

  export function create() {

• yup/src/string.ts

  Line 26 in 3ca0ebf

  export function create() {

I'm trying to upgrade from yup@0.27.0 + @types/yup@0.26.22 to just yup@0.32.6 (without the DefinitelyTypes @types definitions), but I can't because your type definitions would require too much unsafe casting. If you don't have time to fix this yourself, let me know and I'll make a PR.

[jquense]

open to a PR, the problem tho is the generics on the factory methods, get incorrectly inferred to any in the context of an object() shape like:

object({
  name: string() // ends up StringSchema<any, any>
})

which is clearly worse than their default values. I didn't have an obvious solution so deferred. Happy for some help if you want to jump in.

[jquense]

You may be able to work around it with explicit overloads!

[chasecaleb]

Hmm, I can't reproduce that... this works for me:

export function create<
    TType extends Maybe<string> = string | undefined,
    TContext extends AnyObject = AnyObject,
    TOut extends TType = TType
    >() {
  return new StringSchema<TType, TContext, TOut>();
}

With a new test file:

import { object, string, StringSchema } from '../src';
import { AnyObject, OptionalObjectSchema } from '../src/object';

it("should compile", () => {
   // I made the type explicit here to force a compile error if it didn't match
    const foo: OptionalObjectSchema<{ a: StringSchema<string | undefined, AnyObject>}> = object({ a: string() });
})

By the way, I don't think you're type checking your test files. I had to add @types/jest as a dev dependency, remove a line with a type error from test/types.ts, and then finally run tsc --build tsconfig.json from tests directory. On the bright side, my test snippet above compiles.

---

Have you tried comparing your code against the DefinitelyTyped definitions? I know they're a little bit out of date (v0.29 according to the comment at the top of file), but they handle generics properly. Another issue I noticed is your ObjectShape definition which uses Record:

yup/src/object.ts

Lines 30 to 33 in 3ca0ebf

	export type ObjectShape = Record<
	string,
	AnySchema | Reference | Lazy<any, any>
	>;

Whereas DefinitelyTyped uses [field in keyof T] to constrain the keys:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/e539b14e9b5c4359ffaa67edbb845998d4fe8e27/types/yup/index.d.ts#L354-L360

The advantage of DefinitelyTyped's version is that it allows stricter value typing like this:

interface LoginFormValues {
    readonly user: string;
    readonly password: string;
}
const loginValidator = yup.object<LoginFormValues>().shape({
    user: yup.string().required(),
    password: yup.string().required()
});

This is a contrived example, but I use this a ton with a large codebase at work where I'm passing form values around between yup, Formik, and Redux. If I changed LoginFormValues to be a Record I would lose type safety, which is bad.

[jquense]

By the way, I don't think you're type checking your test files.

I'm not, at least not as part of CI, at the moment i'm manually checking them in my editor

Have you tried comparing your code against the DefinitelyTyped definitions

No, intentionally so. The types here take a different approach with a different set of tradeoffs. I've definitely consulted the DT types, but overall I'm not trying to match them

Another issue I noticed is your ObjectShape definition which uses Record

this is a good example of a difference. the DT types start with a concrete type and infer a Shape that matches it, whereas the types here go the other way. You specify a concrete shape, and it derives the resulting type. There are pros/cons of both approaches, but I went with the current one because it allows producing a more correct output type. The two generics are different things here, i don't think you are losing the type safety (but happy to take a look if you have a specific example)

[chasecaleb]

Sure, here's a few examples of how DefinitelyTyped's version provides type safety:

import * as yup from "yup";

interface LoginFormValues {
    readonly user: string;
    readonly password: string;
}

// Compiles:
export const loginValidatorValid = yup.object<LoginFormValues>({
    user: yup.string().required(),
    password: yup.string().required()
});
// ERROR:
// Argument of type '{ user: yup.StringSchema<string>; passworx: yup.StringSchema<string>; }' is not assignable to parameter of type 'ObjectSchemaDefinition<LoginFormValues>'.
//  Object literal may only specify known properties, but 'passworx' does not exist in type 'ObjectSchemaDefinition<LoginFormValues>'. Did you mean to write 'password'?
export const loginValidatorTypo = yup.object<LoginFormValues>({
    user: yup.string().required(),
    passworx: yup.string().required()
});
// ERROR:
// Argument of type '{ user: yup.StringSchema<string>; password: yup.StringSchema<string>; shouldNotBeHere: yup.StringSchema<string>; }' is not assignable to parameter of type 'ObjectSchemaDefinition<LoginFormValues>'.
//  Object literal may only specify known properties, and 'shouldNotBeHere' does not exist in type 'ObjectSchemaDefinition<LoginFormValues>'.
export const loginValidatorExtraField = yup.object<LoginFormValues>({
    user: yup.string().required(),
    password: yup.string().required(),
    shouldNotBeHere: yup.string().required()
});
// ERROR:
// Type 'NumberSchema<number>' is not assignable to type 'Schema<string> | Ref'.
//  Type 'NumberSchema<number>' is not assignable to type 'Schema<string>'.
//    Types of property 'concat' are incompatible.
//      Type '(schema: NumberSchema<number>) => NumberSchema<number>' is not assignable to type '(schema: Schema<string>) => Schema<string>'.
//        Types of parameters 'schema' and 'schema' are incompatible.
//          Type 'Schema<string>' is missing the following properties from type 'NumberSchema<number>': min, max, lessThan, moreThan, and 8 more.
export const loginValidatorWrongValueType = yup.object<LoginFormValues>({
    user: yup.string().required(),
    password: yup.number().required()
});

Whereas using your types from yup@0.32.6 (import and interface omitted):

// ERROR:
// Type 'LoginFormValues' does not satisfy the constraint 'Record<string, AnySchema<any, any, any> | Reference<unknown> | Lazy<any, any>>'.
//  Index signature is missing in type 'LoginFormValues'.
export const loginValidatorNoLongerValid = yup.object<LoginFormValues>({
    user: yup.string().required(),
    password: yup.string().required()
});
// Since an index signature is required, I can't constrain the generic in any meaningful way.
// So this works...
export const loginValidatorValid = yup.object({
    user: yup.string().required(),
    password: yup.string().required()
});
// And now we get to the problem: all of the below versions still pass type checking, even though
// they are not the intended/correct behavior.
export const loginValidatorTypo = yup.object({
    user: yup.string().required(),
    passworx: yup.string().required()
});
export const loginValidatorExtraField = yup.object({
    user: yup.string().required(),
    password: yup.string().required(),
    shouldNotBeHere: yup.string().required()
});
export const loginValidatorWrongValueType = yup.object({
    user: yup.string().required(),
    password: yup.number().required()
});

See how the DefinitelyTyped version makes sure that my validator type and value type (LoginFormValues) match? That's a big deal because this is the entry point for user data into the rest of the app, so I need to be sure that my validations are correct to avoid e.g. crashing when persisting the data to my Redux store or making an API request with incorrect data.

[jquense]

sure, if you want to ensure a schema matches an object you still can, but the signature is a bit different. Now you would do something like:

interface LoginFormValues {
    readonly user: string;
    readonly password: string;
}

export const loginValidatorValid: yup.SchemaOf<LoginFormValues> = yup.object({
    user: yup.string().required(),
    password: yup.string().required()
});

[chasecaleb]

Oh, I missed that. That works for 2/3 of my negative examples: loginValidatorTypo and loginValidatorWrongValueType. It isn't causing an error on the extra field for loginValidatorExtraField, which admittedly is the least important of the 3 but it's still meaningful. I'm still digging into your definitions to see if I can come up with a suggestion, but for reference here are the docs for TypeScript's excess property checks. I think the difference is that DefinitelyTyped's version lets me specify the generic on the RHS so it's treated as an object literal, whereas with your version it's specified on the LHS as the return type so it isn't subject to excess property checking. Do you have any thoughts on solutions?

[jquense]

I'm not sure it's possible to get excess property checks here, it'd require a specific interface or mapped type neither which seem hard to use here. I can give it a shot at some point tho

[chasecaleb]

That's not quite it, it needs the generic applied to the object literal (i.e. the object(..) call's parameter in the case of the examples above), as opposed to right now how it's applied to the return type. Not the end of the world though because that's a less likely and easier to catch error compared to missing keys or incorrect validator types.

Anyways I think I took us on a massive tangent compared to my original concern with this issue, namely the create() methods. Sorry and thanks for humoring it!

I think where we left off was the first part of my comment here: #1159 (comment) :

Hmm, I can't reproduce that... this works for me:

export function create<
    TType extends Maybe<string> = string | undefined,
    TContext extends AnyObject = AnyObject,
    TOut extends TType = TType
    >() {
  return new StringSchema<TType, TContext, TOut>();
}

With a new test file:

import { object, string, StringSchema } from '../src';
import { AnyObject, OptionalObjectSchema } from '../src/object';

it("should compile", () => {
   // I made the type explicit here to force a compile error if it didn't match
    const foo: OptionalObjectSchema<{ a: StringSchema<string | undefined, AnyObject>}> = object({ a: string() });
})

I know you said this was causing overly-broad inference with object(..) when you initially tried, but since I can't reproduce that I think it's feasible now. It could be because of inference improvements with newer TypeScript versions, if nothing else.

Do you want to try this out, or would you rather I modify all the generic-less create() methods and make a PR for you to review? And if you want a PR, could you give me a suggestion for how you'd like it tested? The closest to type testing I see is test/types.ts, but it currently doesn't compiler, is excluded from jest, and I'm not sure how the $ExpectType comments work. The quick and easy way I would go with if left to figure it out myself would be to remove the non-compiling part of test/types.ts, add some more code with explicitly-typed variables (to make the compiler verify it), and add a tsc command to yarn test to type check it. I can test it however you'd like, though.

[jquense]

Happy to take a PR adding the right generics here. you would want to test via $ExpectType comments tho not just the TS compiles e.g.

const foo: OptionalObjectSchema<{ a: StringSchema<string | undefined, AnyObject>}> = object({ a: string() });

is not actually ensuring that it's correct, because any is assignable to any other type e.g. this passes just fine

const foo: OptionalObjectSchema<{ a: StringSchema<string | undefined, AnyObject>}> = object({ a: null as any });

[chasecaleb]

Oh yeah, that's true. Is https://github.com/microsoft/dtslint what you're using for $ExceptType?

[jquense]

using https://github.com/4Catalyzer/eslint-plugin-ts-expect but it's functionally the same thing, except uses eslint instead of dtslint

[chasecaleb]

Ah, thanks! I'll start working on a PR, I should have something for you to review by the end of the week, if not sooner.

[chasecaleb]

Well this is awkward. Turns out I misunderstood what was going on, I think due to some inaccuracies in the DefinitelyTyped definitions I was using previously. The way those create() methods were typed is correct and shouldn't take generic parameters after all.

To illustrate what I was getting confused by:

// Compile error:
// Type 'StringSchema<string | undefined, Record<string, any>, string | undefined>' is not assignable to type 'BaseSchema<Maybe<string>, Record<string, any>, string>'.
//   Types of property '__outputType' are incompatible.
//     Type 'string | undefined' is not assignable to type 'string'.
//       Type 'undefined' is not assignable to type 'string'.  TS2322
export const invalid: Yup.SchemaOf<string> = Yup.string();

// Everything below compiles
export const correctA: Yup.SchemaOf<string> = Yup.string().ensure();
export const correctB: Yup.SchemaOf<string> = Yup.string().defined();
export const correctC: Yup.SchemaOf<string> = Yup.string().required();

I was trying to fix the invalid version by doing something like Yup.string<string>() or something like that... In retrospect I'm not honestly sure why I thought that would make sense. I think I was getting confused because the way I provide input to Yup when validating makes sure that I never have undefined or null values, so I never encountered a situation where the unsound DefinitelyTyped definitions made a difference.

So TLDR: no PR needed and you can close this issue - I was confused due to unsound definitions from DefinitelyTyped. Thanks so much for humoring me and helping me work through upgrading to the newest version of Yup with your type definitions. And nice job migrating to TypeScript!

---

I did come across one unrelated issue related to the SchemaOf type definition, so I made a PR for that: #1169 ... so at least something useful came out of all of this for you.

[jquense]

yeah i think that's right, generally. There is still some reason to add a generic like so:

function create<T extends string>() {
  return new StringSchema<T | undefined>()
}

const specific: 'hi' = create<'hi'>().default('hi')

but that's a lot nichey-er of a use case

[activebiz]

sure, if you want to ensure a schema matches an object you still can, but the signature is a bit different. Now you would do something like:

interface LoginFormValues {
    readonly user: string;
    readonly password: string;
}

export const loginValidatorValid: yup.SchemaOf<LoginFormValues> = yup.object({
    user: yup.string().required(),
    password: yup.string().required()
});

This does work but I am still lossing the type safety inside the yup.object(...). All we are doing is just casting to make typescript happy. (which works fine as temp. solution) but this problem needs proper fix.