doc: documentation deprecation of process.binding by jasnell · Pull Request #22004 · nodejs/node · GitHub

This is the first step in a long process of deprecating
process.binding() and replacing it with internalBinding().

Eventually, once we have replaced internal uses of
process.binding() with internalBinding(), we can escalate
to a runtime deprecation and eventual end-of-life.

/cc @nodejs/tsc @nodejs/security-wg

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

This is the first step in a long process of deprecating `process.binding()` and replacing it with `internalBinding()`. Eventually, once we have replaced internal uses of `process.binding()` with `internalBinding()`, we can escalate to a runtime deprecation and eventual end-of-life.

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

LGTM

It might be best to avoid the possessive by removing a couple words so it becomes: bridge between JavaScript and native code.

Even better perhaps is remove everything after code from this sentence. You don't have to explain what the function does. That's superfluous information in this case.

AFAIK, we use userland everywhere and user-land nowhere, so let's stick with userland.

Nit: remove strictly

Why?

It's unnecessary. It's also imprecise. "only" would be more precise.

So maybe this?:

process.binding() is intended for use by Node.js internal code only.

Putting all my nits together (and one or two that I didn't leave), the wording might be simplified to:

process.binding() is intended for use by Node.js internal code. Use in
userland code is unsupported.

...or (adding one word more):

process.binding() is intended for use by Node.js internal code only. Use in
userland code is unsupported.

We needed this for a long time.

Technically, it was never part of a documented public API afaik so we could just move to runtime deprecation, but given that the usage (of certain modules) was widespread, it seems to be a good decision to doc-deprecate it first.

@jasnell Perhaps DEP0103 text should be updated (in the «should be avoided» part) to mention this deprecation?

I'd be great to make some things, like process.binding('config'), accessible in a more user-friendly way. As we move thing along we should try to examine what folks have found handy (to cause them to reach for these internals) and try to fill the feature gap in a more polished way.

@jdalton can you please open an issue on how you use process.bindings(‘config’), and what would you need to have as a public API? Thanks!

LGTM with @Trott comments addressed

LGTM

CI-Lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/375/

Will land this on Monday if there are no objections by then.

by Node.js internal only code -> by Node.js internal code only or only by Node.js internal code.

fixed the word ordering

What about DEP0103?

Ah, right, yeah I'll update the description for that

CI Pipeline: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/391/

This is the first step in a long process of deprecating `process.binding()` and replacing it with `internalBinding()`. Eventually, once we have replaced internal uses of `process.binding()` with `internalBinding()`, we can escalate to a runtime deprecation and eventual end-of-life. PR-URL: #22004 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Landed in 182051b

@jasnell Is it supposed to include the deprecation codes as DEP00XX?
Are those assigned at the time of a release, or was that not deliberate?

The deprecation number should be assigned when the PR is landed.

Doh... Missed the step in landing. I used to have that in my local checks but I got rid of those when switching to node-core-util. Completely forgot about it. Will do a fixup pr

@targos @jasnell Apparently, that doesn't work :-/.

As the space of deprecation codes is pretty high, perhaps we could assign codes at PR open time instead and add a test for those? Would holes or out-of-order landing of deprecation codes hurt?

It would be helpful if node-core-util could handle it :)

Fixup here: #22062

@mcollina

jdalton can you please open an issue on how you use process.bindings(‘config’), and what would you need to have as a public API? Thanks!

Opened #22064

This is the first step in a long process of deprecating `process.binding()` and replacing it with `internalBinding()`. Eventually, once we have replaced internal uses of `process.binding()` with `internalBinding()`, we can escalate to a runtime deprecation and eventual end-of-life. PR-URL: #22004 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

nodejs-github-bot added deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. labels Jul 27, 2018

gireeshpunathil approved these changes Jul 27, 2018

View reviewed changes

mcollina approved these changes Jul 27, 2018

View reviewed changes

cjihrig approved these changes Jul 27, 2018

View reviewed changes

vdeturckheim approved these changes Jul 27, 2018

View reviewed changes

addaleax approved these changes Jul 27, 2018

View reviewed changes

bengl approved these changes Jul 27, 2018

View reviewed changes

vsemozhetbyt approved these changes Jul 27, 2018

View reviewed changes

addaleax added the process Issues and PRs related to the process subsystem. label Jul 27, 2018

sagirk approved these changes Jul 27, 2018

View reviewed changes

devsnek approved these changes Jul 27, 2018

View reviewed changes

Trott reviewed Jul 27, 2018

View reviewed changes

ChALkeR approved these changes Jul 27, 2018

View reviewed changes

BridgeAR approved these changes Jul 29, 2018

View reviewed changes

tniessen approved these changes Jul 29, 2018

View reviewed changes

targos approved these changes Jul 29, 2018

View reviewed changes

mhdawson approved these changes Jul 29, 2018

View reviewed changes

[Squash] Nits

6688e01

Trott previously requested changes Jul 29, 2018

View reviewed changes

[Squash] Add note to DEP0103

dd83b73

jasnell closed this Aug 1, 2018

jasnell mentioned this pull request Aug 1, 2018

doc: fixup process.binding deprecation code #22062

Closed

3 tasks

jdalton mentioned this pull request Aug 1, 2018

Migration from process.binding #22064

Closed

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 1, 2018

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

+              Type: Documentation-only
+              The `process.binding()` API is intended for use strictly by Node.js internal
+              code to provide a bridge between Node.js' JavaScript and native code layer.


		Type: Documentation-only

		The `process.binding()` API is intended for use strictly by Node.js internal

doc: documentation deprecation of process.binding by jasnell · Pull Request #22004 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: documentation deprecation of process.binding - #22004

doc: documentation deprecation of process.binding#22004jasnell wants to merge 4 commits intonodejs:masternodejs/node:masterfrom jasnell:docs-deprecate-process-bindingjasnell/node:docs-deprecate-process-bindingCopy head branch name to clipboard

Conversation

jasnell commented Jul 27, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jul 27, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Trott Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

ChALkeR Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

Trott commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChALkeR left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdalton commented Jul 28, 2018

Uh oh!

mcollina commented Jul 28, 2018

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jul 29, 2018

Uh oh!

jasnell commented Jul 29, 2018

Uh oh!

Trott Jul 29, 2018

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Jul 29, 2018

Uh oh!

jasnell commented Jul 29, 2018

Uh oh!

jasnell commented Aug 1, 2018

Uh oh!

jasnell commented Aug 1, 2018

Uh oh!

ChALkeR commented Aug 1, 2018

Uh oh!

targos commented Aug 1, 2018

Uh oh!

jasnell commented Aug 1, 2018

Uh oh!

ChALkeR commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doc: documentation deprecation of process.binding#22004
jasnell wants to merge 4 commits into
nodejs:masterfrom
jasnell:docs-deprecate-process-binding

Trott Jul 27, 2018 •
edited
Loading

Trott commented Jul 27, 2018 •
edited
Loading

ChALkeR left a comment •
edited
Loading

ChALkeR commented Jul 27, 2018 •
edited
Loading

ChALkeR commented Aug 1, 2018 •
edited
Loading