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.

Sign up for GitHub

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

Jump to bottom

fix: improve populate type narrowing #11503

Merged
merged 8 commits into from Mar 11, 2022
Merged

fix: improve populate type narrowing #11503

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0b9dc6f
fix populate type narrowing
Uzlopak Mar 8, 2022
1a01dfd
Fix populate type narrowing issue
mohammad0-0ahmad Mar 8, 2022
3c57076
Merge branch 'Automattic:master' into gh11496
mohammad0-0ahmad Mar 8, 2022
ebbebd7
Refact UnpackedIntersection type
mohammad0-0ahmad Mar 9, 2022
6dffc06
Remove commented line
mohammad0-0ahmad Mar 10, 2022
69eb9bc
Refact UnpackedIntersection & delete UnpackedIntersectionWithNull
mohammad0-0ahmad Mar 10, 2022
643a38d
Refact UnpackedIntersection type & delete two wrong tests
mohammad0-0ahmad Mar 11, 2022
2c04ac9
refact
mohammad0-0ahmad Mar 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 33 additions & 1 deletion test/types/populate.test.ts
View file
Open in desktop
@@ -1,7 +1,7 @@
import { Schema, model, Document, PopulatedDoc, Types, HydratedDocument } from 'mongoose';
// Use the mongodb ObjectId to make instanceof calls possible
import { ObjectId } from 'mongodb';
import { expectError } from 'tsd';
import { expectAssignable, expectError, expectType } from 'tsd';

interface Child {
name: string;
Expand Down Expand Up @@ -149,4 +149,36 @@ function gh11321(): void {
return 'foo';
}
});
}

function gh11503() {
interface Friend {
blocked: boolean
}
const FriendSchema = new Schema<Friend>({
blocked: Boolean
});

interface User {
friends: Types.ObjectId[];
}
const UserSchema = new Schema<User>({
friends: [{ type: Schema.Types.ObjectId, ref: 'friends' }]
});
const Users = model<User>('friends', UserSchema);

Users.findOne({}).populate('friends').then(user => {
expectType<Types.ObjectId | undefined>(user?.friends[0]);
expectError(user?.friends[0].blocked);
expectError(user?.friends.map(friend => friend.blocked));
});

Users.findOne({}).populate<{friends: Friend[]}>('friends').then(user => {
expectAssignable<Friend>(user?.friends[0]);
expectType<boolean>(user?.friends[0].blocked);
expectType<Types.ObjectId>(user?.friends[0]._id);
const firstFriendBlockedValue = user?.friends.map(friend => friend)[0];
expectType<boolean>(firstFriendBlockedValue?.blocked);
expectType<Types.ObjectId>(firstFriendBlockedValue?._id);
});
}
4 changes: 2 additions & 2 deletions test/types/queries.test.ts
View file
Open in desktop
Expand Up @@ -18,11 +18,11 @@ const schema: Schema<ITest, Model<ITest, QueryHelpers>, {}, QueryHelpers> = new
endDate: Date
});

schema.query._byName = function(name: string): QueryWithHelpers<any, ITest, QueryHelpers> {
schema.query._byName = function(name: string): QueryWithHelpers<ITest[], ITest, QueryHelpers> {
return this.find({ name });
};

schema.query.byName = function(name: string): QueryWithHelpers<any, ITest, QueryHelpers> {
schema.query.byName = function(name: string): QueryWithHelpers<ITest[], ITest, QueryHelpers> {
expectError(this.notAQueryHelper());
return this._byName(name);
};
Expand Down
11 changes: 7 additions & 4 deletions types/index.d.ts
View file
Open in desktop
Expand Up @@ -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>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need UnpackedIntersectionWithNull type?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

type UnpackedIntersection<T, U> = T extends (infer A)[]
? (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]};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added HydratedDocument To populated property, is that correct? Else tests at line will fail at lines mentioned with #HydratedDocument.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I will refact that in the next commit :)

Copy link
Collaborator

@Uzlopak Uzlopak Mar 11, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your words, no worries it was funny actually at the beginning I though that as well but it's works now.


type ProjectionFields<DocType> = {[Key in keyof Omit<LeanDocument<DocType>, '__v'>]?: any} & Record<string, any>;

Expand Down Expand Up @@ -2115,8 +2118,8 @@ declare module 'mongoose' {
polygon(path: string, ...coordinatePairs: number[][]): this;

/** Specifies paths which should be populated with other documents. */
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>;
Copy link
Collaborator

@Uzlopak Uzlopak Mar 11, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #11518 we got a regression reported.

Suggested change
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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vkarpov15 Only the second populate type can handle string arrays correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OR maybe only the first one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets wait for feedback by vkarpov

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/** Get/set the current projection (AKA fields). Pass `null` to remove the current projection. */
projection(): ProjectionFields<DocType> | null;
Expand Down