zlib: split JS code as prep for non-zlib-backed streams#24939
zlib: split JS code as prep for non-zlib-backed streams#24939addaleax wants to merge 1 commit intonodejs:masterfrom
Conversation
Split the `Zlib` class into `ZlibBase` and `Zlib` classes, to facilitate introduction of similar streams with minor implementation differences.
| this._finishFlushFlag = finishFlush; | ||
| this._nextFlush = -1; | ||
| this._info = opts && opts.info; | ||
| this._defaultFullFlushFlag = fullFlush; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
| 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. |
There was a problem hiding this comment.
Is this because of npm doing something dumb or naughty?
There was a problem hiding this comment.
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…
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
addaleax
added
the
author ready
|
Another review would be great here. @indutny @mscdex @joyeecheung @jasnell |
|
Landed in 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>
|
This does not land cleanly on v11.x, should it be backported? |
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>
|
Backport in #25228 |
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>
8 participants
Split out from #24938
Split the
Zlibclass intoZlibBaseandZlibclasses,to facilitate introduction of similar streams with minor
implementation differences.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes