Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 1 commit
2dbe288
03e34db
ec6eb3d
9321fd5
2145afa
34ae74c
967bd65
e75d4b6
ff2bf3e
1bf940e
4a5d712
295bad2
cd215a7
a7967d9
e9d4491
526ff6a
411350b
b942ce8
6bcb7a4
56af40b
9cdec80
b3e9668
a42bbde
302de56
96571b1
897452b
503c0d1
aa6497d
a3b32ac
569d4eb
0fe4928
f4f17d7
6ef2f6c
6a77f09
f21dfbe
7093195
555db64
0131b7e
47469d6
e0d299f
e137a6d
6547fe2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 clauseT & Required<{ _id: U }>
is never used.Thus,
_id
will be always infered with typeObjectId
, even if I have defined another type such asString
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:
The type of variable
theId
will bestring | Types.ObjectId
, rather thanstring
.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:
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.
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,
Both
t2_old
andt2_new
isnever
(which I think is expected behaviour), and for t1,t1_old
is{_id: never}
butt1_new
isnever
(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: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 isnever
, because the following code:gives error output
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:
when testing it with
tsd
, gives the following error:According to @mohammad0-0ahmad :
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.