refactor(NODE-6201): cursor to use fetchBatch when current batch is empty #4093
Conversation
nbbeeken
force-pushed
the
refactor-cursor-nextBatch
branch
2 times, most recently
from
f39ebac
to
ff12d3d
Compare
| if ( | ||
| options?.error == null || | ||
| options?.error?.name === 'MongoExpiredSessionError' || | ||
| options?.force | ||
| ) { |
There was a problem hiding this comment.
Without this change, endSession is sensitive to the exact order of endSession calls that were made by our cursor before this refactor.
There was a problem hiding this comment.
Can you elaborate, and clarify why the ordering of endSessions calls changed?
nbbeeken
force-pushed
the
refactor-cursor-nextBatch
branch
2 times, most recently
from
29beee7
to
092ee7b
Compare
nbbeeken
force-pushed
the
refactor-cursor-nextBatch
branch
from
092ee7b
to
8614df3
Compare
W-A-James
added
the
Primary Review
nbbeeken
changed the title
W-A-James
added
Team Review
| if ( | ||
| options?.error == null || | ||
| options?.error?.name === 'MongoExpiredSessionError' || | ||
| options?.force | ||
| ) { |
There was a problem hiding this comment.
Can you elaborate, and clarify why the ordering of endSessions calls changed?
src/cursor/abstract_cursor.ts
Outdated
| // @ts-expect-error: not sure.. | ||
| this.emit('close'); |
There was a problem hiding this comment.
If we're going to leave a ts-expect-error in, can we document why it's an error? but tbh I'd expect this to work, not sure why an expect error is needed here.
There was a problem hiding this comment.
Yes sorry meant to get back to this is is declared in the same way ConnectionEvents are so I'm not sure what is going on here.
Argument of type '[]' is not assignable to parameter of type 'Parameters<CursorEvents["close"]>'.ts(2345)Maybe it is because CursorEvents is generic so Parameters<CursorEvents["close"]> may not be assignable to []. Not sure how to require extenders do not add parameters.
nbbeeken
changed the title
Description
What is changing?
nextinto cursor and rename to fetchBatchIs there new documentation needed for these changes?
No
What is the motivation for this change?
The
nextfloating function owned too many details of the implementation of each cursor API, tryNext and next now do a small amount more lifting and we have a fetchBatch method that is focused on obtaining the next batch either with the init command or with a getMore.Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript