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

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

Merged

Conversation

devsnek
Copy link
Member

@devsnek devsnek 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 nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 23, 2018
@devsnek devsnek 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?

@devsnek devsnek force-pushed the refactor/readable-stream-async-iterator branch from a0f83a8 to ae8b7f8 Compare September 23, 2018 20:25
@devsnek
Copy link
Member Author

devsnek commented Sep 23, 2018

Copy link
Member

@mcollina mcollina 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
Member Author

devsnek commented Sep 23, 2018

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

@mcollina
Copy link
Member

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

Copy link
Member

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

@devsnek devsnek force-pushed the refactor/readable-stream-async-iterator branch from ae8b7f8 to 49eedb7 Compare September 24, 2018 00:02
@devsnek
Copy link
Member Author

devsnek commented Sep 24, 2018

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 24, 2018
@danbev
Copy link
Contributor

danbev commented Oct 4, 2018

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

@devsnek
Copy link
Member Author

devsnek commented Oct 7, 2018

Closes: nodejs#23041

- Rewrite `ReadableAsyncIterator` class into
`ReadableStreamAsyncIteratorPrototype` which contains no constructor and
inherits from `%AsyncIteratorPrototype%`.

- Rewrite `AsyncIteratorRecord` into dumb function.

PR-URL: nodejs#23042
Fixes: nodejs#23041
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@devsnek devsnek force-pushed the refactor/readable-stream-async-iterator branch from 49eedb7 to 8bce9e8 Compare October 9, 2018 03:18
@devsnek
Copy link
Member Author

devsnek commented Oct 9, 2018

landed in 8bce9e8

@devsnek devsnek merged commit 8bce9e8 into nodejs:master Oct 9, 2018
@devsnek devsnek 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
Closes: #23041

- Rewrite `ReadableAsyncIterator` class into
`ReadableStreamAsyncIteratorPrototype` which contains no constructor and
inherits from `%AsyncIteratorPrototype%`.

- Rewrite `AsyncIteratorRecord` into dumb function.

PR-URL: #23042
Fixes: #23041
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Closes: #23041

- Rewrite `ReadableAsyncIterator` class into
`ReadableStreamAsyncIteratorPrototype` which contains no constructor and
inherits from `%AsyncIteratorPrototype%`.

- Rewrite `AsyncIteratorRecord` into dumb function.

PR-URL: #23042
Fixes: #23041
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. experimental Issues and PRs related to experimental features. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants