Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
26.94k stars 3.84k forks source link

[Typscript] Wrong type for `new Model(doc)` #10302

Open Zenthae opened 3 years ago

Zenthae commented 3 years ago

Do you want to request a feature or report a bug?

bug

What is the current behavior?

When using new Model(doc) to create a new model and the save() it. the type of doc is always any which break the auto-completion for properties added by the user model.

image

If the current behavior is a bug, please provide the steps to reproduce.

Quick Start example from the website

tsconfig.json

{
  "compilerOptions": {
    "target": "ES2020",
    "module": "commonjs",
    "outDir": "./dist",
    "rootDir": "./src",
    "strict": true,
    "moduleResolution": "node",
    "types": ["mongoose"],
    "esModuleInterop": true,
    "skipLibCheck": true,
    "forceConsistentCasingInFileNames": true
  },
  "exclude": ["node_modules"],
  "include": ["src/**/*.ts"]
}

What is the expected behavior?

instead of having any it should use the provided type.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

nodejs: v16.1.0 mongoose: 5.12.10

DVGY commented 3 years ago

const Kitten = mongoose.model('Kitten', kittenSchema);

Is this line added correctly in your code ?

Zenthae commented 3 years ago

yes it is, it's the line hidden by Vscode popup

DVGY commented 3 years ago

yes it is, it's the line hidden by Vscode popup @Zenthae change the line from this

const Kitten = mongoose.model('Kitten', kittenSchema);

to this const Kitten = mongoose.model<Ikitten>('Kitten', kittenSchema);

Zenthae commented 3 years ago

same

DVGY commented 3 years ago

same

@Zenthae one more thing make sure your interface IKitten extends Document is like this

Zenthae commented 3 years ago

still the same issue

DVGY commented 3 years ago

still the same issue

can you share on codepen ?

vkarpov15 commented 3 years ago

Make sure you're running Mongoose >= 5.12.6, we changed this type definition with #10074 so either you're using an older version of Mongoose or VSCode is picking up the wrong version of Mongoose type definitions.

Zenthae commented 3 years ago

i'm using mongoose 5.12.13, checked the type definition,

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

but it's still not working, the inferred type is any for new()

here is my model

interface UserModel extends IUser, Document {
  comparePassword(plainPassword: string): boolean;
}

export const userSchema = new Schema<UserModel>({
  username: { type: String, unique: true },
  password: String,
});

userSchema.pre('save', function (this: UserModel, next) {
  if (!this.isModified('password')) return next();

  this.password = hashSync(this.password, 15);

  next();
});

userSchema.method('comparePassword', function (this: UserModel, plainPassword: string) {
  return compareSync(plainPassword, this.password);
});

export const User = model<UserModel>('User', userSchema);

const u = new User({});

i tested using this too, and it's still inferring any, so maybe it's my vscode that doing something wrong but then i don't now how to even start fixing or testing it ....

vkarpov15 commented 3 years ago

We should see if:

new(doc?: T): EnforceDocument<T, TMethods>;
new(doc?: any): EnforceDocument<T, TMethods>;

helps, re: https://github.com/Automattic/mongoose/pull/10343

vkarpov15 commented 3 years ago

We took a look and unfortunately the suggestion in https://github.com/Automattic/mongoose/issues/10302#issuecomment-865185892 doesn't really help. TypeScript just always falls back to any if any required properties in T are missing. You should use as User if you want to get better VS code autocomplete:

interface User {
  name: string;
  id: number;
}

const UserModel = model<User>('User', new Schema({ name: String, id: Number }));

const doc = new UserModel({ name: 'test' } as User);

An alternative that we'll consider for the future is making UserModel() strongly typed for better autocomplete, but leaving new UserModel() the way it currently is. So the below TypeScript defs:

    new(doc?: T | any): EnforceDocument<T, TMethods>;
    (doc?: T): EnforceDocument<T, TMethods>;

Another potential alternative would be creating a separate static function, like from(), that allows for better autocomplete. Like UserModel.from({ ... })

