-
-
Notifications
You must be signed in to change notification settings - Fork 4k
BREAKING CHANGE: consolidate RootQuerySelector, Condition, etc. types with MongoDB driver's #15593
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
Conversation
… with MongoDB driver's
hasezoey
left a comment
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.
On the surface this looks like a good change, but in its current state it causes some errors like the 2 below in typegoose:
src/typeguards.ts:21:12 - error TS2677: A type predicate's type must be assignable to its parameter's type.
Type 'Array<DocumentType<NonNullable<T>>>' is not assignable to type 'Array<Ref<T, S>>'.
Type 'DocumentType<NonNullable<T>>' is not assignable to type 'Ref<T, S>'.
Type 'DocumentType<NonNullable<T>>' is not assignable to type 'DocumentType<T, BeAnObject>'.
Type 'DocumentType<NonNullable<T>>' is not assignable to type 'Document<unknown, BeAnObject, T, DefaultIdVirtual, {}>'.
21 ): docs is mongoose.Types.Array<DocumentType<NonNullable<T>>>;(this one at least can be solved by removing the NonNullable<>, though i dont know why it wouldnt be compatible)
test/tests/ref.test.ts:93:3 - error TS2322: Type 'Document<unknown, BeAnObject, RefTestString, DefaultIdVirtual, {}> & Omit<RefTestString & Required<{ _id: string; }> & { ...; }, "id" | "typegooseName"> & DefaultIdVirtual & IObjectWithTypegooseFunction' is not assignable to type 'Ref<RefTestStringOptional> | undefined'.
Type 'Document<unknown, BeAnObject, RefTestString, DefaultIdVirtual, {}> & Omit<RefTestString & Required<{ _id: string; }> & { ...; }, "id" | "typegooseName"> & DefaultIdVirtual & IObjectWithTypegooseFunction' is not assignable to type 'DocumentType<RefTestStringOptional, BeAnObject>'.
Type 'Document<unknown, BeAnObject, RefTestString, DefaultIdVirtual, {}> & Omit<RefTestString & Required<{ _id: string; }> & { ...; }, "id" | "typegooseName"> & DefaultIdVirtual & IObjectWithTypegooseFunction' is not assignable to type 'Document<unknown, BeAnObject, RefTestStringOptional, DefaultIdVirtual, {}>'.
The types returned by 'deleteOne(...)' are incompatible between these types.
Type 'QueryWithHelpers<DeleteResult, Document<unknown, BeAnObject, RefTestString, DefaultIdVirtual, {}> & Omit<RefTestString & Required<...> & { ...; }, "id" | "typegooseName"> & DefaultIdVirtual & IObjectWithTypegooseFunction, BeAnObject, RefTestString, "deleteOne", Record<...>>' is not assignable to type 'QueryWithHelpers<DeleteResult, Document<unknown, BeAnObject, RefTestStringOptional, DefaultIdVirtual, {}>, BeAnObject, RefTestStringOptional, "deleteOne", Record<...>>'.
Type 'QueryWithHelpers<DeleteResult, Document<unknown, BeAnObject, RefTestString, DefaultIdVirtual, {}> & Omit<RefTestString & Required<...> & { ...; }, "id" | "typegooseName"> & DefaultIdVirtual & IObjectWithTypegooseFunction, BeAnObject, RefTestString, "deleteOne", Record<...>>' is not assignable to type 'Query<DeleteResult, Document<unknown, BeAnObject, RefTestStringOptional, DefaultIdVirtual, {}>, BeAnObject, RefTestStringOptional, "deleteOne", Record<...>>'.
The types returned by '$where(...)' are incompatible between these types.
Type 'QueryWithHelpers<(Document<unknown, BeAnObject, RefTestString, DefaultIdVirtual, {}> & Omit<RefTestString & Required<{ _id: string; }> & { ...; }, "id" | "typegooseName"> & DefaultIdVirtual & IObjectWithTypegooseFunction)[], ... 4 more ..., Record<...>>' is not assignable to type 'QueryWithHelpers<Document<unknown, BeAnObject, RefTestStringOptional, DefaultIdVirtual, {}>[], Document<unknown, BeAnObject, RefTestStringOptional, DefaultIdVirtual, {}>, BeAnObject, RefTestStringOptional, "deleteOne", Record<...>>'.
Type 'QueryWithHelpers<(Document<unknown, BeAnObject, RefTestString, DefaultIdVirtual, {}> & Omit<RefTestString & Required<{ _id: string; }> & { ...; }, "id" | "typegooseName"> & DefaultIdVirtual & IObjectWithTypegooseFunction)[], ... 4 more ..., Record<...>>' is not assignable to type 'Query<Document<unknown, BeAnObject, RefTestStringOptional, DefaultIdVirtual, {}>[], Document<unknown, BeAnObject, RefTestStringOptional, DefaultIdVirtual, {}>, BeAnObject, RefTestStringOptional, "deleteOne", Record<...>>'.
Types of property 'and' are incompatible.
Type '(array: FilterQuery<RefTestString>[]) => QueryWithHelpers<(Document<unknown, BeAnObject, RefTestString, DefaultIdVirtual, {}> & Omit<...> & DefaultIdVirtual & IObjectWithTypegooseFunction)[], ... 4 more ..., Record<...>>' is not assignable to type '(array: FilterQuery<RefTestStringOptional>[]) => Query<Document<unknown, BeAnObject, RefTestStringOptional, DefaultIdVirtual, {}>[], ... 4 more ..., Record<...>>'.
Types of parameters 'array' and 'array' are incompatible.
Type 'FilterQuery<RefTestStringOptional>[]' is not assignable to type 'FilterQuery<RefTestString>[]'.
Type 'FilterQuery<RefTestStringOptional>' is not assignable to type 'FilterQuery<RefTestString>'.
Type '{ _id?: Condition<ApplyBasicQueryCasting<StringQueryTypeCasting | undefined>>; } & RootFilterOperators<{ _id?: ApplyBasicQueryCasting<StringQueryTypeCasting | undefined>; }>' is not assignable to type 'FilterQuery<RefTestString>'.
Type '{ _id?: Condition<ApplyBasicQueryCasting<StringQueryTypeCasting | undefined>>; } & RootFilterOperators<{ _id?: ApplyBasicQueryCasting<StringQueryTypeCasting | undefined>; }>' is missing the following properties from type 'Query<any, any, {}, unknown, "find", Record<string, never>>': _mongooseOptions, exec, all, allowDiskUse, and 93 more.
93 refTypeTest.refFieldStringOptional = new RefTestStringModel();
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~Note that this error seemingly only happens to classes which have _id?: Type (or _id: Type | undefined). (which is i think used to make it optional for defaulting)
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.
Pull Request Overview
This PR consolidates type definitions by replacing Mongoose's custom Condition, RootQuerySelector, and QuerySelector types with equivalent types from the MongoDB Node driver to reduce duplication and ensure consistency.
- Removes Mongoose-specific type imports and replaces them with MongoDB driver types
- Updates type annotations to use
mongodb.Condition<T>instead of Mongoose'sCondition<T> - Simplifies type definitions by leveraging the MongoDB driver's existing type system
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/types/queries.test.ts | Removes Condition and QuerySelector imports, adds mongodb import, updates type annotations to use mongodb.Condition<T> |
| test/types/populate.test.ts | Simplifies function parameter types and updates toObject generic type usage |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@hasezoey can you please take another look and see if you still get the same error with typegoose? |
|
I checked and you're right, removing the NonNullable<> in the following fixes typegoose tests. export function isDocumentArray<T, S extends RefType>(
docs: mongoose.Types.Array<Ref<T, S>> | null | undefined
): docs is mongoose.Types.Array<DocumentType<NonNullable<T>>>;I'll take a look as to why. |
|
@hasezoey can you please take another look? Typegoose tests seem to pass on latest
I also ended up stripping out a bunch of unnecessary generics to shave off some instantiations for performance. Will need to make some changelog notes for that. |
hasezoey
left a comment
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.
Looks good to me now in its current state. Though the generics removal should arguably be a separate PR.
| filter: QueryFilter<TSchema>; | ||
| /** A document or pipeline containing update operators. */ | ||
| update: UpdateQuery<TSchema>; | ||
| /** A set of filters specifying to which array elements an update should apply. */ | ||
| arrayFilters?: AnyObject[]; | ||
| /** Specifies a collation. */ | ||
| collation?: mongodb.CollationOptions; | ||
| /** The index to use. If specified, then the query system will only consider plans using the hinted index. */ | ||
| hint?: mongodb.Hint; | ||
| /** When true, creates a new document if no document matches the query. */ | ||
| upsert?: boolean; | ||
| /** When false, do not add timestamps. When true, overrides the `timestamps` option set in the `bulkWrite` options. */ | ||
| timestamps?: boolean; | ||
| } | ||
|
|
||
| export interface DeleteOneModel<TSchema = AnyObject> { | ||
| /** The filter to limit the deleted documents. */ | ||
| filter: QueryFilter<TSchema>; | ||
| /** Specifies a collation. */ | ||
| collation?: mongodb.CollationOptions; | ||
| /** The index to use. If specified, then the query system will only consider plans using the hinted index. */ | ||
| hint?: mongodb.Hint; | ||
| } | ||
|
|
||
| export interface DeleteManyModel<TSchema = AnyObject> { | ||
| /** The filter to limit the deleted documents. */ | ||
| filter: QueryFilter<TSchema>; | ||
| /** Specifies a collation. */ |
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.
Why remove the ability for updateOne to specify / overwrite the result type?
Edit: in case github still shows this later, somehow in the PR-thread interface it show the completely wrong lines, but shows it correctly in the PR-files interface.
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.
Better tsc performance due to fewer instantiations is the primary driver.
I also don't see updateOne<ResultType> being especially useful because updateOne() always returns UpdateOneResult, the generic parameter doesn't update the ResultType virtual. Also, updateOne not having a generic is more consistent with deleteOne, which similarly always returns DeleteOneResult.

Summary
QuerySelector,RootQuerySelector, andConditionare almost exactly duplicates of types the MongoDB Node driver exports. With a little work, we can make it so that Mongoose uses the MongoDB driver's types rather than maintaining its own (which may be out of date).Examples