types(NODE-3729): add id type to TSchema return types

[ImRodry]

Description

What is changing?

This PR simply adds the _id field whenever TSchema/Document is returned as a type by using the existing WithId type, as explained in the linked issue

Is there new documentation needed for these changes?

No, because this was already the expected behavior and nothing changed code-wise

What is the motivation for this change?

While working on a personal project of mine, I had to delete the _id property from the findOne result object in order to compare it to a similarly generated object, and noticed I was getting a TS error saying the property _id didn't exist on the returned type. This PR aims to fix that so that the _id type is always present regardless of whether the Collection type has it or not. Jira issue: https://jira.mongodb.org/browse/NODE-3729

Double check the following

• Ran npm run check:lint script
• Self-review completed using the steps outlined here
• PR title follows the correct format: <type>(NODE-xxxx)<!>: <description>
• Changes are covered by tests
• New TODOs have a related JIRA ticket

⚠️ Note: I was not able to run the test scripts as I kept getting an error related to node-gyp when using npm i, npm ci, or yarn, so if someone else could run them I'd highly appreciate it! (Alternatively, a CI could be added to run that)

[ImRodry]

Found more types that need to be changed, will commit in a bit

[ImRodry]

Think that's all, please let me know if I missed any/if I changed some that I shouldn't have

[dariakp]

@ImRodry Hi there, thank you for reaching out.

Unfortunately, we had an issue with our CI builds that blocked all our tests from running and necessitated an update to main - if you could, please rebase your branch onto the new main to ensure the CI is able to run on your PR.

As far as your actual suggested changes - you do bring up a good point that something like a findOne will by default return the document with an _id field in addition to the fields specified in the collection type. However, we must be careful when applying this logic to aggregation pipelines, because the result of an aggregation operation almost never has the shape of the source document. I think it is a good idea to add WithId to our defaults for find operations where we specify TSchema as the default return type, but we need to make sure that we do not impose this on aggregation results, which need to accept a separate type definition wherever they are expected.

[ImRodry]

Absolutely! Will rebase in a second

as for the aggregation pipelines, I’m gonna be honest: I’m not too familiar with those so it’s possible I made some mistakes. Please review the lines you think should be reverted/changed so that I can apply those changes

[dariakp]

@ImRodry Thanks! We'll take a closer look soon.

With find specifically, if you do something like collection.find({}, { projection: { _id: 0 } }), you will get all documents without the _id property; and, in general, you can use the projection option to include or exclude fields from the return result, which is why we allow an optional override type on find operations, so that you are able to specify the return type you expect given your specific set of options.

I mentioned aggregation because I see you also modified the aggregation cursor file: the AggregationCursor is used by the aggregate api in the driver and operates on a "pipeline" of transformation stages, where the end result depends on what transformations you specify (e.g., you could sum all values of a particular field in matching documents, and return a document with a single "total" field) - you can read more here, if curious.

[ImRodry]

Thanks for the projection tip! I was actually not aware of that and it highly helps my code.
What I did in the AggregationCursor file was mostly by looking at it and trying to understand what types it was expecting, so I'll be awaiting the full review once you have time for that :D
Also slight note: the CI is still failing and there is no CI to run the test script, is that intentional? I still cannot install node_modules locally after the rebase so I'm not sure what to do

[ImRodry]

I somehow managed to run the test script and it's giving me errors on two files: src/change_stream.ts:714:64 and src/change_stream:339:43 that I am not sure how to fix. If you wish I can simply revert the changes to change_stream and leave the others that I am more familiar with.
I also noticed an error in your test script in build:ts as it's missing the keyword node after the &&

[nbbeeken]

Hi @ImRodry thanks again for helping out with our TS. I've got some comments for you here but I'm running into a wall with some of the details.

One of my comments is on the change we need to make to the find's that take a type override, they'll end up looking something like this:

findOne(): Promise<WithId<TSchema> | null>;
findOne<T = TSchema>(): Promise<T | null>;

But then you run into an issue where the return types of the overrides are incompatible. I'm not sure how we reconcile this, and still provide the type override feature.

[nbbeeken]

The purpose of these overrides is to allow users to specify an exact return type (to account for projections that modify the shape of documents returned) So we can't enforce WithId on the result type here.

[ImRodry]

I can create a new overload that accounts for projection with no _id, but I believe the other ones should stay.
Would be something like this

  findOne<T = TSchema>(filter: Filter<TSchema>, options?: FindOptions & { projection: { _id: 0 } }): Promise<T | null>;

[ImRodry]

This overload actually creates way more issues as it doesn't match with the general function declaration, can we go without it?

[nbbeeken]

What type issue is requiring the cursor class to be modified? I think nothing here should have to change.
I feel like where ever we create cursors, the WithId modifier would pass through the class's parameterization. Using FindCursor<WithId<TSchema>> or FindCursor<TSchema> would make all the APIs the cursor has return the correct type, I believe?

[ImRodry]

This is used in some functions like on line 235. Reverting the change yields this error:

[nbbeeken]

We'll need to consider more testing. If you look in test/types/community/collection/findX.test-d.ts there's some example schemas making use of the find APIs. And test/types/community/cursor.test-d.ts has cursor API tests.

[nbbeeken]

I also noticed an error in your test script in build:ts as it's missing the keyword node after the &&

It makes use of the relative path to the installed version of tsc our driver depends on ./node_modules/typescript/bin/tsc. (due to this issue: tsdjs/tsd#122)

It should be marked as executable so no need to proceed it node. 🤔 Maybe there's some difference in environment? Windows OS perhaps?

[ImRodry]

It should be marked as executable so no need to proceed it node. 🤔 Maybe there's some difference in environment? Windows OS perhaps?

Well yeah I am on windows and had to add the node keyword before that path. It executed just fine after this however, maybe it should be added to work on all OS's

[nbbeeken]

The specific use case surrounding our findOne and find functions has been covered by #3039

@ImRodry this may help your use case, is there further changes still needed to get the other APIs working as you would expect?

[ImRodry]

I only touched the other APIs because I felt the PR would be incomplete without doing so, but I have personally never messed with those. I will see if that PR fixes my issue and if it doesn’t, I’ll open a new one. For now, I’ll close this. Thanks!