chore(NODE-4321): add typescript to eslint rules

[nbbeeken]

Description

What is changing?

Adds @typescript-eslint/recommended-requiring-type-checking rules which include enforcing promise returning functions are awaited.

Skips the following:

• @typescript-eslint/no-unsafe-member-access
• @typescript-eslint/no-unsafe-argument
• @typescript-eslint/no-unsafe-assignment
• @typescript-eslint/no-unsafe-return
• @typescript-eslint/no-unsafe-call
• @typescript-eslint/restrict-plus-operands
• @typescript-eslint/restrict-template-expressions

I'll file follow up tickets for us to address these.

There are outstanding TODOs that need discussion.

What is the motivation for this change?

We can use async await internally as long as we pass the result through our callback / maybePromise handling logic. We want this lint rule to help us catch any mistakes where we might forget to await a promise returning funciton.

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

[baileympearson]

It doesn't mention it in your ticket but should we enable these rules in BSON as well? If not in this ticket, we should probably file a follow-up.

[addaleax]

Btw, the docs for this rule state regarding prefixing the expression with void:

This can be a good way to explicitly mark a promise as intentionally not awaited.

That's also something we have standardized on in our team's JS code, i.e. that would be:

Suggested change

	this.cursor.close().catch(() => null);
	void this.cursor.close();

This is purely stylistic, of course, so feel free to ignore this.

[nbbeeken]

Appreciate the suggestion! IIUC wouldn't leaving the promise without a rejection handler risk exiting the process if it did error? it's spec-ed behavior to ignore errors from close (@baileympearson do you have the reference?)

[baileympearson]

I don't know if it spec-ed. We had a discussion about this on the PR already but I'll post this again with more detail.

Relevant ticket: jira.mongodb.org/browse/NODE-4217
Related PR: #3226

I encountered this issue for cursor.close and filed the aforementioned ticket and left the TODOs that you see in the above PR. When we triaged/refined it, we decided that the current behavior was correct and closed the ticket as won't do (at least, I thought the jira status should be won't do but I guess we kinda ruined that flow by adding AC to the ticket related to removing TODOs. Maybe that's a process improvement we could make for the future). You can see Marin's AC on the ticket is to remove the TODOs I added, which I did in the PR. We don't have any real record of this but I do remember Jeff mentioning that this would be the intended behavior. My understanding was that the justification was two-fold:

1. What exactly would the user do in the case that cursor.close failed? Try again? If killCursors fails, there isn't much they can do because it probably means something larger is going wrong.
2. Cursors timeout on the server anyways, so if the cursor fails to close eventually it will be GC'd.

I think we could revisit this though because I think an argument could be made for cursor.close() propagating errors.

---

Now this particular instance is a separate issue, irrespective whether or not cursor.close() swallows errors. This call to cursor.close() is inside the error handler for ChangeStreams. This code is run if there is an error with the change stream. The desired (but not explicitly spec'd behavior that I know of) would be

1. Error occurs in the CS (resumable or not)
2. We attempt to resume. This involves cleaning up the previous cursor and restarting the stream
3. Resuming, or failing to resume and propagating the original error to the user

What we don't want is the error from cursor.close() either crashing the user's process (unhandled process rejection) or overridding the original change stream error. The fact that our CS uses cursors internally and that they're closed during the resume process is an implementation detail and not something that a user cares about. So in this scenario, irrespective of whether or not cursor.close() swallows errors, we should swallow the error.

[addaleax]

IIUC wouldn't leaving the promise without a rejection handler risk exiting the process if it did error?

Right, it does make sense to leave a .catch(() => {}) in places where you intentionally ignore errors 👍