-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix the schema to accept typed SchemaDefinition #9789
Fix the schema to accept typed SchemaDefinition #9789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't SchemaDefModel
default to DocType
if it is specified? Seems a little messy to add yet another generic parameter.
I agree that it's messy and if I have to change the default value of Another possibility(also not backward compatible) is to get rid of What do you think? Update:
|
ping |
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 |
I'm really looking forward for this PR, the current |
@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 //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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a closer look and this is likely the best way to go to avoid any breaking changes.
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 |
Thanks for the question but if you look closely at the changes in the previous PR there is actually no difference in the |
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 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! |
@Rossh87 there's a lot of different questions in your scripts, so I'll comment on each script individually:
|
This is to complete changes from a previous PR. Please refer to #9761 for more information.