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: use readableObjectMode public api for js stream #27655

Closed
wants to merge 1 commit into from
Closed

stream: use readableObjectMode public api for js stream #27655

wants to merge 1 commit into from

Conversation

antsmartian
Copy link
Contributor

@antsmartian antsmartian commented May 12, 2019 •
edited
Loading

Added readableObjectMode and writableObjectMode to stream, so that we can remove instances like _readableState.* in code.

Refs: #445

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
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3488

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label May 12, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@ZYSzys
Copy link
Member

ZYSzys commented May 12, 2019

I think we need to add some tests for this.

@nodejs-github-bot
Copy link
Collaborator

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

@antsmartian
Copy link
Contributor Author

@mcollina PTAL.. Also, if getters/setters to be added in the prototype, I guess we need to document those as well.

@antsmartian antsmartian added the wip Issues and PRs that are still a work in progress. label May 12, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@antsmartian antsmartian force-pushed the 445-js-stream branch 2 times, most recently from 98fae1c to 334b049 Compare May 13, 2019 01:52
@antsmartian antsmartian removed the wip Issues and PRs that are still a work in progress. label May 13, 2019
@antsmartian
Copy link
Contributor Author

@nodejs/streams @jasnell PTAL.. I have added the test cases and as well the relevant documentation. cc @ZYSzys .. Thanks..

@nodejs-github-bot
Copy link
Collaborator

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

antsmartian
antsmartian commented May 13, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
lpinca
lpinca reviewed May 13, 2019
View reviewed changes
lib/stream.js Outdated Show resolved Hide resolved
lib/stream.js Outdated Show resolved Hide resolved
@lpinca
Copy link
Member

lpinca commented May 13, 2019

Did you benchmark this? getters used to add a noticeable overhead over direct property access.

mcollina
mcollina requested changes May 13, 2019
View reviewed changes
lib/stream.js Outdated Show resolved Hide resolved
@antsmartian
Copy link
Contributor Author

@mcollina @lpinca PTAL.. Addressed your comments.
@lpinca I haven't run any benchmarks for this change. Will run them and update here.

mcollina
mcollina approved these changes May 13, 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 a review from addaleax May 13, 2019 14:55
lpinca
lpinca reviewed May 13, 2019
View reviewed changes
lib/_stream_readable.js Outdated Show resolved Hide resolved
mcollina
mcollina requested changes May 13, 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.

Marking as request changes. Would you mind removing the setters?

addaleax
addaleax approved these changes May 13, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax 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 the setters removed

@nodejs-github-bot
Copy link
Collaborator

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

ZYSzys
ZYSzys reviewed May 13, 2019
View reviewed changes
lib/_stream_readable.js Outdated Show resolved Hide resolved
lib/_stream_writable.js Outdated Show resolved Hide resolved
@antsmartian
Copy link
Contributor Author

@ZYSzys @mcollina PTAL..

mcollina
mcollina approved these changes May 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

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member

Landed in b4735ec 🎉

@addaleax addaleax closed this May 19, 2019
addaleax pushed a commit that referenced this pull request May 19, 2019
@antsmartian @addaleax
stream: use readableObjectMode public api for js stream
b4735ec 
PR-URL: #27655
Refs: #445
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
targos pushed a commit that referenced this pull request May 20, 2019
@antsmartian @targos
stream: use readableObjectMode public api for js stream
32fd0ac 
PR-URL: #27655
Refs: #445
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell 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. 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.
7 participants
@antsmartian @nodejs-github-bot @ZYSzys @lpinca @addaleax @mcollina @jasnell