Skip to content
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

Merged
merged 3 commits into from Aug 3, 2022

Conversation

emiljanitzek
Copy link
Contributor

Summary

It is not possible to define a model with both generic TInstanceMethods and TQueryHelpers. There is a mismatch between generics for the model and the the Schema.

Examples

See type test for example

@emiljanitzek emiljanitzek force-pushed the feature/model-schema-type branch 6 times, most recently from 3e2ae76 to dd0dd96 Compare July 22, 2022 14:36
Copy link
Collaborator

@vkarpov15 vkarpov15 left a 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.

interface UserInstanceMethods {
findByName(name: string): Promise<HydratedDocument<User>>;
}
type UserModel = Model<User, UserQueryHelpers> & UserInstanceMethods;
Copy link
Collaborator

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()...

Copy link
Contributor Author

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.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a 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.

@vkarpov15 vkarpov15 added this to the 6.5.1 milestone Aug 3, 2022
@vkarpov15 vkarpov15 merged commit 53f27aa into Automattic:master Aug 3, 2022
hasezoey added a commit to hasezoey/mongoose that referenced this pull request Aug 19, 2022
hasezoey added a commit to hasezoey/mongoose that referenced this pull request Aug 19, 2022
vkarpov15 added a commit that referenced this pull request Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants