fix: improve populate type narrowing #11503
Conversation
|
This one is better. |
types/index.d.ts
Outdated
| @@ -1813,7 +1813,8 @@ declare module 'mongoose' { | |||
|
|
|||
| type QueryWithHelpers<ResultType, DocType, THelpers = {}, RawDocType = DocType> = Query<ResultType, DocType, THelpers, RawDocType> & THelpers; | |||
|
|
|||
| type UnpackedIntersection<T, U> = T extends (infer V)[] ? (V & U)[] : T & U; | |||
| type UnpackedIntersection<T, U> = T extends (infer A)[] ? (A & U)[] : keyof U extends never ? T & U: { [P in keyof U]: U[P] extends (infer B)[] ? (B & T)[] : U[P] & T }; | |||
There was a problem hiding this comment.
Shouldn't the last bit be something like T[P] & U[P]? I think T should come first, and then we should be merging T's keys as well as opposed to merging T into U's keys.
There was a problem hiding this comment.
Check the new commit
test/types/populate.test.ts
Outdated
|
|
||
| Users.findOne({}).populate<{friends: Friend[]}>('friends').then(user => { | ||
| expectAssignable<Friend | undefined>(user?.friends[0]); | ||
| user?.friends[0].blocked; |
There was a problem hiding this comment.
Can we do expectType<boolean> here?
| @@ -1813,8 +1813,11 @@ declare module 'mongoose' { | |||
|
|
|||
| type QueryWithHelpers<ResultType, DocType, THelpers = {}, RawDocType = DocType> = Query<ResultType, DocType, THelpers, RawDocType> & THelpers; | |||
|
|
|||
| type UnpackedIntersection<T, U> = T extends (infer V)[] ? (V & U)[] : T & U; | |||
| type UnpackedIntersectionWithNull<T, U> = T extends null ? UnpackedIntersection<T, U> | null : UnpackedIntersection<T, U>; | |||
There was a problem hiding this comment.
Do we really need UnpackedIntersectionWithNull type?
There was a problem hiding this comment.
We introduced UnpackedIntersectionWithNull to fix #11041. Although it looks like the test for that issue still passes with this change, so looks like we don't need it after all.
types/index.d.ts
Outdated
| ? (Omit<A, keyof U> & U)[] | ||
| : keyof U extends never | ||
| ? T | ||
| : Omit<T, keyof U> & { [P in keyof U]: U[P] extends (infer B)[] ? (HydratedDocument<B> & B)[] : HydratedDocument<U[P]> & U[P]}; |
There was a problem hiding this comment.
I've added HydratedDocument To populated property, is that correct? Else tests at line will fail at lines mentioned with #HydratedDocument.
There was a problem hiding this comment.
Well hmm, actually the thing is, that it is only a HydratedDocument if in populate the option lean is set to false or lean() is used on the query builder. So I suspect this is wrong :/
There was a problem hiding this comment.
Alright, I will refact that in the next commit :)
There was a problem hiding this comment.
@mohammad0-0ahmad
Imho, if you cant fix that than I would actually recommend to remove those expectTypes, which vkarpov15 requested and revert those HydratedDocument changes, if this is a hard case to solve for you. (Not suggesting that this is over your skills). Probably this is happening because tsd uses a different typescript version and thus resulting in some issues which dont happen with latest typescript, which is used in vscode for their intellisense. So this is maybe some kind of scope creep. It was not even originally a task for you. So I actually dont want that you feel forced to solve this issue.
But if you think this is a nice challenge and you really want to supply an A-Grade solution, than feel free to invest more time and energy. And know that your work is atleast by me appreciated.
There was a problem hiding this comment.
Thanks for your words, no worries it was funny actually at the beginning I though that as well but it's works now.
|
I've refacted this & it should work with both VSC intellisense & tsd. |
| populate<Paths = {}>(path: string, select?: string | any, model?: string | Model<any, THelpers>, match?: any): QueryWithHelpers<UnpackedIntersectionWithNull<ResultType, Paths>, DocType, THelpers, RawDocType>; | ||
| populate<Paths = {}>(options: PopulateOptions | Array<PopulateOptions>): QueryWithHelpers<UnpackedIntersectionWithNull<ResultType, Paths>, DocType, THelpers, RawDocType>; | ||
| populate<Paths = {}>(path: string, select?: string | any, model?: string | Model<any, THelpers>, match?: any): QueryWithHelpers<UnpackedIntersection<ResultType, Paths>, DocType, THelpers, RawDocType>; | ||
| populate<Paths = {}>(options: PopulateOptions | Array<PopulateOptions>): QueryWithHelpers<UnpackedIntersection<ResultType, Paths>, DocType, THelpers, RawDocType>; |
There was a problem hiding this comment.
In #11518 we got a regression reported.
| populate<Paths = {}>(options: PopulateOptions | Array<PopulateOptions>): QueryWithHelpers<UnpackedIntersection<ResultType, Paths>, DocType, THelpers, RawDocType>; | |
| populate<Paths = {}>(options: string[] | PopulateOptions | Array<PopulateOptions>): QueryWithHelpers<UnpackedIntersection<ResultType, Paths>, DocType, THelpers, RawDocType>; |
Corresponding typing test would be:
async function gh11518() {
// populate should accept an Array with the populated paths
await Story.findOne().populate(["author"]);
}If you want, we can do this in this PR. If not, I can create a PR. I just didnt wanted to create a PR before this is merged to avoid high probable merge conflict.
There was a problem hiding this comment.
@vkarpov15 Only the second populate type can handle string arrays correct?
There was a problem hiding this comment.
OR maybe only the first one?
There was a problem hiding this comment.
lets wait for feedback by vkarpov
There was a problem hiding this comment.
That's an independent issue that I fixed in #11518, no need to discuss that in this PR. But yes, you can call both syntaxes with an array of paths.
Should resolve #11496
@Uzlopak