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.

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

stream: readableEnded #28814

Closed
wants to merge 4 commits into from
Closed

stream: readableEnded #28814

wants to merge 4 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 23, 2019 •
edited
Loading

Adds a readableEnded property and improved finished compat with possible streamlinke objects.

Refs: #28813

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jul 23, 2019
@ronag ronag changed the title Stream readable finished stream: readableEnded Jul 23, 2019
@ronag ronag force-pushed the stream-readableFinished branch 2 times, most recently from 9f98db0 to 45d181d Compare July 23, 2019 07:48
vsemozhetbyt
vsemozhetbyt reviewed Jul 23, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
mcollina
mcollina approved these changes Jul 23, 2019
View reviewed changes
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

mcollina
mcollina requested changes Jul 23, 2019
View reviewed changes
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.

Would you mind adding a unit test?

vsemozhetbyt
vsemozhetbyt reviewed Jul 23, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@ronag ronag force-pushed the stream-readableFinished branch 2 times, most recently from 82e0c54 to 34a2d13 Compare July 23, 2019 11:50
@ronag
Copy link
Member Author

ronag commented Jul 23, 2019

added test

lpinca
lpinca reviewed Jul 23, 2019
View reviewed changes
lib/_stream_readable.js Outdated Show resolved Hide resolved
mcollina
mcollina approved these changes Jul 24, 2019
View reviewed changes
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

@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina approved these changes Jul 26, 2019
View reviewed changes
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

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 26, 2019
@Trott
Copy link
Member

Trott commented Jul 26, 2019

Pinging @nodejs/streams out of caution.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 26, 2019
mcollina
mcollina requested changes Jul 26, 2019
View reviewed changes
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.

There is a nit in the doc

doc/api/stream.md Outdated Show resolved Hide resolved
@trivikr trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 26, 2019
@ronag @mcollina
Update doc/api/stream.md
a51944f 
Co-Authored-By: Matteo Collina <[email protected]>
@ronag
Copy link
Member Author

ronag commented Aug 2, 2019

@Trott what is this waiting for?

mcollina
mcollina approved these changes Aug 2, 2019
View reviewed changes
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

@ronag
Copy link
Member Author

ronag commented Aug 19, 2019

ping @Trott

@Trott
Copy link
Member

Trott commented Aug 19, 2019

@mcollina Added test looks OK to me. You?

mcollina
mcollina approved these changes Aug 19, 2019
View reviewed changes
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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 19, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Aug 20, 2019

Landed in 317fa3a

Trott pushed a commit that referenced this pull request Aug 20, 2019
@ronag @Trott
stream: add readableEnded
317fa3a 
Adds a readableEnded property and improved finished compat with possible
stream-like objects.

PR-URL: #28814
Refs: #28813
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott Trott closed this Aug 20, 2019
targos pushed a commit that referenced this pull request Aug 20, 2019
@ronag @targos
stream: add readableEnded
a9f8b62 
Adds a readableEnded property and improved finished compat with possible
stream-like objects.

PR-URL: #28814
Refs: #28813
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos added a commit that referenced this pull request Aug 20, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
43d6619 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    https://github.com/nodejs/node/pull/29186/files
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
@targos targos mentioned this pull request Aug 20, 2019
Merged
targos added a commit to targos/node that referenced this pull request Aug 20, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
a9adfdb 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    nodejs#28335
* deps:
  * Updated V8 to 7.6.303.29. nodejs#28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. nodejs#29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. nodejs#25925
    https://github.com/nodejs/node/pull/29186/files
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    nodejs#29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    nodejs#28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    nodejs#28934

PR-URL: nodejs#29210
targos added a commit that referenced this pull request Aug 20, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
e6872f5 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    #29186
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
targos added a commit that referenced this pull request Aug 20, 2019
@targos
2019-08-20, Version 12.9.0 (Current)
edc83a9 
Notable changes:

* crypto:
  * Added an oaepHash option to asymmetric encryption which allows
    users to specify a hash function when using OAEP padding.
    #28335
* deps:
  * Updated V8 to 7.6.303.29. #28955
    * Improves the performance of various APIs such as `JSON.parse` and
      methods called on frozen arrays.
    * Adds the Promise.allSettled method.
    * Improves support of `BigInt` in `Intl` methods.
    * For more information: https://v8.dev/blog/v8-release-76
  * Updated libuv to 1.31.0. #29070
    * `UV_FS_O_FILEMAP` has been added for faster access to memory
      mapped files on Windows.
    * `uv_fs_mkdir()` now returns `UV_EINVAL` for invalid filenames on
      Windows. It previously returned `UV_ENOENT`.
    * The `uv_fs_statfs()` API has been added.
    * The `uv_os_environ()` and `uv_os_free_environ()` APIs have been
      added.
* fs:
  * Added `fs.writev`, `fs.writevSync` and `filehandle.writev` (promise
    version) methods. They allow to write an array of `ArrayBufferView`s
    to a file descriptor. #25925
    #29186
* http:
  * Added three properties to `OutgoingMessage.prototype`:
    `writableObjectMode`, `writableLength` and `writableHighWaterMark`
    #29018
* stream:
  * Added an new property `readableEnded` to readable streams. Its value
    is set to `true` when the `'end'` event is emitted.
    #28814
  * Added an new property `writableEnded` to writable streams. Its value
    is set to `true` after `writable.end()` has been called.
    #28934

PR-URL: #29210
This was referenced Aug 20, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@ad-m ad-m mentioned this pull request Jan 29, 2020
Closed
@chudley chudley mentioned this pull request Mar 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@lpinca lpinca lpinca left review comments

@wa-Nadoo wa-Nadoo wa-Nadoo left review comments

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. stream Issues 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
@ronag @nodejs-github-bot @Trott @mcollina @wa-Nadoo @lpinca @vsemozhetbyt @trivikr