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

stream: add bytesRead property for readable #4372

Closed
wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

Add a bytesRead property for readable is
useful in some use cases.

When user want know how many bytes read of
readable, need to caculate it in userland.
If encoding is specificed, get the value is
very slowly.

@mscdex mscdex added stream Issues and PRs related to the stream subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 21, 2015
@trevnorris
Copy link
Contributor

I'm cool w/ the change, but there a test that can be added? Also should probably be accompanied by a docs addition.

@JacksonTian
Copy link
Contributor Author

Thanks, @trevnorris , I'd like to add test and docs.

@JacksonTian JacksonTian force-pushed the bytes_read branch 3 times, most recently from 05727c1 to d559dd7 Compare December 31, 2015 09:44
@JacksonTian
Copy link
Contributor Author

Add test cases and docs.


var Readable = require('stream').Readable;
var Duplex = require('stream').Duplex;
var Transform = require('stream').Transform;
Copy link
Contributor

Choose a reason for hiding this comment

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

Preempting the request from others to make these const

EDIT: and anywhere else that the variable doesn't change.

@trevnorris
Copy link
Contributor

Few comments but LGTM otherwise.

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

Add a bytesRead property for readable is
useful in some use cases.

When user want know how many bytes read of
readable, need to caculate it in userland.
If encoding is specificed, get the value is
very slowly.
@JacksonTian
Copy link
Contributor Author

Thanks @trevnorris , I updated by your suggestion.

@trevnorris
Copy link
Contributor

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Jan 4, 2016

LGTM

@ronkorving
Copy link
Contributor

Really looking forward to this 👍

@cjihrig
Copy link
Contributor

cjihrig commented Jan 14, 2016

LGTM. Getting a fresh CI run: https://ci.nodejs.org/job/node-test-pull-request/1234/

@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

Failures in CI look unrelated

jasnell pushed a commit that referenced this pull request Jan 15, 2016
Add a bytesRead property for readable is
useful in some use cases.

When user want know how many bytes read of
readable, need to caculate it in userland.
If encoding is specificed, get the value is
very slowly.

PR-URL: #4372
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

Landed in bfb2cd0

@jasnell jasnell closed this Jan 15, 2016
@JacksonTian JacksonTian deleted the bytes_read branch January 15, 2016 08:38
@@ -135,6 +137,7 @@ function readableAddChunk(stream, state, chunk, encoding, addToFront) {
var e = new Error('stream.unshift() after end event');
stream.emit('error', e);
} else {
stream.bytesRead += state.objectMode ? 0 : chunk.length;
Copy link
Member

Choose a reason for hiding this comment

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

Are you really sure this tracks bytes and not characters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an edge case, when encoding of this.push(string) is same as encoding readable, the string was just pass though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a bit of a problem then? :(

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to revert this?

Copy link
Member

Choose a reason for hiding this comment

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

I think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR coming

cjihrig added a commit to cjihrig/node that referenced this pull request Jan 18, 2016
cjihrig added a commit that referenced this pull request Jan 18, 2016
This reverts commit bfb2cd0.

The bytesRead property, as implemented, tracks characters
instead of bytes when using an identity encoding.

Refs: #4372
PR-URL: #4746
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell jasnell mentioned this pull request Mar 17, 2016
@Fishrock123 Fishrock123 added dont-land-on-v5.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 1, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Add a bytesRead property for readable is
useful in some use cases.

When user want know how many bytes read of
readable, need to caculate it in userland.
If encoding is specificed, get the value is
very slowly.

PR-URL: nodejs#4372
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
This reverts commit bfb2cd0.

The bytesRead property, as implemented, tracks characters
instead of bytes when using an identity encoding.

Refs: nodejs#4372
PR-URL: nodejs#4746
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants