Revert "zlib: improve performance"

[Trott]

This reverts commit add4b0a.

@MylesBorins

Checklist

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

Affected core subsystem(s)

zlib

[Trott]

Fixes: #14161

[Trott]

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

[mscdex]

Can't we just fix minizlib instead (which shouldn't be accessing internals anyway)?

[addaleax]

@mscdex At least @iarna said that’s what most likely to happen, but it certainly hasn’t happened yet (and I didn’t have the time to look into a proper fix for all of this either), and Node 9 is moving closer … this revert doesn’t have to be permanent.

[Trott]

Can't we just fix minizlib instead (which shouldn't be accessing internals anyway)?

@mscdex I'll be happy if this ends up being unnecessary and we can close it without landing.

[mcollina]

LGTM but we shouldn’t need this.

[mcollina]

can you address #15625 in a separate commit that is easily backportable?

[addaleax]

Actually, @iarna just wrote a comment on isaacs/node-tar#144 which reminded me that that PR exists and also solves the problem here, for node-tar & npm.

[iarna]

I'll see if I can't wrangle the node-tar patch into a npm release this week.

[refack]

Can't we just fix minizlib instead (which shouldn't be accessing internals anyway)?

that’s what most likely to happen, but it certainly hasn’t happened yet

I'll see if I can't wrangle the node-tar patch into a npm release this week.

Can we have a deadline for either this or a compatible npm release? Suggesting Mon 10/23, if node9+npm is still broke, let's land this?

[jasnell]

I'm generally -1 on this but I'm not going to give it the big red X or block it from moving forward. As others have said, I'd prefer the userland fix instead.

[Fishrock123]

I don't think we should do this. We made a stance on this long enough ago and my understanding is minizlib violated those expectations somewhat recently, and is authored by someone who very well knows our expectations on these things.

Edit: in extension, if npm or subdependencies use that module, it is their fault if things break.

[zkat]

heads-up: the next release of npm is going out tomorrow, and will include a new version of node-tar which should be compatible.

If y'all wanna try the patch out in the meantime, you can use $ npx npmc install to try it out in the npm canary.

[targos]

I think the latest npm (5.5.1) still has version 4.0.1 of node-tar.

[addaleax]

@zkat Nice, this seems to work. 🎉

[targos]

@zkat do you know when that release of npm will be available?
@jasnell is preparing the release of Node 9.0.0 and it would be really nice to have it in (we are currently stuck with 5.3.0 on master)

[jasnell]

I'm going to try to make the final mass update to 9.0.0 today and focus solely on targeted updates the rest of the week. If the npm update gets in on time, then someone please put it on the 9.0.0 milestone so it can be tracked.

…

On Oct 23, 2017 02:31, "Michaël Zasso" ***@***.***> wrote: @zkat <https://github.com/zkat> do you know when that release of npm will be available? @jasnell <https://github.com/jasnell> is preparing the release of Node 9.0.0 and it would be really nice to have it in (we are currently stuck with 5.3.0 on master) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16280 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eUuvEmtfMDRaMnlESAb7FjDKkd94ks5svFzWgaJpZM4P9Jz6> .