Fix the schema to accept typed SchemaDefinition

[mroohian]

This is to complete changes from a previous PR. Please refer to #9761 for more information.

[vkarpov15]

Shouldn't SchemaDefModel default to DocType if it is specified? Seems a little messy to add yet another generic parameter.

[mroohian]

I agree that it's messy and if I have to change the default value of SchemaDefModel I'd use LeanDocument<DocType>. However this would mean that SchemaDefinition<undefined> wouldn't be accepted by Schema and this breaks the backward compatibility.

Another possibility(also not backward compatible) is to get rid of SchemaDefModel all together and update SchemaDefinition to only use LeanDocument<DocType> which I don't recommend.

What do you think?

Update:

LeanDocument keeps _id and __v and forces the SchemaDefinition<LeanDocument<DocType>> to define them. You'd have to use something like: SchemaDefinition<Omit<LeanDocument<DocType>, '_id' | '__v'>>

[mroohian]

ping

[vkarpov15]

This is something I'll have to look into more, I don't know enough about TypeScript to know the right solution off the top of my head. I just really want to avoid adding yet another generic parameter to Schema, it's getting a bit unwieldy.

[davidkhierl]

I'm really looking forward for this PR, the current SchemaDefinition only accepts type of String and it's actually a bug. https://stackoverflow.com/questions/65635637/mongoose-5-11-11-schemadefinition-typing

[Rossh87]

@mroohian Thanks for all your work on this! I've been poking around with this a bit in response to #9857, and I think there's some confusion around TypeScript's constructor types vs primitive types; unfortunately, I don't believe the type API you outline in your comment to #9761 is possible:

interface User {
  readonly email: string;  //Denotes the typescript type 'string'--i.e., a JavaScript primitive string
  readonly password: string;
}

const userSchemaDefinition: SchemaDefinition<User> = {
  email: { type: String, required: true },  //Crucially, the type of userSchemaDefinition.email.type is the TypeScript type StringConstructor--i.e. not a primitive
  password: { type: String, required: true },
}

Now, please consider the type definition of SchemaTypeOptions, which is the type userSchemaDefinition.email (e.g.) must satisfy:

//T here is the type specified for User.email, i.e. the *primitive type* 'string'

interface SchemaTypeOptions<T> {
    type?: T;  //This is wrong--we need StringConstructor here, not just a plain old string
//...
    default?: T | ((this: any, doc: any) => T);  //Here we have the opposite problem: if T was type StringConstructor, as required above, T would not be the appropriate type for this field
//...
}

This is just an example; there are other places where the same sort of inconsistencies crop up. I have some thoughts about how you might simplify some of these types and still get most of the developer experience I think you're looking for, but I don't want to take any more of your time if you're already on top of this. Just lemme know and I'm happy to say more, and/or submit a PR. Thanks!

[vkarpov15]

I took a closer look and this is likely the best way to go to avoid any breaking changes.

[Rossh87]

Sorry, I don't mean to be a pest about this, but I think these changes are going to create more issues than they resolve. Specifically, they imply that a user can derive an accurate, typesafe type for their schema definition object by passing in the interface that mongoose documents derived from the schema will implement. That isn't the case. At best, the result is a type that admits obviously incorrect members; at worst a correct implementation won't compile because the compiler expects the wrong types. The test file added in bdbf25d does compile, but not for the right reasons. I've reproduced the problems documented in several closed issues with the current version of Master in this repo. Please see the files/comments in /src/, and let me know if you'd like any further help with this. Thanks!

[mroohian]

@mroohian Thanks for all your work on this! I've been poking around with this a bit in response to #9857, and I think there's some confusion around TypeScript's constructor types vs primitive types; unfortunately, I don't believe the type API you outline in your comment to #9761 is possible:

interface User {
  readonly email: string;  //Denotes the typescript type 'string'--i.e., a JavaScript primitive string
  readonly password: string;
}

const userSchemaDefinition: SchemaDefinition<User> = {
  email: { type: String, required: true },  //Crucially, the type of userSchemaDefinition.email.type is the TypeScript type StringConstructor--i.e. not a primitive
  password: { type: String, required: true },
}

Now, please consider the type definition of SchemaTypeOptions, which is the type userSchemaDefinition.email (e.g.) must satisfy:

//T here is the type specified for User.email, i.e. the *primitive type* 'string'

interface SchemaTypeOptions<T> {
    type?: T;  //This is wrong--we need StringConstructor here, not just a plain old string
//...
    default?: T | ((this: any, doc: any) => T);  //Here we have the opposite problem: if T was type StringConstructor, as required above, T would not be the appropriate type for this field
//...
}

This is just an example; there are other places where the same sort of inconsistencies crop up. I have some thoughts about how you might simplify some of these types and still get most of the developer experience I think you're looking for, but I don't want to take any more of your time if you're already on top of this. Just lemme know and I'm happy to say more, and/or submit a PR. Thanks!

Thanks for the question but if you look closely at the changes in the previous PR there is actually no difference in the SchemaDefinition fields type. It just gives you a hint about the existence of model fields in the schema. In the example that you've provided userSchemaDefinition.email must exist but its type is not being enforced. It can be any of SchemaDefinitionProperty type options. Mapping between all possible types in the model and SchemaDefinitionProperty is difficult if not impossible.

[Rossh87]

I appreciate the prompt response! Sorry if I implied that these changes introduce a new bug--they don't. In fact, it did clear up behavior A from #9857. However, there are other issues that (as I understand) this PR is meant to address, but does not. Have you had a chance to look at the issues I've reproduced here on the latest commit of Master? I would highlight src/iss_9761_PR.ts. All of these samples contain valid code that won't compile for me, because SchemaDefinitionProperty DOES pass the property types specified in its type argument through to the type of SchemaTypeOptions:

type SchemaDefinitionProperty<T = undefined> = SchemaTypeOptions<T extends undefined ? any : T> |
    SchemaDefinitionWithBuiltInClass<T> |
    typeof SchemaType |
    Schema<T extends Document ? T : Document<any>> |
    Schema<T extends Document ? T : Document<any>>[] |
    SchemaTypeOptions<T extends undefined ? any : T>[] |
    Function[] |
    SchemaDefinition<T> |
    SchemaDefinition<T>[];

  type SchemaDefinition<T = undefined> = T extends undefined
    ? { [path: string]: SchemaDefinitionProperty; }
    : { [path in keyof T]?: SchemaDefinitionProperty<T[path]>; };

Again, my apologies if I've misunderstood the intent of these changes. Thanks!

[vkarpov15]

@Rossh87 there's a lot of different questions in your scripts, so I'll comment on each script individually:

1. iss_9761_PR.ts: this script doesn't compile, and I'd argue that's intentional. If you specify a generic for SchemaDefinition and you pass in a schema definition that includes extra fields, TypeScript will error out. Based on your comments, it looks like you understand this behavior and agree with it.
2. iss_9761.ts: this script compiles against Mongoose 5.11.17, so no issues here.
3. iss_9857.ts: this script looks correct and compiles correctly, no issues here.
4. iss_9862.ts: these values are not "clearly inappropriate". Mongoose does allow arbitrary strings and functions as schema properties to support custom schematypes.
5. iss_9863.ts: this script compiles correctly against Mongoose 5.11.17. And as for your concern about NumberConstructor, that seems like intentional behavior. age is a property that is set to NumberConstructor, which is the type that TypeScript assigns to JavaScript's built-in Number() function.