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: allow arbitrary keys in query filters again #14874

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

vkarpov15
Copy link
Collaborator

Revert #14764
Fix #14863
Fix #14862

Summary

#14764 introduced issues for quite a few devs, see #14863. We should revert for now, and consider adding #14764 in 9.0.

cc @alex-statsig

Examples

@vkarpov15 vkarpov15 added this to the 8.6.2 milestone Sep 9, 2024
@alex-statsig
Copy link
Contributor

Sorry, sounds good to me! Waiting for 9.0 to make it a breaking change seems reasonable. Guess we could even expose a "typesafe" or "unsafe" version of each function to allow people to opt out, although unsafe is the same as just doing model.query(queryObj as any), so maybe thats a simpler workaround in 9.0

@vkarpov15
Copy link
Collaborator Author

@alex-statsig there's also another issue that popped up in #14842: Mongoose discriminators support specifying the discriminator key in the query filter, and Mongoose will infer the schema based on the discriminator key. For example:

const Person = new Schema({ name: String });
const Employee = Person.discriminator('Employee', new Schema({ title: String }));

// Because discriminator key `__t` is set, Mongoose is able to infer the type of the `title` property, even
// though the model is `Person` not `Employee`
await Person.findOne({ __t: 'Employee', title: 'CEO' });

Is there a way to support this with TypeScript? I imagine we would need some way of defining what discriminators are tied to a particular model in TS.

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, lets wait for 9.0 to re-consider introducing this strictness

@@ -216,7 +216,7 @@ function find() {
Project.find({ name: 'Hello' });

// just callback; this is no longer supported on .find()
expectError(Project.find((error: CallbackError, result: IProject[]) => console.log(error, result)));
Project.find((error: CallbackError, result: IProject[]) => console.log(error, result));
Copy link
Collaborator

Choose a reason for hiding this comment

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

after the revert, does the "expectError" now fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Functions match "object with any keys" in TypeScript, and telling TypeScript "this parameter can be any non-function object" is tricky, I haven't been able to figure out how to do that.

image

I would love to keep the "this parameter can't be a function" behavior, but not sure how that would work

@hasezoey hasezoey added the typescript Types or Types-test related issue / Pull Request label Sep 10, 2024
@vkarpov15 vkarpov15 merged commit e4e1d66 into master Sep 10, 2024
5 checks passed
@hasezoey hasezoey deleted the vkarpov15/revert-14764 branch November 1, 2024 15:14
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
3 participants