refactor: pull CursorStream out of Cursor #2543
Conversation
| @@ -72,16 +72,17 @@ describe('Cursor Async Iterator Tests', function () { | |||
| } | |||
| }); | |||
|
|
|||
| it('should properly stop when cursor is closed', { | |||
| // TODO - figure out how to get this test working again | |||
There was a problem hiding this comment.
The for await ... of statement consumes the entire stream before entering the body of the loop, so this test fails with count larger than 1. I'm not sure if that means there's something incorrect with the implementation, or if this is a bad test.
There was a problem hiding this comment.
@mbroadst This test is still failing if un-skipped, any thoughts on what could be going wrong here?
There was a problem hiding this comment.
There are a couple of issues here:
-
We removed the Symbol.asyncIterator implementation for cursor because it already existed on
Readable, so that actually needs to be reimplemented here because we want people to be able to dofor await (const doc of cursor) { ... } -
If this is currently broken, that means that
CursorStreamis broken somehow. You might just have a silly bug here where you're creating a new stream on every iteration, in which case this fix should suffice:
const cursorStream = cursor.stream();
for await (const doc of cursorStream) { ... }
Otherwise, this would indicate that when you close the cursor it is not closing the stream which might be just fine! We assume that the stream might consume a bunch of document on that first iteration so this is probably desired behavior.
Let's fix the first problem, and revert the changes to the test here as your validation that the async iterator is implemented properly.
…adable-from-cursor
Co-authored-by: Matt Broadstone <mbroadst@mongodb.com>
| @@ -72,16 +72,17 @@ describe('Cursor Async Iterator Tests', function () { | |||
| } | |||
| }); | |||
|
|
|||
| it('should properly stop when cursor is closed', { | |||
| // TODO - figure out how to get this test working again | |||
There was a problem hiding this comment.
There are a couple of issues here:
-
We removed the Symbol.asyncIterator implementation for cursor because it already existed on
Readable, so that actually needs to be reimplemented here because we want people to be able to dofor await (const doc of cursor) { ... } -
If this is currently broken, that means that
CursorStreamis broken somehow. You might just have a silly bug here where you're creating a new stream on every iteration, in which case this fix should suffice:
const cursorStream = cursor.stream();
for await (const doc of cursorStream) { ... }
Otherwise, this would indicate that when you close the cursor it is not closing the stream which might be just fine! We assume that the stream might consume a bunch of document on that first iteration so this is probably desired behavior.
Let's fix the first problem, and revert the changes to the test here as your validation that the async iterator is implemented properly.
…adable-from-cursor
There was a problem hiding this comment.
@emadum This is really cool, a lot in here! LGTM!
One request: could you add a description of the fundamental changes here to this pull? With the intention of bringing those into the body of the commit message, just because this is such a big change it would be really nice to have a plain-english description of the changes here.
Cursoris no longer aReadablestream,.stream()must now be called to get a stream.NODE-2820
Description
Cursorused to extendReadable, doubling as both a readable stream and an iterable cursor. This was a likely source of confusion, because these usages couldn't be mixed on a given cursor, and the resulting errors were not particularly informative. Calling.stream()simplifies both the implementation and usage by making the separation between steaming and iterable cursors more explicit.What changed?
Are there any files to ignore?