Skip to content

streams: refactor ReadableStream asyncIterator creation and a few fixes#23042

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.

Sign up for GitHub

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

Jump to bottom
Merged
devsnek merged 1 commit into from
Oct 9, 2018
Merged

streams: refactor ReadableStream asyncIterator creation and a few fixes #23042

devsnek merged 1 commit into from
Oct 9, 2018

Conversation

@devsnek
Copy link
Member

@devsnekdevsnek commented Sep 23, 2018

Closes: #23041

@nodejs/streams

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 23, 2018

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/986/pipeline

@nodejs-github-botnodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 23, 2018
@devsnekdevsnek added the experimental Issues and PRs related to experimental features. label Sep 23, 2018
@targos
Copy link
Member

targos commented Sep 23, 2018

Can you explain in the commit message the reason for the refactor and what the fixes are?

@devsnekdevsnekforce-pushed the refactor/readable-stream-async-iterator branch from a0f83a8 to ae8b7f8CompareSeptember 23, 2018 20:25
@devsnek
Copy link
MemberAuthor

devsnek commented Sep 23, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/17393/

mcollina
mcollina reviewed Sep 23, 2018
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work!

Can you add a test that shows that #23041 is fixed? I know it is, but the test would be more readable that way.

@devsnek
Copy link
MemberAuthor

devsnek commented Sep 23, 2018

@mcollina the lines i added to the test file are that

@mcollina
Copy link
Member

mcollina commented Sep 23, 2018

@devsnek I specifically mean:

This might be useful if you wanted to consume the first X chunks of a stream using await iterator.next() and then conditionally consume the rest with for await of.

This seems a valid use case to test anyway.

jasnell
jasnell approved these changes Sep 23, 2018
View reviewed changes
Copy link
Member

@jasnelljasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with @mcollina's nit addressed

@devsnekdevsnekforce-pushed the refactor/readable-stream-async-iterator branch from ae8b7f8 to 49eedb7CompareSeptember 24, 2018 00:02
@devsnek
Copy link
MemberAuthor

devsnek commented Sep 24, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/17396/

mcollina
mcollina approved these changes Sep 24, 2018
View reviewed changes
Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 24, 2018
@joyeecheungjoyeecheung mentioned this pull request Sep 25, 2018
Closed
@danbev
Copy link
Contributor

danbev commented Oct 4, 2018

Re-run of failing node-test-commit-custom-suites-freestyle.

@devsnek
Copy link
MemberAuthor

devsnek commented Oct 7, 2018

CI https://ci.nodejs.org/job/node-test-pull-request/17675/

@devsnek
streams: refactor ReadableStream asyncIterator creation and a few fixes
8bce9e8
Closes: nodejs#23041 - Rewrite `ReadableAsyncIterator` class into `ReadableStreamAsyncIteratorPrototype` which contains no constructor and inherits from `%AsyncIteratorPrototype%`. - Rewrite `AsyncIteratorRecord` into dumb function. PR-URL: nodejs#23042Fixes: nodejs#23041 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@devsnekdevsnekforce-pushed the refactor/readable-stream-async-iterator branch from 49eedb7 to 8bce9e8CompareOctober 9, 2018 03:18
@devsnek
Copy link
MemberAuthor

devsnek commented Oct 9, 2018

landed in 8bce9e8

@devsnekdevsnek merged commit 8bce9e8 into nodejs:masterOct 9, 2018
@devsnekdevsnek deleted the refactor/readable-stream-async-iterator branch October 9, 2018 03:20
targos pushed a commit that referenced this pull request Oct 10, 2018
@devsnek@targos
streams: refactor ReadableStream asyncIterator creation and a few fixes
2b77b94
Closes: #23041 - Rewrite `ReadableAsyncIterator` class into `ReadableStreamAsyncIteratorPrototype` which contains no constructor and inherits from `%AsyncIteratorPrototype%`. - Rewrite `AsyncIteratorRecord` into dumb function. PR-URL: #23042Fixes: #23041 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@targostargos mentioned this pull request Oct 10, 2018
Merged
This was referenced Oct 10, 2018
Closed
Closed
Closed
This was referenced Oct 11, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
jasnell pushed a commit that referenced this pull request Oct 17, 2018
@devsnek@jasnell
streams: refactor ReadableStream asyncIterator creation and a few fixes
7084ed9
Closes: #23041 - Rewrite `ReadableAsyncIterator` class into `ReadableStreamAsyncIteratorPrototype` which contains no constructor and inherits from `%AsyncIteratorPrototype%`. - Rewrite `AsyncIteratorRecord` into dumb function. PR-URL: #23042Fixes: #23041 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
@rektiderektide mentioned this pull request Mar 6, 2019
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@jasnelljasnelljasnell approved these changes

@BridgeARBridgeARBridgeAR approved these changes

+1 more reviewer

@apapirovskiapapirovskiapapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.experimentalIssues and PRs related to experimental features.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@devsnek@nodejs-github-bot@targos@mcollina@danbev@apapirovski@jasnell@BridgeAR