types: enhanced Document's methods #13739
Conversation
There was a problem hiding this comment.
This PR has some neat ideas, but also has several issues that prevent us from merging. Overall suggestions:
- Get rid of
FlatPath<>entirely. Make theT extends FlatPath<DocType>generics intoT extends keyof DocType. That will mean we at least have some of the syntactic benefits for top-level paths. - Bring back fallback syntaxes for passing in arbitrary strings.
types/utility.d.ts
Outdated
| @@ -40,4 +40,34 @@ type IfEquals<T, U, Y = true, N = false> = | |||
| (<G>() => G extends T ? 1 : 0) extends | |||
| (<G>() => G extends U ? 1 : 0) ? Y : N; | |||
|
|
|||
| type IsAny<T> = unknown extends T ? ([keyof T] extends [never] ? false : true) : false; | |||
There was a problem hiding this comment.
We already have IfAny<>, so this is unnecessary
types/document.d.ts
Outdated
| validate(options: { pathsToSkip?: pathsToSkip }): Promise<void>; | ||
|
|
||
| /** Executes registered validation rules (skipping asynchronous validators) for this document. */ | ||
| validateSync(options: { pathsToSkip?: pathsToSkip, [k: string]: any }): Error.ValidationError | null; | ||
| validateSync(pathsToValidate?: pathsToValidate, options?: AnyObject): Error.ValidationError | null; | ||
| validateSync<T extends FlatPath<DocType>>(pathsToValidate?: T | T[], options?: AnyObject): Error.ValidationError | null; |
There was a problem hiding this comment.
I'm a bit wary of using FlatPath<> by default because of potential infinite recursion issues. Any self-referencing objects will lead to FlatPath<> blowing up, which is one of the reasons why the MongoDB Node driver no longer uses their Join helper by default.
types/document.d.ts
Outdated
| @@ -207,7 +209,7 @@ declare module 'mongoose' { | |||
| populated(path: string): any; | |||
|
|
|||
| /** Sends a replaceOne command with this document `_id` as the query selector. */ | |||
| replaceOne(replacement?: AnyObject, options?: QueryOptions | null): Query<any, this>; | |||
| replaceOne(replacement?: Partial<Omit<DocType, '_id'>>, options?: QueryOptions | null): Query<any, this>; | |||
There was a problem hiding this comment.
This is incorrect. 1) AnyObject is more correct, because Mongoose will run casting to handle any types that don't match, 2) Partial<> is incorrect because required fields are still required, 3) omitting _id is incorrect: you can specify _id as long as the replacement has the same _id as the document, but there's no way to know whether the _id is the same at compile time.
types/document.d.ts
Outdated
| @@ -191,7 +193,7 @@ declare module 'mongoose' { | |||
| * for immutable properties. Behaves similarly to `set()`, except for it | |||
| * unsets all properties that aren't in `obj`. | |||
| */ | |||
| overwrite(obj: AnyObject): this; | |||
| overwrite(obj: Partial<Omit<DocType, '_id'>>): this; | |||
There was a problem hiding this comment.
Similar here. AnyObject is more correct, Partial<> is wrong, and omitting _id is wrong.
types/document.d.ts
Outdated
|
|
||
| /** Checks if `path` was explicitly selected. If no projection, always returns true. */ | ||
| isDirectSelected(path: string): boolean; | ||
| isDirectSelected<T extends FlatPath<DocType>>(path: T): boolean; |
There was a problem hiding this comment.
This syntax is nice, but would be good to keep the isDirectSelected(path: string): boolean; syntax as a fallback because it is perfectly valid to call isDirectSelected() on a field that's not in the schema.
|
You have some good points. Changes made as instructed. Please review. Thanks |
ref: #13738
Summary
Improved various Document's methods