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

fix: allow calling Model.aggregate() with options #10604

Merged
merged 2 commits into from Aug 25, 2021

Conversation

amitbeck
Copy link
Contributor

Summary

I want Mongoose to support passing options to Model.aggregate() and to not have to call Aggregate.option() manually in a separate operation.

Examples

Until now:

// error
Model.aggregate([ /* pipeline */], { allowDiskUse: true }).exec() // Callback must be a function, got [object Object]

// workaround, requires 2 separate operations including storing the `Aggregate` instance as variable
const aggregate = Model.aggregate([ /* pipeline */]);
await aggregate.option({ allowDiskUse: true }).exec();

After this PR:

Model.aggregate([ /* pipeline */], { allowDiskUse: true }).exec() // works

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.

Quick comment re: TS, but otherwise LGTM

index.d.ts Outdated
aggregate<R = any>(pipeline?: any[]): Aggregate<Array<R>>;
aggregate<R = any>(pipeline: any[], cb: Function): Promise<Array<R>>;
aggregate<R = any>(pipeline?: any[], options?: Record<string, unknown>): Aggregate<Array<R>>;
aggregate<R = any>(pipeline: any[], options?: Record<string, unknown>, cb: Function): Promise<Array<R>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure this will cause a TS error because a mandatory param can't follow an optional param. We'd need to make options required here, and add a separate overload for aggregate(pipeline, cb)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow I can't believe I actually didn't notice this lol

@amitbeck
Copy link
Contributor Author

@vkarpov15 thanks for the fix, funny that I missed this.

@vkarpov15 vkarpov15 added this to the 6.0.1 milestone Aug 25, 2021
@vkarpov15 vkarpov15 merged commit f8e9f91 into Automattic:master Aug 25, 2021
@amitbeck amitbeck deleted the Model_aggregate_options branch August 25, 2021 18:30
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.

None yet

2 participants