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

types: avoid unnecessary MergeType<> if TOverrides not set, clean up statics and insertMany() type issues #13577

Merged
merged 5 commits into from Jul 10, 2023

Conversation

vkarpov15
Copy link
Collaborator

Summary

#13529 points out some issues with MergeType, this PR makes it so that we avoid using MergeType in HydratedDocument<> where possible.

However, for a future release, we should remove HydratedDocument, and instead rely on inferring the hydrated document type from the schema. Inferring HydratedDocument type from the raw document type isn't possible in general, and relies on Omit<>, which is super brittle in TypeScript - breaks on private/protected fields, unions with generics, and other cases.

Examples

@hasezoey
Copy link
Collaborator

hasezoey commented Jul 4, 2023

However, for a future release, we should remove HydratedDocument, and instead rely on inferring the hydrated document type from the schema.

to my knowledge HydratedDocument is useful when you want to assign values explicitly and dont have something like typegoose would provide called DocumentType
(ie have a type that is guaranteed to be the correct and full Document, with Require_id, overrides applied and everything that might be necessary in the future)

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, also mostly compiles fine with current typegoose

currently typegoose defines HydratedDocument<Something, any>, which makes it incompatible with HydratedDocument<Something, {}>, changing to the following helps, but i dont know if this is the best

TOverrides extends Record<string, never> ?
  Document<unknown, TQueryHelpers, DocType> & Require_id<DocType> :
  IfAny<TOverrides, Document<unknown, TQueryHelpers, DocType> & Require_id<DocType>, Document<unknown, TQueryHelpers, DocType> & MergeType<
    Require_id<DocType>,
    IfAny<TOverrides, {}>
  >>

another option (on typegoose's side) would be to remove the , any from the HydratedDocument call

@vkarpov15
Copy link
Collaborator Author

The idea is to make type HydratedDocument just pull a generic from a schema or model, instead of attempting to reconstruct the hydrated document from the raw document. In other words, right now we infer the raw document from the schema, then infer the hydrated document type from the raw type. We should instead infer both the raw document type and the hydrated document type from the schema. That will avoid having to Omit<>, would also clean up our problem with FlattenMaps in #13523

@vkarpov15
Copy link
Collaborator Author

@hasezoey I applied your suggested updates in 8b8f37b, can you please re-review?

Copy link
Collaborator

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me and compiles and runs without problems in the current latest typegoose

@vkarpov15 vkarpov15 merged commit 2188458 into master Jul 10, 2023
3 checks passed
@vkarpov15 vkarpov15 added this to the 7.3.3 milestone Jul 10, 2023
@orgads
Copy link
Contributor

orgads commented Jul 10, 2023

Thank you!

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

3 participants