-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: reintroduce clone and rewind for cursors #2647
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
Conversation
This reintroduces support for rewinding a cursor to its uninitialized state. NODE-2811
91e2ece
to
df0b77e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, mod one typo
src/cursor/find_cursor.ts
Outdated
@@ -49,6 +49,16 @@ export class FindCursor extends AbstractCursor { | |||
} | |||
} | |||
|
|||
/** | |||
* Returns a new unitialized copy of this cursor, with options matching those that have been set on the current instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Returns a new unitialized copy of this cursor, with options matching those that have been set on the current instance | |
* Returns a new uninitialized copy of this cursor, with options matching those that have been set on the current instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed here and in aggregation_cursor too!
df0b77e
to
59c5de2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM one suggestion
If we do want the abstract clone I think it's still available to us:
59c5de2
to
7bbc569
Compare
Description
These two commits add back support for cloning and rewinding cursors.
What changed?
The changes should be fairly straightforward, but I wanted to call out a few design decisions:
- I had originally intendedclone
to be an abstract method onAbstractCursor
, forcing us to implement it for all possible cursor subclasses. This would have unfortunately required us to makeAbstractCursor
generic over the subclass types, since the following signature results in type ambiguity at the point of implementation in the subclass:abstract clone(): this
Therefore,
clone
is implemented by hand inAbstractCursor
subclasses. I only implemented it forFindCursor
andAggregateCursor
because it's likely that's where legacy users have actually used this feature.There is a correction in the
rewind
implementation regarding sessions. When a session is started as part of cursor initialization I had originally marked this as anexplicit: true
start, however, this is clearly an implicit session the driver is creating in the absence of an explicitly provided one.Are there any files to ignore?
none