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.

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

Bug/types/update query #11588

Merged
merged 4 commits into from
Mar 27, 2022
Merged

Bug/types/update query #11588

merged 4 commits into from
Mar 27, 2022

Conversation

AbdelrahmanHafez
Copy link
Collaborator

Currently, we can do the following in JavaScript:

const userSchema = new Schema({ name: String });
const User = mongoose.model('User', userSchema);

await User.updateOne({}, { name: 'Sam' });

But TypeScript doesn't know that the second parameter to updateOne can take name as a top level property.
It would, however, understand the types if we use $set:

// provides inteliisense support
await User.updateOne({}, { $set: { name: 'Sam' } });

// does not provide intellisense support
await User.updateOne({}, { name: 'Sam' });

I managed to make it read the properties from the interface, but another bug seems to be that it converts all types to any instead of the type defined on the schema.

@AbdelrahmanHafez AbdelrahmanHafez added the typescript Types or Types-test related issue / Pull Request label Mar 27, 2022
@AbdelrahmanHafez AbdelrahmanHafez added this to the 6.2.11 milestone Mar 27, 2022
@@ -2058,7 +2058,7 @@ declare module 'mongoose' {
* { age: 30 }
* ```
*/
export type UpdateQuery<T> = _UpdateQuery<_UpdateQueryDef<T>> & AnyObject;
export type UpdateQuery<T> = Partial<T> & _UpdateQuery<_UpdateQueryDef<T>> & AnyObject;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the reason is the Union Type with AnyObject resulting in a Type Widening. I would usually try with Omit<AnyObject, keyof T & _UpdateQuery<_UpdateQueryDef>> instead of just using AnyObjecy.

Suggested change
export type UpdateQuery<T> = Partial<T> & _UpdateQuery<_UpdateQueryDef<T>> & AnyObject;
export type UpdateQuery<T> = Partial<T> & _UpdateQuery<_UpdateQueryDef<T>> & Omit<AnyObject, keyof T & _UpdateQuery<_UpdateQueryDef<T>>>;

Didn't test it, just wrote it with my mobile.

Also look into the populate typing PR. @mohammad0-0ahmad used a different notation to achieve the correct typings

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe the following, if update query should accept any type:
@vkarpov15 what do u think about that ?

export type UpdateQuery<T> = _UpdateQuery<_UpdateQueryDef<T>> & (T | AnyObject);

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mohammad0-0ahmad that approach works for me, as long as it is & (Partial<T> | AnyObject)

@AbdelrahmanHafez AbdelrahmanHafez modified the milestones: 6.2.11, 6.2.10 Mar 27, 2022
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks. Overall, I think this approach works and is a neat improvement for intellisense. I'll try out @mohammad0-0ahmad 's suggestion after merging.

@@ -2058,7 +2058,7 @@ declare module 'mongoose' {
* { age: 30 }
* ```
*/
export type UpdateQuery<T> = _UpdateQuery<_UpdateQueryDef<T>> & AnyObject;
export type UpdateQuery<T> = Partial<T> & _UpdateQuery<_UpdateQueryDef<T>> & AnyObject;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mohammad0-0ahmad that approach works for me, as long as it is & (Partial<T> | AnyObject)

@vkarpov15 vkarpov15 modified the milestones: 6.2.10, 6.2.9 Mar 27, 2022
@vkarpov15 vkarpov15 merged commit bfa65e9 into master Mar 27, 2022
@vkarpov15 vkarpov15 deleted the bug/types/update-query branch March 27, 2022 20:19
@vkarpov15 vkarpov15 restored the bug/types/update-query branch March 27, 2022 20:23
@vkarpov15
Copy link
Collaborator

Actually, I didn't notice that this is causing test failures, sorry. I'm going to have to revert and we'll have to consider how we can do this without breaking our tests.

@vkarpov15 vkarpov15 removed this from the 6.2.9 milestone Mar 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants