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

zlib: expose amount of data read for compression/decompression #12939

Closed
wants to merge 2 commits into from

Conversation

AlexanderOMara
Copy link
Contributor

@AlexanderOMara AlexanderOMara commented May 10, 2017

Added bytesRead property to Zlib engines and an option to expose the engine to
convenience method callbacks (so that the bytesRead value can be used in the convenience methods, per related issue).

Fixes: #8874

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
Affected core subsystem(s)
  • zlib

I can try to add documentation later, once I know the API does not need any changes.

Added bytesRead property to Zlib engines and an option to expose the engine to
convenience method callbacks.

Fixes: nodejs#8874
@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label May 10, 2017
@mscdex
Copy link
Contributor

mscdex commented May 10, 2017

It seems like this introduces two different changes, one that does what the PR title says, but there is also a (possible) change in the value passed to callbacks or returned from synchronous methods. I think it would be better to separate out those into separate PRs since the unrelated change would be semver-major IMHO.

@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 10, 2017
lib/zlib.js Outdated
if (engine._opts.info) {
return {
buffer: buffer,
engine: engine
Copy link
Member

Choose a reason for hiding this comment

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

You can just use return { buffer, engine }; here. :)

Copy link
Contributor Author

@AlexanderOMara AlexanderOMara May 15, 2017

Choose a reason for hiding this comment

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

Good point! Done.

@AlexanderOMara
Copy link
Contributor Author

AlexanderOMara commented May 18, 2017

@mscdex I've made an alternative set of pull requests that breaks the changes in two.

(I've closed this pull request in favor of those two different ones.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zlib.inflateRawSync how do I get the number of bytes read?
4 participants