FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

zlib: split JS code as prep for non-zlib-backed streams by addaleax · Pull Request #24939 · nodejs/node · GitHub

/ node Public

zlib: split JS code as prep for non-zlib-backed streams - #24939

Closed
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:zlib-split
ConversationCommits (1)ChecksFiles changed
Closed

zlib: split JS code as prep for non-zlib-backed streams#24939
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:zlib-split

Conversation

addaleax commented Dec 10, 2018

Copy link
Copy Markdown
Member

Split out from #24938

Split the Zlib class into ZlibBase and Zlib classes,
to facilitate introduction of similar streams with minor
implementation differences.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

zlib: split JS code as prep for non-zlib-backed streams
b3d6008 
Split the `Zlib` class into `ZlibBase` and `Zlib` classes,
to facilitate introduction of similar streams with minor
implementation differences.

nodejs-github-bot commented Dec 10, 2018

Copy link
Copy Markdown
Collaborator

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1885/pipeline

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Dec 10, 2018
bnoordhuis approved these changes Dec 10, 2018
View reviewed changes
Comment thread lib/zlib.js
this._finishFlushFlag = finishFlush;
this._nextFlush = -1;
this._info = opts && opts.info;
this._defaultFullFlushFlag = fullFlush;

bnoordhuis Dec 10, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is fine, it mirrors the other flush flag fields (triple alliteration bonus!), I just wonder when it would be anything but Z_FULL_FLUSH? Is it different for brotli?

addaleax Dec 11, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, brotli uses a different set of constants that don’t map 1:1 to zlib’s ones (BROTLI_OPERATION_FLUSH would be the one in this case)

Comment thread lib/zlib.js
const handle = new binding.Zlib(mode);
// Ideally, we could let ZlibBase() set up _writeState. I haven't been able
// to come up with a good solution that doesn't break our internal API,
// and with it all supported npm versions at the time of writing.

bnoordhuis Dec 10, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is this because of npm doing something dumb or naughty?

addaleax Dec 11, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

npm uses https://www.npmjs.com/package/minizlib, which until very recently provided an API similar to our own zlib API using process.binding('zlib'), so they effectively locked us into an API contract for the native binding. I’ve had a PR to (at least to some degree) address this merged & released last week, so it’s probably just a matter of slowly waiting for already-released npm versions to become outdated now…

bnoordhuis Dec 11, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

First released in 2017?! You'd think Isaac learned from the graceful-fs debacle two years before but I guess I'm a hopeless optimist.

Well okay, let's give npm one more pass. Don't want to punish the community for the actions of an individual.

jasnell Dec 11, 2018

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I have a very difficult time being sympathetic to minizlib here. Our policy around process.binding has been very clear. If we break that module making necessary changes to the binding then so be it

addaleax Dec 11, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@jasnell Yes, it’s unfortunate that we can’t do a bit more cleanup here yet, but I don’t think there are any necessary changes to be made here. Plus, whether we like it or not, I don’t think breaking every version of npm that is in use is something we could realistically do.

danbev commented Dec 13, 2018 •
edited by Trott
Loading

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-pull-request/19472/ ✔️

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 15, 2018

Trott commented Dec 16, 2018

Copy link
Copy Markdown
Member

Another review would be great here. @indutny @mscdex @joyeecheung @jasnell

addaleax commented Dec 18, 2018

Copy link
Copy Markdown
Member Author

Landed in 3b9e0f2

addaleax closed this Dec 18, 2018
addaleax added a commit that referenced this pull request Dec 18, 2018
zlib: split JS code as prep for non-zlib-backed streams
3b9e0f2 
Split the `Zlib` class into `ZlibBase` and `Zlib` classes,
to facilitate introduction of similar streams with minor
implementation differences.

PR-URL: #24939
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax deleted the zlib-split branch December 18, 2018 15:41

MylesBorins commented Dec 25, 2018

Copy link
Copy Markdown
Contributor

This does not land cleanly on v11.x, should it be backported?

addaleax added a commit to addaleax/node that referenced this pull request Dec 26, 2018
zlib: split JS code as prep for non-zlib-backed streams
696e9bd 
Split the `Zlib` class into `ZlibBase` and `Zlib` classes,
to facilitate introduction of similar streams with minor
implementation differences.

PR-URL: nodejs#24939
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax mentioned this pull request Dec 26, 2018
Closed
2 tasks

addaleax commented Dec 26, 2018

Copy link
Copy Markdown
Member Author

Backport in #25228

targos pushed a commit that referenced this pull request Jan 1, 2019
zlib: split JS code as prep for non-zlib-backed streams
e534dcd 
Split the `Zlib` class into `ZlibBase` and `Zlib` classes,
to facilitate introduction of similar streams with minor
implementation differences.

Backport-PR-URL: #25228
PR-URL: #24939
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
zlib: split JS code as prep for non-zlib-backed streams
ab52a06 
Split the `Zlib` class into `ZlibBase` and `Zlib` classes,
to facilitate introduction of similar streams with minor
implementation differences.

PR-URL: nodejs#24939
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BridgeAR mentioned this pull request Jan 16, 2019
Merged
MylesBorins mentioned this pull request Jan 24, 2019
Merged
addaleax added a commit that referenced this pull request May 13, 2019
zlib: split JS code as prep for non-zlib-backed streams
ebe51a4 
Split the `Zlib` class into `ZlibBase` and `Zlib` classes,
to facilitate introduction of similar streams with minor
implementation differences.

Backport-PR-URL: #25228
PR-URL: #24939
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
zlib: split JS code as prep for non-zlib-backed streams
dd8d1da 
Split the `Zlib` class into `ZlibBase` and `Zlib` classes,
to facilitate introduction of similar streams with minor
implementation differences.

Backport-PR-URL: #25228
PR-URL: #24939
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BethGriggs mentioned this pull request May 16, 2019
Merged
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
beevelop mentioned this pull request May 29, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

bnoordhuis bnoordhuis approved these changes
jasnell jasnell left review comments
+1 more reviewer
Kronhyx Kronhyx approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. zlib Issues and PRs related to the zlib subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL