-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
🐛fix: improve inferred Schema Type #12007
Conversation
- Correctly detect types for Schema[] and Schema in typeKey - Use correct ObjectId type Fixes #12002
The default value for an array field is an empty array unless specifed to be undefined. This checks if the path explicitly sets the default property to `undefined`
Does this pr fixes #12016 too? |
@imranbarbhuiya... I didn't get the issue in the |
To the maintainer helping me run the workflows, If the mongoose/types/inferschematype.d.ts Line 153 in 79c9ce4
{} and ObjectConstructor types also caused issues, I was able to solve them but ultimately decided to remove their checks as well and let those three types resolve to unknown
It's meant to match nested paths that aren't I don't know how you feel about that, but since |
I've created PR in your fork repo, check the changes and merge it if that will solve all mentioned issues. |
Yeah @mohammad0-0ahmad. I'm on it. I've been thinking what would be allowed? Should the default type match the typeKey's type or any valid Because if the default and typeKey's types are different, does that mean it's a mixed path? But in the case of Dates where you may default it to |
I think we can make it match the TypeKey's type for now. |
types/inferschematype.d.ts
Outdated
@@ -75,7 +75,7 @@ type IsPathRequired<P, TypeKey extends TypeKeyBaseType> = | |||
? P extends { default: undefined } | |||
? false | |||
: true | |||
: P extends PathWithTypePropertyBaseType<TypeKey> | |||
: P extends (Record<TypeKey, NumberSchemaDefinition | StringSchemaDefinition | BooleanSchemaDefinition | DateSchemaDefinition>) |
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 assumed not all of typeKey's types should be allowed to be remove the optional flag on that path.
So I set it to these 4. Because I think they're the only ones that should be able to provide defaults.
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.
So if you don't want the types to be limited, I'll revert the whole commit. @mohammad0-0ahmad
PathValueType extends DateConstructor | 'date' | 'Date' | typeof Schema.Types.Date ? Date : | ||
PathValueType extends StringSchemaDefinition ? PathEnumOrString<Options['enum']> : | ||
PathValueType extends NumberSchemaDefinition ? number : | ||
PathValueType extends DateSchemaDefinition ? Date : |
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.
These changes are just because I noticed they're defined in their respective SchemaDefinition types.
Sorry @iammola, I can't contributing for now, I will check changes when I can as soon as possible, please consider checking TS benchmark while refactoring types to avoid slower performance. |
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.
This is great, thanks!
@iammola sorry for being late, But I would like to say well done : ) |
Thank you, man... I really appreciate it. I honestly didn't do much, but I'll accept it 🙏 |
Fixes #12002
Fixes #12011
Fixed #12016
Summary
It uses the correct
Type.ObjectId
when inferringObjectId
fields (fixes the issue) and correctly infers the type for sub-documents defined with thetypeKey
and all sub-document arrays. Previously, theResolvePathType
helper type didn't match any Schema types in its pyramid of type checks. The only array check looped the sameResolvePathType
, so it never matched anySchema
in arrays and it goes deeper in arrays for deeper nested Schemas.I'm not sure how the other change will be received, but currently, any array that isn't explicitly marked with
required: true
gets resolved to be optional. I understand why, but the default value for an array schema type is an empty one unless the user explicitly sets it toundefined
.So I think it'll be better if array paths that don't have their defaults set to
undefined
aren't marked as optional types. Mongoose already allows document fields to be skipped when creating them with a method likeModel.create
. So if in the result of thefind*
queries, the arrays are marked as possibly undefined. It could cause unnecessary errors with TS.Example