Skip to content
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

Merged
merged 6 commits into from Dec 31, 2019

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Dec 30, 2019

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 and readWriteStream, 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

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 30, 2019
@codecov
Copy link

codecov bot commented Dec 30, 2019

Codecov Report

Merging #843 into master will decrease coverage by 0.13%.
The diff coverage is 85.29%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
dev/src/external-modules.d.ts 0% <0%> (ø) ⬆️
dev/src/index.ts 98.78% <100%> (+0.04%) ⬆️
dev/src/watch.ts 98.45% <100%> (ø) ⬆️
dev/src/reference.ts 99.07% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b94c367...865981e. Read the comment docs.

@schmidt-sebastian schmidt-sebastian added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 30, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 30, 2019
@schmidt-sebastian
Copy link
Contributor Author

@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.

dev/src/index.ts Outdated Show resolved Hide resolved
* @returns A Promise with the resulting read-only stream.
*/
readStream(
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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',
Copy link
Member

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)

Copy link
Contributor Author

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) {
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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);
    });

Copy link
Contributor Author

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.

@schmidt-sebastian schmidt-sebastian merged commit 68795c4 into master Dec 31, 2019
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/removebun branch January 3, 2020 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streams do not provide the full Node.js stream API (Lack of Symbol.asyncIterator)
4 participants