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

TypeScript: Fix for model constructor using "any." new Model(doc?:any) #10475

Closed
adingeist opened this issue Jul 20, 2021 · 3 comments
Closed
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@adingeist
Copy link

adingeist commented Jul 20, 2021

Problem:

When using the model constructor, the doc argument is typed with any. Shown here:
Screen Shot 2021-07-20 at 7 14 44 PM

Source of the issue in index.d.ts (line 631):

  interface Model<T, TQueryHelpers = {}, TMethods = {}> extends NodeJS.EventEmitter, AcceptsDiscriminator {
    new(doc?: T | any): EnforceDocument<T, TMethods>;

Although using the generic T has good intentions at including model properties, the union with any overrides type checking, making the type simply "any."

My solution

Note that not everything in a model's schema is used by a constructor (which is likely why "any" was union'ed with in the first place.) My solution to this is by creating a DeepPartial type to make all properties and their nested properties optional. Credit for DeepPartial

Here is the diff that solved my problem:

diff --git a/node_modules/mongoose/index.d.ts b/node_modules/mongoose/index.d.ts
index be86d72..380fcce 100644
--- a/node_modules/mongoose/index.d.ts
+++ b/node_modules/mongoose/index.d.ts
@@ -625,10 +625,13 @@ declare module 'mongoose' {
   interface AnyObject { [k: string]: any }
   type EnforceDocument<T, TMethods> = T extends Document ? T : T & Document<any, any, T> & TMethods;
 
+  type DeepPartial<T> = Partial<{ [P in keyof T]: DeepPartial<T[P]> }>;
+
   export const Model: Model<any>;
   // eslint-disable-next-line no-undef
   interface Model<T, TQueryHelpers = {}, TMethods = {}> extends NodeJS.EventEmitter, AcceptsDiscriminator {
-    new(doc?: T | any): EnforceDocument<T, TMethods>;
+    new(doc?:  DeepPartial<T>): EnforceDocument<T, TMethods>;
+    
 
     aggregate<R = any>(pipeline?: any[]): Aggregate<Array<R>>;
     // eslint-disable-next-line @typescript-eslint/ban-types
@@ -871,7 +874,7 @@ declare module 'mongoose' {
     replaceOne(filter?: FilterQuery<T>, replacement?: Object, options?: QueryOptions | null, callback?: (err: CallbackError, res: any) => void): QueryWithHelpers<any, EnforceDocument<T, TMethods>, TQueryHelpers, T>;
 
     /**
      * @deprecated use `updateOne` or `updateMany` instead.

This issue body was partially generated by patch-package.

Look at that beautiful type checking! 🎉

Screen Shot 2021-07-20 at 7 45 13 PM

Screen Shot 2021-07-20 at 7 45 29 PM

A better solution may exist because the time complexity of the DeepPartial isn't great using looping.

@IslandRhythms IslandRhythms added the typescript Types or Types-test related issue / Pull Request label Jul 21, 2021
@vkarpov15 vkarpov15 added this to the 5.13.4 milestone Jul 27, 2021
@vkarpov15
Copy link
Collaborator

We will take a look and see if this approach works. any is intentional - you can pass any object to a model constructor. But we may be able to figure out some way to make autocomplete work better without breaking everyone's code.

@vkarpov15 vkarpov15 reopened this Jul 28, 2021
@adingeist
Copy link
Author

Thanks that was an accident and also thanks for looking into it.

Update on the usage with my first post.

What I proposed was unfortunately a breaking approach with the way some people set up tests.

See the following:
Screen Shot 2021-07-28 at 8 55 49 AM

In this case, Partial and DeepPartial will throw the following typescript error,

Argument of type 'Partial<IUser>' is not assignable to parameter of type 'Partial<{ _id: Partial<{ generationTime: number; equals: Partial<{}>; getTimestamp: Partial<{}>; toHexString: Partial<{}>; }>; createdAt: Partial<{ toString: Partial<{}>; ... 42 more ...; [Symbol.toPrimitive]: Partial<...>; }>; ... 71 more ...; validateSync: Partial<...>; }>'. Types of property '$locals' are incompatible. Type 'Record<string, unknown> | undefined' is not assignable to type 'Partial<{ [x: string]: Partial<{}>; }> | undefined'. Type 'Record<string, unknown>' is not assignable to type 'Partial<{ [x: string]: Partial<{}>; }>'

so everyone would have to go into their code and change document variables to be defined as DeepPartial, which obviously is very bad.

@vkarpov15
Copy link
Collaborator

I figure we'll just pull top-level keys from T - that's better than having any since at least the key names will show up, if not necessarily the correct typings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

No branches or pull requests

3 participants