-
Notifications
You must be signed in to change notification settings - Fork 148
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
feat: add Symbol.asyncInterator to Query.stream() #843
Conversation
fddd37c
to
2515259
Compare
2515259
to
5df7fea
Compare
Codecov Report
@@ Coverage Diff @@
## master #843 +/- ##
==========================================
- Coverage 88.83% 88.69% -0.14%
==========================================
Files 27 27
Lines 16769 16681 -88
Branches 1156 1151 -5
==========================================
- Hits 14896 14796 -100
- Misses 1868 1880 +12
Partials 5 5
Continue to review full report at Codecov.
|
@bcoe It looks like the coverage decrease in this PR is because of the license comment I had to add to external-modules.d.ts. |
* @returns A Promise with the resulting read-only stream. | ||
*/ | ||
readStream( |
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.
Is it a breaking change?
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.
This is not part of of our public API (we define our own types). I can use an underscore prefix here, but in general the client is pretty inconsistent here.
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.
OK. Since it's TypeScript, we might as well make those methods private
.
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.
Unfortunately, these are "module private" and used in other internal classes.
methodName: string, | ||
mode: 'unidirectional' | 'bidirectional', |
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.
if it's an externally visible function, I would vote for an enum (but up to you)
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.
This should be private.
await randomCol.doc().set({foo: 'bar'}); | ||
|
||
const stream = randomCol.stream(); | ||
for await (const chunk of stream) { |
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.
Does TypeScript compile it into something that older versions of Node.js can understand? Async iterators only appeared in Node 10, right?
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.
I am a little surprised too, but it seems to work. I stole this test from googleapis/nodejs-storage#799
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.
Typescript transpiles it away:
it('stream() supports readable[Symbol.asyncIterator]()', async () => {
var e_1, _a;
let received = 0;
await randomCol.doc().set({ foo: 'bar' });
await randomCol.doc().set({ foo: 'bar' });
const stream = randomCol.stream();
try {
for (var stream_1 = __asyncValues(stream), stream_1_1; stream_1_1 = await stream_1.next(), !stream_1_1.done;) {
const chunk = stream_1_1.value;
++received;
}
}
catch (e_1_1) { e_1 = { error: e_1_1 }; }
finally {
try {
if (stream_1_1 && !stream_1_1.done && (_a = stream_1.return)) await _a.call(stream_1);
}
finally { if (e_1) throw e_1.error; }
}
chai_1.expect(received).to.equal(2);
});
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.
Also verifies that the test does indeed work with Symbol.asyncIterator by changing the target level to ES2019.
This PR removes the dependency on
bun
, which exposed an old Stream interface that did not expose Symbol.asyncInterator.A lot of the code churn in this PR is combining
readStream
andreadWriteStream
, which were almost identical before (readStream sent request to the GAPIC method directly, whereas readWriteStream sent it to the stream). The new method handles all Stream callbacks uniformly and combines both stream modes.A disclaimer: I don't really understand Node streams, but we have semi-good test coverage for most error cases.
Fixes #789