mongoose icon indicating copy to clipboard operation
mongoose copied to clipboard

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

Open Zenthae opened this issue 4 years ago • 29 comments

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

Zenthae avatar May 28 '21 02:05 Zenthae

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

Is this line added correctly in your code ?

DVGY avatar May 29 '21 09:05 DVGY

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

Zenthae avatar May 29 '21 21:05 Zenthae

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);

DVGY avatar May 30 '21 04:05 DVGY

same

Zenthae avatar May 30 '21 22:05 Zenthae

same

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

DVGY avatar May 31 '21 04:05 DVGY

still the same issue

Zenthae avatar May 31 '21 14:05 Zenthae

still the same issue

can you share on codepen ?

DVGY avatar May 31 '21 16:05 DVGY

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.

vkarpov15 avatar Jun 14 '21 16:06 vkarpov15

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

Zenthae avatar Jun 15 '21 01:06 Zenthae

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 avatar Jun 21 '21 16:06 vkarpov15

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

vkarpov15 avatar Jun 21 '21 18:06 vkarpov15

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

Zenthae avatar Jun 21 '21 22:06 Zenthae

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.

cakeslice avatar Nov 09 '21 19:11 cakeslice

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.

FINDarkside avatar Nov 23 '21 15:11 FINDarkside

@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.

vkarpov15 avatar Nov 27 '21 19:11 vkarpov15

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.

FINDarkside avatar Nov 27 '21 19:11 FINDarkside

@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.

mjfwebb avatar Jun 04 '22 20:06 mjfwebb

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?

andrewrjohn avatar Jun 20 '22 20:06 andrewrjohn

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

hasezoey avatar Sep 22 '22 12:09 hasezoey

@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.

vkarpov15 avatar Oct 02 '22 15:10 vkarpov15

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

hasezoey avatar Oct 02 '22 17:10 hasezoey

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. 🙂

FilipPyrek avatar Oct 12 '22 19:10 FilipPyrek

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

hasezoey avatar Oct 13 '22 07:10 hasezoey

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

FilipPyrek avatar Oct 13 '22 11:10 FilipPyrek

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)?

hasezoey avatar Oct 13 '22 12:10 hasezoey

@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.

vkarpov15 avatar Oct 24 '22 20:10 vkarpov15

@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.

FilipPyrek avatar Oct 25 '22 13:10 FilipPyrek

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?

JavaScriptBach avatar Feb 15 '23 19:02 JavaScriptBach

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

akraminakib avatar Jul 11 '24 19:07 akraminakib