-
Notifications
You must be signed in to change notification settings - Fork 1.8k
chore(NODE-4321): add typescript to eslint rules #3304
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -894,7 +894,7 @@ export class ChangeStream< | |||||
|
||||||
if (isResumableError(changeStreamError, this.cursor.maxWireVersion)) { | ||||||
this._endStream(); | ||||||
this.cursor.close(); | ||||||
this.cursor.close().catch(() => null); | ||||||
|
||||||
const topology = getTopology(this.parent); | ||||||
topology.selectServer(this.cursor.readPreference, {}, serverSelectionError => { | ||||||
|
@@ -913,6 +913,7 @@ export class ChangeStream< | |||||
* | ||||||
* we promisify _processErrorIteratorModeCallback until we have a promisifed version of selectServer. | ||||||
*/ | ||||||
// eslint-disable-next-line @typescript-eslint/unbound-method | ||||||
private _processErrorIteratorMode = promisify(this._processErrorIteratorModeCallback); | ||||||
|
||||||
/** @internal */ | ||||||
|
@@ -923,7 +924,7 @@ export class ChangeStream< | |||||
} | ||||||
|
||||||
if (isResumableError(changeStreamError, this.cursor.maxWireVersion)) { | ||||||
this.cursor.close(); | ||||||
this.cursor.close().catch(() => null); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, the docs for this rule state regarding prefixing the expression with
That's also something we have standardized on in our team's JS code, i.e. that would be:
Suggested change
This is purely stylistic, of course, so feel free to ignore this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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:
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
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, it does make sense to leave a |
||||||
|
||||||
const topology = getTopology(this.parent); | ||||||
topology.selectServer(this.cursor.readPreference, {}, serverSelectionError => { | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.