Skip to content

stream: use readableObjectMode public api for js stream#27655

Closed
antsmartian wants to merge 1 commit into
nodejs:masterfrom
antsmartian:445-js-stream
Closed

stream: use readableObjectMode public api for js stream#27655
antsmartian wants to merge 1 commit into
nodejs:masterfrom
antsmartian:445-js-stream

Conversation

@antsmartian

@antsmartian antsmartian commented May 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot commented May 12, 2019

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

nodejs-github-bot commented May 12, 2019

Copy link
Copy Markdown
Collaborator

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

@ZYSzys

ZYSzys commented May 12, 2019

Copy link
Copy Markdown
Member

I think we need to add some tests for this.

@nodejs-github-bot

nodejs-github-bot commented May 12, 2019

Copy link
Copy Markdown
Collaborator

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

@antsmartian

antsmartian commented May 12, 2019

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

nodejs-github-bot commented May 12, 2019

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

antsmartian commented May 13, 2019

Copy link
Copy Markdown
Contributor Author

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

@nodejs-github-bot

nodejs-github-bot commented May 13, 2019

Copy link
Copy Markdown
Collaborator

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

antsmartian
antsmartian commented May 13, 2019
View reviewed changes
Comment thread doc/api/stream.md Outdated
lpinca
lpinca reviewed May 13, 2019
View reviewed changes
Comment thread lib/stream.js Outdated
Comment thread lib/stream.js Outdated
@lpinca

lpinca commented May 13, 2019

Copy link
Copy Markdown
Member

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
Comment thread lib/stream.js Outdated
@antsmartian

antsmartian commented May 13, 2019

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

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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
Comment thread lib/_stream_readable.js Outdated
mcollina
mcollina requested changes May 13, 2019
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented May 13, 2019

Copy link
Copy Markdown
Collaborator

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

ZYSzys
ZYSzys reviewed May 13, 2019
View reviewed changes
Comment thread lib/_stream_readable.js Outdated
Comment thread lib/_stream_writable.js Outdated
@antsmartian

antsmartian commented May 19, 2019

Copy link
Copy Markdown
Contributor Author

@ZYSzys @mcollina PTAL..

mcollina
mcollina approved these changes May 19, 2019
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented May 19, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 19, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 19, 2019

Copy link
Copy Markdown
Collaborator

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

@addaleax

addaleax commented May 19, 2019

Copy link
Copy Markdown
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@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

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

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