-
-
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(types): map correct generics from model to schema #12125
fix(types): map correct generics from model to schema #12125
Conversation
3e2ae76
to
dd0dd96
Compare
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.
In general LGTM, just had a question about one line in the tests that looks potentially wrong.
test/types/models.test.ts
Outdated
interface UserInstanceMethods { | ||
findByName(name: string): Promise<HydratedDocument<User>>; | ||
} | ||
type UserModel = Model<User, UserQueryHelpers> & UserInstanceMethods; |
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.
Seems a little odd that UserInstanceMethods
is part of the UserModel
type. You shouldn't be able to do UserModel.findByName()
...
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.
You are correct, I think the confusion came from TInstanceMethod
and TStaticMethods
on Schema and TMethodsAndOverrides
on Model which is the same as TInstanceMethod
. I've renamed the interfaces in the test and added statics too.
dd0dd96
to
343ff7b
Compare
343ff7b
to
e6128cb
Compare
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.
LGTM. I'm going to get rid of the U
constraint, because that seems to ding performance, but otherwise looks good.
[types] Apply #12125 to Connection
Summary
It is not possible to define a model with both generic
TInstanceMethods
andTQueryHelpers
. There is a mismatch between generics for themodel
and the theSchema
.Examples
See type test for example