Skip to content

Conversation

@jasnell
Copy link
Member

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
Copy link
Collaborator

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

@mcollinamcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@addaleaxaddaleax added the process Issues and PRs related to the process subsystem. label Jul 27, 2018
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.
Copy link
Member

@TrottTrottJul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove strictly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe this?:

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

@Trott
Copy link
Member

Trott commented Jul 27, 2018

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.

Copy link
Member

@ChALkeRChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

ChALkeR commented Jul 27, 2018

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

@jdalton
Copy link
Member

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
Copy link
Member

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

Copy link
Member

@targostargos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with @Trott comments addressed

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jasnell
Copy link
MemberAuthor

@jasnell
Copy link
MemberAuthor

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

Trott
Trott previously requested changes Jul 29, 2018
Type: Documentation-only
The `process.binding()` API is intended for use by Node.js internal only
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@TrottTrott dismissed their stale reviewJuly 29, 2018 19:54

fixed the word ordering

@ChALkeR
Copy link
Member

What about DEP0103?

@jasnell
Copy link
MemberAuthor

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

@jasnell
Copy link
MemberAuthor

jasnell added a commit that referenced this pull request Aug 1, 2018
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@jasnell
Copy link
MemberAuthor

Landed in 182051b

@jasnelljasnell closed this Aug 1, 2018
@ChALkeR
Copy link
Member

@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
Copy link
Member

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

@jasnell
Copy link
MemberAuthor

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
Copy link
Member

ChALkeR commented Aug 1, 2018

@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
Copy link
MemberAuthor

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

@jasnell
Copy link
MemberAuthor

Fixup here: #22062

@jdalton
Copy link
Member

@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

@TrottTrott added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 1, 2018
targos pushed a commit that referenced this pull request Aug 1, 2018
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@rvaggrvagg mentioned this pull request Aug 13, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecationsIssues and PRs related to deprecations.docIssues and PRs related to the documentations.notable-changePRs with changes that should be highlighted in changelogs.processIssues and PRs related to the process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

18 participants

@jasnell@nodejs-github-bot@Trott@ChALkeR@jdalton@mcollina@targos@bengl@addaleax@cjihrig@sagirk@tniessen@devsnek@gireeshpunathil@vdeturckheim@BridgeAR@mhdawson@vsemozhetbyt