fix(NODE-3442): AsyncIterator has incorrect return type #2916
Conversation
| return executeLegacyOperation(topology, operation, [{}, null], options).then(null, err => { | ||
| expect(err).to.equal(expectedError); |
There was a problem hiding this comment.
This was failing on the first commit I pushed, might have been flaky and I coulda tried a rerun but changing this to not rely on a setTimeout seems like a quick cleanup win.
There was a problem hiding this comment.
Cleaning up flaky tests is always good in my book! 👍
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| const { expect } = require('chai'); | |||
| const Sinon = require('sinon'); | |||
| const { Promise: BluebirdPromise } = require('bluebird'); | |||
There was a problem hiding this comment.
I brought in bluebird because our current test just did class P extends Promise and I needed something that wouldn't extend the native promise.
There was a problem hiding this comment.
SGTM, I think it's a good idea to test bluebird anyway, since it's the most likely alternative promise library people would use.
src/cursor/abstract_cursor.ts
Outdated
| if (value !== null && value !== undefined) return { value, done: false }; | ||
| return { value: undefined, done: true }; |
There was a problem hiding this comment.
Can totally ternary this, thoughts?
There was a problem hiding this comment.
The ternary was nice and succinct, do you prefer it this way?
There was a problem hiding this comment.
I spread it out for debugging but just now thought I can clean it up, changed it, looks v neat now :)
src/cursor/abstract_cursor.ts
Outdated
| if (value !== null && value !== undefined) return { value, done: false }; | ||
| return { value: undefined, done: true }; |
There was a problem hiding this comment.
The ternary was nice and succinct, do you prefer it this way?
| @@ -2,6 +2,7 @@ | |||
|
|
|||
| const { expect } = require('chai'); | |||
| const Sinon = require('sinon'); | |||
| const { Promise: BluebirdPromise } = require('bluebird'); | |||
There was a problem hiding this comment.
SGTM, I think it's a good idea to test bluebird anyway, since it's the most likely alternative promise library people would use.
| return executeLegacyOperation(topology, operation, [{}, null], options).then(null, err => { | ||
| expect(err).to.equal(expectedError); |
There was a problem hiding this comment.
Cleaning up flaky tests is always good in my book! 👍
|
Hey @adityapatadia that sounds like a pretty bad bug, could you open a ticket on our JIRA describing the steps to reproduce? |
|
Apologies for wrong flag. When we use newly released v7.0 of snappy library, none of the queries work. I downgraded snappy and everything works fine. |
|
Funny you mention it! I identified a bug with the new Snappy lib just hours ago, for now the driver can only use snappy 6.x but we plan to add a fix to support either soon. Keep an eye on the release notes for the fix or follow this ticket: https://jira.mongodb.org/browse/NODE-3528 |
The AsyncIterator had TSchema or null as its return type the truth is that the for await loop ends when the next function returns null so you never get null in your loop.