-
-
Notifications
You must be signed in to change notification settings - Fork 3.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
Feature: Infer schema type automatically. #11563
Feature: Infer schema type automatically. #11563
Conversation
ed1eb00
to
c6bd5c3
Compare
3ced3a7
to
c4bc17e
Compare
When do you think this PR will be ready for merge? |
When it is reviewed 🙂 @Uzlopak |
c4bc17e
to
64d4397
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not easy to digest :D
Made some comments.
@Uzlopak |
@vkarpov15 |
Maybe we should talk about documenting https://github.com/Automattic/mongoose/blob/master/docs/typescript.md |
e00c561
to
d3e3b2f
Compare
338a9e5
to
1347304
Compare
Hello @vkarpov15! |
when will typing become available for general use? |
I am waiting for @vkarpov15 review. |
I hope you are still motivated. Issue is that complex typescript PR are always leaving an unease feeling in the stomach. So dont take it personally. :) Can you check please if we need some documentation changes? https://github.com/Automattic/mongoose/tree/master/docs/typescript |
I am still motivated, and I understand that, but I've provided some JS docs for the types, and I ready to explain the unclear things in this PR. |
1347304
to
eb84d41
Compare
8253a3a
to
9867f6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the type key support and auto typed schemas. But my big concerns are 1) performance, 2) backwards btreaking changes. Our TS benchmark shows before this PR:
Run node ./benchmarks/typescript
simple instantiations: 35805
simple memory used: 152972.7
simple check time: 3.089
simple total time: 4.831999999999999
After this PR:
Run node ./benchmarks/typescript
simple instantiations: 227139
simple memory used: 191137.1
simple check time: 3.8959999999999995
simple total time: 5.3
After spending over 6 months fixing the performance of our TypeScript types, I do not want to merge something that undoes all the progress we made in #10349.
98d75d4
to
e137a6d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks 👍 We'll merge this into the 6.4 branch. Thanks for all your hard work on this.
Thank you @Uzlopak @vkarpov15 as well. |
Hi, and thanks for all the work done here. I see that mongoose-tsgen is no longer updated due to this PR. Will it be possible to export these inferred types as separate files? It would be useful to use these types or interfaces elsewhere, for example on the frontend, where mongoose is not installed, or converting them to json schemas, to perform input validation on both the frontend and backend. |
@magyarb I'm open to continuing support for mongoose-tsgen if it is still useful to the community. I assumed that it became redundant with the type inference, but the use case for exporting types still exists so might be worth maintaining for now. We could also look at integrating something like it into Mongoose natively if that's something the team is interested in. Happy to help in whatever method is preferred! |
It might be great if you can make it availabe internally @francescov1 or at least mention that mongoose-tsgen can generate types files in mongoose docs. |
@francescov1 I'd say continue to support mongoose-tsgen for now 👍 |
@vkarpov15 Will do 👍 |
@@ -101,7 +101,15 @@ declare module 'mongoose' { | |||
[k: string]: any | |||
} | |||
|
|||
export type Require_id<T> = T extends { _id?: any } ? (T & { _id: T['_id'] }) : (T & { _id: Types.ObjectId }); | |||
export type Require_id<T> = T extends { _id?: infer U } | |||
? U extends any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that I'm not excellent in Typescript, but what does U extends any
here mean?
As far as I know, everything extends any? so the contiditional clause (T & { _id: Types.ObjectId })
is always used, and the clause T & Required<{ _id: U }>
is never used.
Thus, _id
will be always infered with type ObjectId
, even if I have defined another type such as String
in the Schema definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example:
import {model, Schema} from "mongoose";
const schema_with_string_id = new Schema({_id: String, nickname: String})
const theModel = model('test', schema_with_string_id)
const obj = new theModel()
const theId = obj._id
The type of variable theId
will be string | Types.ObjectId
, rather than string
.
Is this designed behaviour for some reason I don't know, or just a bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohammad0-0ahmad @Uzlopak Sorry for bothering you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Starrah !
Firstly, you are not bothering, and you are totally right.
I think this meant to be something like:
export type Require_id<T> = T extends { _id?: infer U }
? IfEquals<U, any> extends true
? (T & { _id: Types.ObjectId })
: T & Required<{ _id: U }>
: T & { _id: Types.ObjectId };
Thanks for your review, I will make sure to refactor this.
What are your thoughts @Uzlopak about that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did this to check if T is not never.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I did this to check if T is not never.
First, does you mean you did that to check if {_id: }
in T is not never, rather than check T?
Because according to my experiment,
import {Require_id, Types} from "mongoose";
type Required_id_old<T> = T extends { _id?: any } ? (T & { _id: T['_id'] }) : (T & { _id: Types.ObjectId });
type Required_id_new<T> = T extends { _id?: infer U }
? U extends any
? (T & { _id: Types.ObjectId })
: T & Required<{ _id: U }>
: T & { _id: Types.ObjectId };
type t1_old = Required_id_old<{_id: never}>
type t1_new = Required_id_new<{_id: never}>
type t2_old = Required_id_old<never>
type t2_new = Required_id_new<never>
Both t2_old
and t2_new
is never
(which I think is expected behaviour), and for t1, t1_old
is {_id: never}
but t1_new
is never
(I have no idea which is your expected behaviour).
Then, if my assumption is true and you does mean check if
{_id: } in T is not never
, then maybe the two conditional clause should be swapped, as the following:
type Required_id_new<T> = T extends { _id?: infer U }
? U extends any
? T & Required<{ _id: U }>
: (T & { _id: Types.ObjectId })
: T & { _id: Types.ObjectId };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, my Typescript version is 4.7.2. I surprisingly found that T extends any? true: false
equals to never when T is never
, because the following code:
type Test<T> = T extends any ? true : false
type A = Test<never>
const a: A = false
gives error output
Error:(3, 7) TS2322: Type 'boolean' is not assignable to type 'never'.
The behaviour is the same in Typescript 4.6.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess, I should have implemented appropriate typings tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohammad0-0ahmad @Uzlopak
Though with the discussion above, we (or just I) cannot still figure out what the codes here means, specifically, what does the U extends any
mean.
However, I want to point out that the following test case written by me, got failed:
// index.test-d.ts
import {model, Schema} from "mongoose";
import {expectType} from "tsd";
const schema_with_string_id = new Schema({_id: String, nickname: String})
const theModel = model('test', schema_with_string_id)
const obj = new theModel()
expectType<string>(obj._id)
when testing it with tsd
, gives the following error:
index.test-d.ts:8:0
✖ 8:0 Parameter type string is declared too wide for argument type string & ObjectId.
According to @mohammad0-0ahmad :
Hello @Starrah ! Firstly, you are not bothering, and you are totally right. I think this meant to be something like:
export type Require_id<T> = T extends { _id?: infer U } ? IfEquals<U, any> extends true ? (T & { _id: Types.ObjectId }) : T & Required<{ _id: U }> : T & { _id: Types.ObjectId };Thanks for your review, I will make sure to refactor this. What are your thoughts @Uzlopak about that?
I think that the test case failure above indicates a bug. So, I decide to raise a new issue (#12070) to report and/or further discuss this bug.
fix(types): apply changes from #11563 to "connection.model"
Does this work for method too? |
Tasks to be implemented :
Status:
Do you have any suggestions or ideas to be implemented in this PR, please leave a comment 🙂
Get it early:
You can install mongoose including this feature by running:
Screenshots :