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(index.d.ts): types for aggregation pipeline stages #10971

Merged
merged 5 commits into from
Dec 5, 2021
Merged

fix(index.d.ts): types for aggregation pipeline stages #10971

merged 5 commits into from
Dec 5, 2021

Conversation

jeremyben
Copy link
Contributor

@jeremyben jeremyben commented Nov 11, 2021

Summary

Using more and more aggregations in our projects, I decided to type accurately every pipeline stage according to the last mongodb documentation.

Now I'm proposing it for the mongoose package.

It does not narrow what cannot be narrowed (expressions for example, are still typed with any).
Only Pipeline stages are typed, pipeline operators types are not implemented, as this would require a lot of work.

Example

Following the first aggregation example in the MongoDB doc: https://docs.mongodb.com/manual/core/aggregation-pipeline/#complete-aggregation-pipeline-example

With a model Order that implements the following interface:

{ 
	_id: ObjectId, 
	productName: string, 
	status: 'new' | 'urgent',
	quantity: number
}

The following aggregate request is almost fully typed:

const urgentProductsByQuantity = await Order.aggregate<{ _id: string, sumQuantity: number }>([
	  { $match: { status: "urgent" } },
	  { $group: { _id: "$productName", sumQuantity: { $sum: "$quantity" } } } 
])

Comparing with the present aggregate signature any[], this is quiet an improvement IMHO.

@jeremyben jeremyben changed the title fix(index.d.ts): types for aggretation pipeline stages fix(index.d.ts): types for aggregation pipeline stages Nov 11, 2021
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.

Overall, I like this PR, but my biggest concern is that this represents a non-trival increase in TS' memory usage and # of instantiations. With the benchmark from #10349,

Before this PR:

Instantiations:             216148
Memory used:               174112K

After this PR:

Instantiations:             221961
Memory used:               180364K

Not terrible, but it also unfortunately undoes some of the gains we made in #10349. And, with TypeScript, the issue unfortunately comes down to the fact that the TS compiler is often too slow to handle accurate types, particularly with complex objects.

I'm leaning towards merging this but I'll take some more time to review.

aggregate<R = any>(pipeline?: any[], options?: Record<string, unknown>): Aggregate<Array<R>>;
aggregate<R = any>(pipeline: any[], cb: Function): Promise<Array<R>>;
aggregate<R = any>(pipeline: any[], options: Record<string, unknown>, cb: Function): Promise<Array<R>>;
aggregate<R = any>(pipeline?: PipelineStage<T>[], options?: mongodb.AggregateOptions, callback?: Callback<R[]>): Aggregate<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.

This change is incorrect, because we need to support the aggregate(pipeline, cb) syntax. Please add back aggregate<R = any>(pipeline: any[], cb: Function): Aggregate<Array<R>>;

Thanks for fixing the issue with returning Promise though, that is an issue in our type definitions.

Copy link
Contributor Author

@jeremyben jeremyben Nov 20, 2021

Choose a reason for hiding this comment

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

Will revert back the two params signature 👍 .

@jeremyben
Copy link
Contributor Author

jeremyben commented Nov 20, 2021

About performance, how about removing the generic T from PipelineStage<T>, as it only helps for autocompletion of only a few stages like $match ?
Or do you think that the problem is not about the generic but the whole single property object big union ?

@vkarpov15 vkarpov15 added this to the 6.1.0 milestone Dec 5, 2021
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.

Gonna merge this into the 6.1 branch and make the suggested changes: removing the generic from PipelineStage and revert to the correct signature.

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