Zenthae commented 3 years ago

i think that the from() solution hold value, because at the only way, (from my knowledge, which ins't lot) to create a new Model instance is the create() function which automatically add it to the database, which might not be the desired outcome

cakeslice commented 2 years ago

You should use as User if you want to get better VS code autocomplete:

I think there should be a way to enforce object creation that matches the schema.

const doc = new UserModel({ name: 'test' } as User); is error prone. All it takes is to forget the "as User" and then: const doc = new UserModel({ namee: 'test' }); goes through.

Not to mention that adding "as User" everywhere is not practical.

FINDarkside commented 2 years ago

An alternative that we'll consider for the future is making UserModel() strongly typed for better autocomplete, but leaving new UserModel() the way it currently is.

Is there a reason why someone would not want it to be strongly typed? This is kinda a bummer since AFAIK we had strongly typed constructor params and create function until mongoose started providing its own types and @types/mongoose wasn't maintained anymore.

All it takes is to forget the "as User" and then:

It'll also happily cast unknown types to whatever they need to be because you're not telling ts to ensure that it's User. You're telling ts that this certainly is an User and ts will only complain if it can be certain that you're wrong.

vkarpov15 commented 2 years ago

@FINDarkside "Is there a reason why someone would not want it to be strongly typed?" <-- Yes, if you want to pass in req.body or some other arbitrary object.

FINDarkside commented 2 years ago

It'd still work if req.body is any and not unknown. If it's unknown you can just cast it to any. It'd be breaking change for sure, but the fix would be trivial. But didn't really consider that unknown technically would be valid type for it since mongoose does the validation, but personally I feel like disallowing unknown to get proper typing would be a good tradeoff.

mjfwebb commented 2 years ago

@FINDarkside "Is there a reason why someone would not want it to be strongly typed?" <-- Yes, if you want to pass in req.body or some other arbitrary object.

I'm not sure about your reasoning here @vkarpov15. This is a confusing reason to default to any.

From a user of Typescript perspective, I'd want new MyModel to expect input parameters as defined in the interface I had explicitly associated with the model.

While there may well be users that want to pass in req.body or an arbitrary object, I'd argue that they should be the ones to explicitly code in that intention.

andrewrjohn commented 2 years ago

From a user of Typescript perspective, I'd want new MyModel to expect input parameters as defined in the interface I had explicitly associated with the model.

This is exactly what I'd expect to be getting from Mongoose when using Typescript (and I think most people would agree). Does anybody know if there is progress being made on this?

hasezoey commented 2 years ago

is this still a issue with mongoose 6.6.1?

currently new types are: https://github.com/Automattic/mongoose/blob/69c99d2d9ee4d13f1635eaa63d042f3a6a6fb7a3/types/models.d.ts#L124

vkarpov15 commented 2 years ago

@hasezoey oddly enough, I think this behavior is still there. I don't think this behavior is an issue though.

For example, the following script compiles fine.

import mongoose from 'mongoose';

const schema = new mongoose.Schema<{ name?: string }>({ name: String });
const Test = mongoose.model<{ name?: string }>('Test', schema);

const doc = new Test({ name: 42, other: 'bar' });

And VS Code autocomplete thinks that DocType is {} ?

image

I genuinely am not sure why.

hasezoey commented 2 years ago

And VS Code autocomplete thinks that DocType is {} ? I genuinely am not sure why.

well in this case it is because the parameter doc uses the function generic DocType, which is defaulted to a unbound T generic in Model, and because it is a "creating method" (how i call it), it uses the value it finds as that generic

TL;DR: the type for parameter doc is unbound, so it uses the value it has, which is {}

generic T: https://github.com/Automattic/mongoose/blob/ff7eed50ead64d209b9d33a6b4fff58d41d7c3b1/types/models.d.ts#L119

generic DocType (and the whole function declaration): https://github.com/Automattic/mongoose/blob/ff7eed50ead64d209b9d33a6b4fff58d41d7c3b1/types/models.d.ts#L124


so basically, i answered my own question:

is this still a issue with mongoose 6.6.1?

yes it is still a issue in mongoose 6.6.3

FilipPyrek commented 2 years ago

Hi there,

I just entered mongoose world and I want it to use in my TypeScript project . So far the setup was okay... but I found out same issue as @hasezoey is talking about in his latest message.

Wrong mongoose type definition in Model:

new <DocType = T>(doc?: DocType, fields?: any | null, options?: boolean | AnyObject): HydratedDocument<T, TMethodsAndOverrides, IfEquals<TVirtuals, {}, ObtainSchemaGeneric<TSchema, 'TVirtuals'>, TVirtuals>> & ObtainSchemaGeneric<TSchema, 'TStaticMethods'>;

produces incorrect behavior when creating new instance of a model image image No type error message coming from TypeScript even though the input doesn't comply with the schema ☝️

When I fix it to

 new (doc?: T, fields?: any | null, options?: boolean | AnyObject): HydratedDocument<T, TMethodsAndOverrides, IfEquals<TVirtuals, {}, ObtainSchemaGeneric<TSchema, 'TVirtuals'>, TVirtuals>> & ObtainSchemaGeneric<TSchema, 'TStaticMethods'>;

Then I'm getting the correct TypeScript error: image

If needed, I can contribute to help to solve this issue ASAP. 🙂

hasezoey commented 2 years ago

If needed, I can contribute to help to solve this issue ASAP. slightly_smiling_face

the change you have made would conflict with other things mongoose supports, at least from what i can tell: because before a save (/ validate) values can be added / changed and are not required to be set in new Model directly, but this is a different story for Model.create.

the proper thing for this issue is to bind T to something instead of being unbound

FilipPyrek commented 2 years ago

Well, that's quite unfortunate because it breaks Developer Experience when trying to ensure type safety with TypeScript.

Maybe it could be done via conditional generic parameters like this for example:

// just a quick illustration of the idea

 new <DocType = T>(doc?: DocType extends 'full' ? T : DocType, fields?: any | null, options?: boolean | AnyObject): HydratedDocument<T, TMethodsAndOverrides, IfEquals<TVirtuals, {}, ObtainSchemaGeneric<TSchema, 'TVirtuals'>, TVirtuals>> & ObtainSchemaGeneric<TSchema, 'TStaticMethods'>;
image
hasezoey commented 2 years ago

i dont think it is a good way to use DocType itself, but ok at showcasing what you mean @vkarpov15 @AbdelrahmanHafez what do you think about this proposed change (conditional generic for strict types)?

vkarpov15 commented 2 years ago

@FilipPyrek how exactly is your suggested approach different from what is already there? The DocType generic is already T by default, so if you don't set any generic you should get the same result.

FilipPyrek commented 2 years ago

@FilipPyrek how exactly is your suggested approach different from what is already there? The DocType generic is already T by default, so if you don't set any generic you should get the same result.

That suggested solution doesn't create a schema breaking change. The problem is that DocType is actually never using the default type T, because DocType type is always available (even if it's just of type {}), thus you never get to the default T type.

In the suggested solution you can choose if you want the original "broken" approach or if you want doc to be always of type T, thus get correct TypeScript checks.

JavaScriptBach commented 1 year ago

https://github.com/Automattic/mongoose/pull/13038 fixes this for Model.create(). @vkarpov15 @hasezoey would you be willing to accept this patch?

edit: reading through this thread, I guess it's similar to what @FilipPyrek has been suggesting. So the question becomes: would you be willing to put this change in Mongoose 7 so Typescript users can have strict types by default?

akraminakib commented 3 months ago

This is still an issue with v8 of mongoose even when following the examples from the docs. Running the below lines infers the results on each to be of type any:

const user1 = await User.findOne({}).exec();

// where params is an object that represents the valid fields defined on the user schema
const user2 = new User({...params});

Edit: Nevermind, this was resolved by upgrading my typescript from 3.9.9 -> 5.X