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

types(query): make FilterQuery props resolve to any for generics support #14510

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

vkarpov15
Copy link
Collaborator

Fix #14473
Re: #14459

Tagging @FaizBShah @sderrow re: #14436, #14397

Summary

It looks like the major reason for FilterQuery<T> setting all props to any is because TypeScript disallows setting an object with a concrete type to an abstract type. This includes generic function parameters, like calling a function with FilterQuery<> as a parameter where FilterQuery<> depends on the generic. For example:

function findById<ModelType extends {_id: Types.ObjectId | string}>(model: Model<ModelType>, _id: Types.ObjectId | string){
    // Will not compile without `as FilterQuery<ModelType>`, even if `FilterQuery<T> = T` because `FilterQuery`
    // depends on generic type `ModelType`
    return model.find({_id: _id});
}

From this SO answer: "Never assign a concrete type to a generic type parameter, consider it as read-only!". That includes calling a function with arguments that rely on the generic type parameter. This restriction makes sense because of never

type ModelType = { _id: never };
const TestModel = model<ModelType>('test', new Schema());

// Compiles fine, apparently `{ _id: never } extends { _id: Types.ObjectId | string }`
findById<ModelType>(TestModel, 'foo');

This whole "generic function for wrapping Mongoose model functions" pattern isn't one that I would personally use, but it seems to be sufficiently common that we've gotten 3 distinct bug reports related to this change #14459, #14473, #14462

Examples

@FaizBShah
Copy link
Contributor

@vkarpov15 It seems that its a very common pattern which is followed by a lot of devs, so in that case I don't think there's much to be done in this case other than making FilterQuery<> handle casting to any type. I'll try to think if there is some way we can improve the typing of this type without compromising the pattern in future, but for now this PR looks good to me. LGTM!!

Copy link
Collaborator

@hasezoey hasezoey left a 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, though asking because i dont fully understand this back-and-forth of the FilterQuery type (i have only skimmed the issues / PRs):

the Condition type has been simplified and now does less checking / work to support more general cases because of typescript weirdness, while still trying to avoid the outcome of any?

@vkarpov15
Copy link
Collaborator Author

Less checking to allow defining generic functions like the following. Any type checking in FilterQuery for prop values causes the following to fail to compile.

function findById<ModelType extends {_id: Types.ObjectId | string}>(model: Model<ModelType>, _id: Types.ObjectId | string){
    return model.find({_id: _id});
}

@vkarpov15 vkarpov15 merged commit 893b9d6 into master Apr 10, 2024
5 checks passed
@vkarpov15 vkarpov15 added this to the 8.3.2 milestone Apr 10, 2024
@hasezoey hasezoey deleted the vkarpov15/gh-14473 branch April 11, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use FilterQuery<Type> when Type is a class
4 participants