zlib: move process.binding('zlib') to internalBinding

[addaleax]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

[nodejs-github-bot]

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

[addaleax]

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

[thefourtheye]

Isn't this a major change?

[cjihrig]

Yes, it's part of the larger #22160. I've added the label.

[addaleax]

@thefourtheye @cjihrig Can you explain why this is semver-major? We specifically have the whitelist in internal/bootstrap/node.js to avoid breakage, any this doesn’t introduce any new runtime or documentation deprecation.

[jasnell]

We have been treating these all as semver-major even with the whitelist, just to be extra careful.

[addaleax]

I get that we can decide to be careful if we can imagine a chance of breakage – but process.binding('zlib') is the only thing being changed here, and that keeps working and returns the same object.

I’m sorry, but labelling these PRs semver-major seems to have no effect to me except creating merge future conflicts.

If there are no firm objections, I’ll remove the label here and for the other PRs that added whitelisted modules. If you do have an objection, I’d really appreciate a reason or even just example code using process.binding() that would break.

[jasnell]

The process.binding method had been replaced entirely to enable the whitelist. It should be ok but we can't guarantee it. I'd prefer semver-major.

[danbev]

Travis CI is failing on a missing subsystem but this does not seem correct as the commit does have a subsystem specified.

[addaleax]

@Trott ^^^ ?

[Trott]

@Trott ^^^ ?

Travis does this:

$ git clone --depth=50 https://github.com/nodejs/node.git nodejs/node
$ cd nodejs/node
$ git fetch origin +refs/pull/23307/merge:
$ git checkout -qf FETCH_HEAD

For some reason, in this case but not in other pull requests as far as I have seen, that results in a merge commit only.

$ git log -2
commit 32b0d865929eff831bd6c00f7784ae74e06ebbca
Merge: eddfa2c52e 7765c16c72
Author: Anna Henningsen <github@addaleax.net>
Date:   Wed Oct 10 00:49:29 2018 +0000

    Merge 7765c16c72eeb9572b347357c5eb482030d69dd2 into eddfa2c52ee91e657c1a3e42815e6dff960a7fe3

commit eddfa2c52ee91e657c1a3e42815e6dff960a7fe3
Author: Rich Trott <rtrott@gmail.com>
Date:   Mon Sep 24 21:54:13 2018 -0700

    doc: simplify governance info in README intro
    
    Remove unnecessary wordiness in the governance sentence. It appears very
    early in the README so it should be short and clear. This also removes
    an instance of passive voice.
    
    PR-URL: https://github.com/nodejs/node/pull/23320
    Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
    Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
$

The merge commit is skipped for commit linting, and since there are no other commits in the PR, this ends up linting the very first commit ever added to Node.js which indeed does not have a subsystem.

$ git log 9d7895c567
commit 9d7895c567e8f38abfff35da1b6d6d6a0a06f9aa
Author: Ryan <ry@tinyclouds.org>
Date:   Mon Feb 16 01:02:00 2009 +0100

    add dependencies
$

If someone more knowledgable about git can explain why this PR results in a merge commit when others apparently do not, I'd be grateful. I'm guessing that the PR branch is missing some commits on master that edit the same files as this PR, so there's a merge that has to happen? But that's a guess. If so, I imagine a rebase would fix the issue for this PR. Will have to think about how to fix it more globally, though...

[Trott]

I think I can fix it in a robust way using TRAVIS_COMMIT environment variable or something similar. PR coming soon...

[Trott]

#23397 should fix that Travis issue. Feel free to head over to approve fast-tracking.

[Trott]

Travis issue (hopefully) fixed in a1edecc. A rebase here to get those changes should show a clean Travis lint run (although I suspect a rebase would have fixed it anyway).

[addaleax]

Landed in 09c42f4