-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Recent updates to mongoose.d.ts broke use of interfaces on model compilation #10110
Comments
@Ruroni Okay I see, thanks for the examples. I'll add the type paramater back onto mongoose.Model<>, I removed it because I don't think there's a general Typescript way to do the above and the way we've done it is specific to how we implemented our previous definitions so it's rigid. |
But, I figured it would break something so I'll add it back. |
@Ruroni @digital216 can you try out https://github.com/simonxca/DefinitelyTyped/blob/model-fix/mongoose/mongoose.d.ts if anything is still broken, please send me code snippets to test. Also, the interface should not extend mongoose.Document since Model.find() returns an array of mongoose.Model instances (mongoose.Model extends mongoose.Document so it should still work, but it's inaccurate and mongoose.Document does not have methods like save() or remove() or inherited EventEmitter methods). Just use:
The definitions will handle extending mongoose.Model transparently for you. I know in the previous definitions we added a bunch of methods to |
In the link click "Raw" and copy/paste. |
So far so good, I'm not running into any other issues (at least how I have it implemented) |
Great, interfaces working again. Made the changes you suggested as well to great success. Unrelated, but since DocumentArray is implemented in this new definition file:
I can assign the type Thanks for getting these definitions updated! |
@Ruroni @digital216 Sounds good thanks for the feedback! |
Adds type parameter back to mongoose.Model class fixes #10110
Out of curiosity (I'm not familiar enough with the Typing thing yet). Would the typed Model have anything to do with it not respecting the unique property? I have a field that I've set the unique on, and it's allowing me to created documents with duplicate values in that field |
I don't think it should. The type is just there to provide the compiler/IDE info about code completion and type checking, it doesn't change the behaviour. But just to be sure, can you post a simple code snippet here? |
Regarding the save() and remove() methods that were previously available through extending Document, how should queries be constructed now instead? Consider this, which worked before:
Now, the save() method is no longer available. Or this:
Now, the remove() method is no longer available. According to the official (javascript) mongoose docs, my code is fine, but not according to the new d.ts file. I'm fine with updating my code so it works with the new type definitions, but I just need to know how since there is very little documentation for it. |
@linusbrolin How are you defining your AddressTypeModel? It should be defined like this:
|
@linusbrolin You're partly right, this is fine according to the docs:
But this is not:
Since AddressType extends Document, but Document does not have a save() or remove() method. |
Yes, I've defined the AddressTypeModel like you described:
So, should dbAddressType in the query be of type AddressTypeModel instead of type AddressType? If I do this, I get an error on the type:
|
Okay good, try removing both types on |
But isn't the point of TypeScript to have typed variables?
But then the code will not be as readable in a generic text editor. If you could make the types public in the d.ts, then I could just add them in my code for readability, but for example:
is not public in your d.ts file. |
The type returned is |
@linusbrolin it's because |
@linusbrolin so we use |
Okay, I think I understand it now.. Thanks for explaining it. :) |
@linusbrolin yea, it is more cumbersome heh :) I know other libraries do this and it's never readable when you hover over in the editor. |
@Ruroni @digital216 @linusbrolin Heads up. Some changes are coming soon for Typescript 2. See here: https://github.com/simonxca/DefinitelyTyped/blob/patch-mongoose/mongoose/mongoose.d.ts After talking with the guys at typings, I changed the implementation of I've added a README of how to use the definitions in case there are other changes that affect you. (I don't think so for now) https://github.com/simonxca/DefinitelyTyped/tree/patch-mongoose/mongoose |
@simonxca Sounds good. |
@linusbrolin alright it's been updated you can give it a try and let me know if you get any errors |
@simonxca Alright, I've modified my code, but I get a few errors.
In the User model, where I do use both passport-local-document and mongoose-paginate, I get the following errors
The 'increment' error is probably from a third plugin I use on some other models, mongoose-sequence. |
@linusbrolin Thanks for the examples. Looking into this. |
@linusbrolin Looks like Typescript is getting confused by all the overrides to In your first example change: In your second example change: Give that a try and see if those errors go away. |
@linusbrolin In the definitions, I had to change: The first version works for Typescript 2.x but not for Typescript 1.x apparently and will cause errors when I commit to DefinitelyTyped. I believe you can fix this error by either: or by upgrading your Typescript compiler to version 2.
|
@simonxca that seems to have fixed the errors, great! :) I have another question, though. ;) For example, I'm building type definitions for the mongoose-sequence plugin, which look like this:
If I remove the overload plugin line:
then I get this error if I try to load a different plugin on the same schema that is using monoose-sequence:
|
@linusbrolin Hmm I asked StackOverflow and apparently it simply doesn't work in Typescript. So you'll have to provide the parent class's signature for |
@simonxca yeah, I figured that's how it was.. Bummer. :/ In the mean time, it means you should probably update your mongoose.d.ts plugin guide to include this information. I will update passport-local-mongoose.d.ts to include the overload, and publish the mongoose-sequence.d.ts |
@simonxca the version of mongoose.d.ts at DefinitelyTyped is not your last version Do you know when will the version be updated? |
@camilleblanc I'm not sure how |
@simonxca thanks, still waiting for your update on that.
Now somewhere I have the _id of a MySubEntity, and I have retrieved the MyEntity where it is.
The problem is: _id is not a property of the interface MySubEntity, but it has one because mongo creates an _id for subdocuments. So my question is: what is the correct way to declare sub documents? I hope my post is clear enough to understand! |
@camilleblanc sorry was busy this week. Looks like it gets auto-updated from what I've read, but doesn't look like mongoose is up to date. I followed up with them here: #11210 (comment) |
@camilleblanc also this thread is getting too long. Can you please open a new issue with your problem above? I'll respond there. |
mongoose/mongoose.d.ts
file in this repo and had problems.Basically this broke my use of mongoose. Not that I am sure I was using it correctly as I'm a bit of a novice in this TS/JS realm.
Snippet:
So I can no longer specify the interface with additional properties within <>.
Simplifying to:
mongoose.model("Location", locationSchema);
Works of course, but now in my controllers when executing a query and explicitly trying to access a property of a document TS is unaware of the additional properties on a document.
Then I thought perhaps creating a class that extends mongoose and implements my interface would work, but then I have to provide an implementation for everything in my interface and mongoose.Document. That seems kind of asinine so I figure I must be missing the most obvious way to do this.
Thoughts?
The text was updated successfully, but these errors were encountered: