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

jasnell · 2018-07-27T15:57:35Z

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.

nodejs-github-bot · 2018-07-27T15:57:36Z

@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

mcollina

LGTM

Trott · 2018-07-27T17:47:03Z

+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.


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.

Trott · 2018-07-27T17:48:15Z

+
+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.
+Use of `process.binding()` by user-land code is unsupported.


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

Trott · 2018-07-27T17:49:34Z

+
+Type: Documentation-only
+
+The `process.binding()` API is intended for use strictly by Node.js internal


Nit: remove strictly

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.

Trott · 2018-07-27T17:52:07Z

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.

ChALkeR

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.

ChALkeR · 2018-07-27T20:05:34Z

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

jdalton · 2018-07-28T02:46:12Z

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.

mcollina · 2018-07-28T03:47:31Z

@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!

targos

LGTM with @Trott comments addressed

mhdawson

LGTM

jasnell · 2018-07-29T17:28:22Z

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

jasnell · 2018-07-29T17:40:31Z

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

Trott · 2018-07-29T19:52:20Z

+
+Type: Documentation-only
+
+The `process.binding()` API is intended for use by Node.js internal only


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

fixed the word ordering

ChALkeR · 2018-07-29T21:28:59Z

What about DEP0103?

jasnell · 2018-07-29T22:29:02Z

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

jasnell · 2018-08-01T13:59:52Z

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>

jasnell · 2018-08-01T14:15:12Z

Landed in 182051b

ChALkeR · 2018-08-01T14:49:13Z

@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?

targos · 2018-08-01T14:51:26Z

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

jasnell · 2018-08-01T14:52:18Z

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

ChALkeR · 2018-08-01T14:53:58Z

@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?

jasnell · 2018-08-01T14:55:56Z

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

jasnell · 2018-08-01T14:58:11Z

Fixup here: #22062

jdalton · 2018-08-01T18:02:26Z

@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

Uh oh!

Uh oh!

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!

TrottJul 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!

TrottJul 27, 2018

Choose a reason for hiding this comment

Uh oh!

TrottJul 27, 2018

Choose a reason for hiding this comment

Uh oh!

TrottJul 27, 2018

Choose a reason for hiding this comment

Uh oh!

ChALkeRJul 27, 2018

Choose a reason for hiding this comment

Uh oh!

TrottJul 27, 2018

Choose a reason for hiding this comment

Uh oh!

TrottJul 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!

TrottJul 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!

jasnell commented Aug 1, 2018

Uh oh!

jasnell commented Aug 1, 2018

TrottJul 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