Incorrect usage of mongoose's generic Schema type

[troywweber7]

Is there an existing issue for this?

• [X] I have searched the existing issues

Current behavior

Mongoose took great care in crafting their types once @types/mongoose was deprecated, but they aren't very readable at times. As a result, I believe this library misuses the types in regards schema creation via class definition. Consider the current definition for SchemaFactory:

export declare class SchemaFactory {
    static createForClass<TClass extends any = any, TDocument extends mongoose.Document = TClass extends mongoose.Document ? TClass : mongoose.Document<TClass>>(target: Type<TClass>): mongoose.Schema<TDocument>;
}

And the partial definition for Schema from mongoose themselves:

class Schema<DocType = any, M = Model<DocType, any, any, any>, TInstanceMethods = {}>  ... {
    constructor(definition?: SchemaDefinition<SchemaDefinitionType<DocType>>, options?: SchemaOptions);
    ...
}

SchemaDefinitionType goes through the trouble of stripping Document properties so you are left only with the document type handed to the SchemaDefinition. Now, what @nestjs/mongoose does in SchemaFactory.createForClass technically works, but I think it would be more correct to return mongoose.Schema<TClass> rather than forcing the first generic parameter to be a document. It would also simplify greatly to then just be the following since you no longer need the generic property.

export declare class SchemaFactory {
    static createForClass<TClass extends any = any>(target: Type<TClass>): mongoose.Schema<TClass>;
}

Since upgrading from a version of mongoose 5 that required me to use 3rd party @types/mongoose, I've been studying the types released from mongoose themselves and I find them to be far superior to the 3rd party types we used to have to rely on. I'm open to finding out that I'm wrong about this, and yes, it certainly works as is, but I think it warrants investigation. After all, if mongoose required that we hand an extension of Document to the Schema type, then I firmly believe they would have typed it as such. Instead they have tons of helper types like EnforceDocument or SchemaDefinitionType that they use internally which allow the user just to hand over the base interface without having extended document.

Minimum reproduction code

This is a discussion of types. I cannot help you with reproduction.

Steps to reproduce

NA

Expected behavior

export declare class SchemaFactory {
    static createForClass<TClass extends any = any>(target: Type<TClass>): mongoose.Schema<TClass>;
}

Package version

9.0.0

mongoose version

6.0.10

NestJS version

8.1.1

Node.js version

14.18.1

In which operating systems have you tested?

• [ ] macOS
• [ ] Windows
• [X] Linux

Other

No response

[troywweber7]

Also, if you are looking for evidence that Mongoose doesn't expect you to hand a document to the Schema generic, then look directly at their docs: https://mongoosejs.com/docs/typescript.html. Specifically https://mongoosejs.com/docs/typescript.html#:~:text=This%20approach%20works%2C%20but%20we%20recommend%20your%20document%20interface%20not%20extend%20Document

[kamilmysliwiec]

Would you like to create a PR for this issue? As this will likely introduce a major breaking change, we'll have to wait till we can publish it anyway

[troywweber7]

When would you have to wait to release this?

Just to be extra clear, mongoose started bundling their "more correct" types and deprecated @types/mongoose in the middle of version 5. So anyone who is using Mongoose 5 and updates to the latest minor/patch of Mongoose 5 (or updates their @types/mongoose package) is already seeing these breaking changes, to no fault of NestJs.

I guess another pertinent question is: what version of mongoose and @types/mongoose does @nestjs/mongoose recommend? If you don't recommend a peer dependency to lock it down, then it's already broken, realistically speaking. I find this to be a bad move on mongoose's part, but it's just the reality of what they did.

So, just curious what the plan is as far as versioning.

For the time being, devs can get around this issue with the types by doing something like const MY_SCHEMA = SchemaFactory.createForClass(DecoratedClass) as unknown as mongoose.Schema<DecoratedClass>.

Because mongoose doesn't recommend that your interface extends Document (https://mongoosejs.com/docs/typescript.html#using-extends-document), this also may cause issues for some of this repo's documentation at https://docs.nestjs.com/techniques/mongodb. Specifically this affects your example in https://docs.nestjs.com/techniques/mongodb#model-injection with generates a CatDocument interface and anywhere else that you use this interface in the article.

I can't promise a PR. I'm doing this on work time and am considering it a courtesy to raise awareness since I took the time to read the documentation more thoroughly. But if I find enough free time, I might try to get to it if no one else already has.

[troywweber7]

If I do find the time to work on this, will I have to fork this entire repo or will I just be able to create a branch and push it to this repo?

[ajwootto]

This issue may be more urgent now that the Mongoose maintainers have plans to fully remove support for this style of extends Document typing: https://github.com/Automattic/mongoose/issues/11615

[kamilmysliwiec]

This should be fixed in 9.1.0