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

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

Open
Zenthae opened this issue May 28, 2021 · 29 comments
Open

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

Zenthae opened this issue May 28, 2021 · 29 comments
Labels
developer-experience This issue improves error messages, debugging, or reporting typescript Types or Types-test related issue / Pull Request

Comments

@Zenthae
Copy link

Zenthae commented May 28, 2021

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
Copy link

DVGY commented May 29, 2021

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

Is this line added correctly in your code ?

@Zenthae
Copy link
Author

Zenthae commented May 29, 2021

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

@DVGY
Copy link

DVGY commented May 30, 2021

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
Copy link
Author

Zenthae commented May 30, 2021

same

@DVGY
Copy link

DVGY commented May 31, 2021

same

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

@Zenthae
Copy link
Author

Zenthae commented May 31, 2021

still the same issue

@DVGY
Copy link

DVGY commented May 31, 2021

still the same issue

can you share on codepen ?

@IslandRhythms IslandRhythms added typescript Types or Types-test related issue / Pull Request confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. labels Jun 3, 2021
@vkarpov15 vkarpov15 added this to the 5.12.14 milestone Jun 4, 2021
@vkarpov15 vkarpov15 removed the confirmed-bug We've confirmed this is a bug in Mongoose and will fix it. label Jun 4, 2021
@vkarpov15
Copy link
Collaborator

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 vkarpov15 removed this from the 5.12.14 milestone Jun 14, 2021
@vkarpov15 vkarpov15 added help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary and removed typescript Types or Types-test related issue / Pull Request labels Jun 14, 2021
@Zenthae
Copy link
Author

Zenthae commented Jun 15, 2021

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 vkarpov15 reopened this Jun 17, 2021
@vkarpov15 vkarpov15 added this to the 5.12.15 milestone Jun 17, 2021
@vkarpov15 vkarpov15 added developer-experience This issue improves error messages, debugging, or reporting and removed help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary labels Jun 17, 2021
@vkarpov15
Copy link
Collaborator

We should see if:

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

helps, re: #10343

@vkarpov15 vkarpov15 removed the developer-experience This issue improves error messages, debugging, or reporting label Jun 21, 2021
@vkarpov15
Copy link
Collaborator

We took a look and unfortunately the suggestion in #10302 (comment) 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 vkarpov15 removed this from the 5.12.15 milestone Jun 21, 2021
@vkarpov15 vkarpov15 added this to the 5.x Unprioritized milestone Jun 21, 2021
@vkarpov15 vkarpov15 added typescript Types or Types-test related issue / Pull Request developer-experience This issue improves error messages, debugging, or reporting labels Jun 21, 2021
@Zenthae
Copy link
Author

Zenthae commented Jun 21, 2021

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

vkarpov15 added a commit that referenced this issue Jul 28, 2021
@cakeslice
Copy link

cakeslice commented Nov 9, 2021

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
Copy link

FINDarkside commented Nov 23, 2021

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
Copy link
Collaborator

@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
Copy link

FINDarkside commented Nov 27, 2021

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
Copy link

mjfwebb commented Jun 4, 2022

@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
Copy link

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
Copy link
Collaborator

is this still a issue with mongoose 6.6.1?

currently new types are:

new <DocType = T>(doc?: DocType, fields?: any | null, options?: boolean | AnyObject): HydratedDocument<T, TMethodsAndOverrides,

@vkarpov15
Copy link
Collaborator

@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
Copy link
Collaborator

hasezoey commented Oct 2, 2022

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:

interface Model<T, TQueryHelpers = {}, TMethodsAndOverrides = {}, TVirtuals = {}, TSchema = any> extends

generic DocType (and the whole function declaration):

new <DocType = T>(doc?: DocType, fields?: any | null, options?: boolean | AnyObject): HydratedDocument<T, TMethodsAndOverrides,


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
Copy link

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
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link

@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
Copy link
Contributor

JavaScriptBach commented Feb 15, 2023

#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
Copy link

akraminakib commented Jul 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience This issue improves error messages, debugging, or reporting typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

No branches or pull requests