fix(NODE-3454): projection types are too narrow #2924
Conversation
dariakp
added
the
Primary Review
There was a problem hiding this comment.
It looks like there are two unused vars failing the build:
[2021/07/30 20:43:13.893] /data/mci/935b7979091d2b96139cddbecd5ed5ae/src/src/mongo_types.ts
[2021/07/30 20:43:13.893] 173:24 warning 'TSchema' is defined but never used @typescript-eslint/no-unused-vars
[2021/07/30 20:43:13.893] /data/mci/935b7979091d2b96139cddbecd5ed5ae/src/src/operations/find.ts
[2021/07/30 20:43:13.893] 21:30 warning 'TSchema' is defined but never used @typescript-eslint/no-unused-vars
| .find({ _id: { $in: [] } }) | ||
| .sort({ _id: -1 }) | ||
| .limit(3) | ||
| .project<PublicMeme>(publicMemeProjection) // <== location of TS2345 error |
There was a problem hiding this comment.
Copy pasta-ed from the ticket its the type error that was occuring with the projection constraints, I put in the actual message for clarity.
There was a problem hiding this comment.
I think including the ticket number would make sense, to provide context in the future about which TS2345 error this is referring to.
| @@ -74,13 +74,13 @@ expectNotType<{ age: number }[]>(await typedCollection.find().project({ name: 1 | |||
| expectType<{ notExistingField: unknown }[]>( | |||
| await typedCollection.find().project({ notExistingField: 1 }).toArray() | |||
| ); | |||
| expectNotType<TypedDoc[]>(await typedCollection.find().project({ notExistingField: 1 }).toArray()); | |||
| expectType<TypedDoc[]>(await typedCollection.find().project({ notExistingField: 1 }).toArray()); | |||
There was a problem hiding this comment.
I thought this was supposed to yield Document[], which shouldn't match TypedDoc[]?
There was a problem hiding this comment.
Corrected the test, I think this is correct, unless we want to change the return to always be generic?
There was a problem hiding this comment.
This is part of the larger issue that we discovered, we'll tackle it in NODE-3468
| likes: number; | ||
| someRandomProp: boolean; // Projection makes no enforcement on anything | ||
| // the convenience parameter project<X>() allows you to define a return type, | ||
| // otherwise projections returns a generic Document |
There was a problem hiding this comment.
I don't see a test to show an expectType<Document[]> here
| /** Sets the limit of documents returned in the query. */ | ||
| limit?: number; | ||
| /** Set to sort the documents coming back from the query. Array of indexes, `[['a', 1]]` etc. */ | ||
| sort?: Sort; | ||
| /** The fields to return in the query. Object of fields to either include or exclude (one of, not both), `{'a':1, 'b': 1}` **or** `{'a': 0, 'b': 0}` */ | ||
| projection?: Projection<TSchema>; | ||
| projection?: Document; |
There was a problem hiding this comment.
Wouldn't setting this to TSchema make the default functionality the same but still allow a more granular level of control without breaking things? And remove the need for the eslint disable
There was a problem hiding this comment.
You mean setting projection?: TSchema? I think that would cause issue with the typical projection of inclusion / exclusion like { _id: 0, name: 1 }. where name: string in the TSchema
nbbeeken
force-pushed
the
NODE-3454/projection-types
branch
from
b84b782
to
d8b55b4
Compare
dariakp
added
Team Review
src/mongo_types.ts
Outdated
| /** | ||
| * @public | ||
| * Projection is flexible to permit the wide array of aggregation operators | ||
| * @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further |
There was a problem hiding this comment.
| * @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further | |
| * @deprecated since v4.1.0: Since projections support all aggregation operations we have no plans to narrow this type further |
src/mongo_types.ts
Outdated
| Partial<Record<string, ProjectionOperators | 0 | 1 | boolean>>; | ||
| /** | ||
| * @public | ||
| * @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further |
There was a problem hiding this comment.
| * @deprecated since v4.1.0: Since projections support all of aggregation operations we have no plans to narrow this type further | |
| * @deprecated since v4.1.0: Since projections support all aggregation operations we have no plans to narrow this type further |
| .find({ _id: { $in: [] } }) | ||
| .sort({ _id: -1 }) | ||
| .limit(3) | ||
| .project<PublicMeme>(publicMemeProjection) // <== location of TS2345 error |
There was a problem hiding this comment.
I think including the ticket number would make sense, to provide context in the future about which TS2345 error this is referring to.
With the introduction of aggregation operations in 4.4 in projection documents typing projections is very complex. I propose here we remove projection type constrains altogether to permit the many operators permitted by the server.