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

BREAKING CHANGE: stop using mquery for updateX(), deleteX(), sort(), always convert sort args to objects #13777

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

vkarpov15
Copy link
Collaborator

Re: #13617

Summary

Unfortunately there's a lot of functionality duplicated between mongoose and mquery, so with #13617 we're moving towards moving this logic into mongoose for easier maintenance. The big change in this PR is sort(): we've moved sort handling into Mongoose, and we're making options.sort always an object. We're now converting .sort([['a', 1]]) into .sort({ a: 1 }), rather than leaving it as [['a', 1]] under the hood. Keeping entries-style syntax internally is not necessary post ES2015, key order is still maintained, and that removes the need for us to handle edge cases like .sort([['a', 1]]).sort({ b: 1 }).

Examples

…t()`, always convert sort args to objects

Re: #13617
Copy link
Collaborator

@AbdelrahmanHafez AbdelrahmanHafez left a comment

Choose a reason for hiding this comment

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

LGTM.
I would've expected some require('mquery') to be removed somewhere, along with its removal from package.json, though.

@hasezoey
Copy link
Collaborator

I would've expected some require('mquery') to be removed somewhere, along with its removal from package.json, though.

to my understanding this PR is only for update, delete and sort, but mquery is used in some more places, mainly the mquery.canMerge function and also Query.prototype is new mquery() (and Query.base)

@vkarpov15
Copy link
Collaborator Author

Yeah there are still a few places where we use mquery, most notably for chaining for geo operators. But this PR is mostly just to remove mquery from updateX(), deleteX() and sort(). Those are the most heavily used pieces of code where we still rely on mquery.

@vkarpov15 vkarpov15 merged commit f4f69e3 into 8.0 Aug 25, 2023
@vkarpov15 vkarpov15 deleted the vkarpov15/gh-13617 branch August 25, 2023 14:07
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.

3 participants