Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process: wrap process.binding for selective fallthrough to internalBinding #22269

Closed
wants to merge 3 commits into from

Conversation

Projects
8 participants
@jasnell
Copy link
Member

commented Aug 11, 2018

Selectively allow process.binding() to fall-through to internalBinding()

This adds a selective deprecation. We might want to make this a PendingDeprecationWarning instead of a regular DeprecationWarning.

Refs: #22163

/cc @addaleax

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
process: wrap process.binding for selective deprecation
Selectively deprecate `process.binding()` and fallthrough

Refs: #22163
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2018

@jasnell jasnell referenced this pull request Aug 11, 2018

Closed

uv: move process.binding('uv') to internalBinding #22163

3 of 3 tasks complete
@devsnek
Copy link
Member

left a comment

lgtm with DeprecationWarning

@addaleax
Copy link
Member

left a comment

Nice, thanks!

@devsnek

This comment has been minimized.

Copy link
Member

commented Aug 11, 2018

actually @jasnell can you switch this to use SafeSet from internal/safe_globals? it's currently possible to hack into internalBinding by overriding Set.prototype methods.

@maclover7

This comment has been minimized.

@maclover7
Copy link
Member

left a comment

CITGM run has yargs looking okay, with docs added this looks good

if (!internalBindingWarned.has(name)) {
process.emitWarning(
`Use of process.binding('${name}') is deprecated.`,
'DeprecationWarning', 'DEP0111');

This comment has been minimized.

Copy link
@maclover7

maclover7 Aug 12, 2018

Member

Can you add docs in doc/api/deprecations.md

This comment has been minimized.

Copy link
@devsnek

devsnek Aug 12, 2018

Member

also it should be DEP00XX

This comment has been minimized.

Copy link
@jasnell

jasnell Aug 12, 2018

Author Member

This is part of the existing DEP0111 process.binding deprecation. The documentation is already there. The odd but is that it's not a complete switch to a runtime deprecation.

// Deprecated specific process.binding() modules, but not all, allow
// selective fallback to internalBinding for the deprecated ones.
const processBinding = process.binding;
const internalBindingWhitelist = new Set(['uv']);

This comment has been minimized.

Copy link
@devsnek

devsnek Aug 12, 2018

Member

please use SafeSet

This comment has been minimized.

Copy link
@jasnell

jasnell Aug 13, 2018

Author Member

Done

@devsnek devsnek referenced this pull request Aug 12, 2018

Closed

url: move process.binding('url') to internalBinding #22204

2 of 2 tasks complete
@jdalton
Copy link
Member

left a comment

I'd like to hold this until an allow list, and those items in it, are more thoroughly digested. Many of the APIs that are deprecated have no user-land alternatives so leaves devs with nothing to fill the functionality gap. I think before a runtime error can be thrown we should have alternatives or guidance for the various APIs.

Update:

The move from process.binding to internalBinding is good. The mapping in process.binding to internalBinding, included in this PR, is good too (needed for a deprecation and migration path).

Update:

See additional comments, here and here, on migration and deprecation of recently removed bindings in master branch.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

@jdalton ... this is not adding a runtime error. It adds a runtime deprecation for only process.binding('uv') while fixing a regression that was added when migrating it to internalBinding('uv').

@jdalton

This comment has been minimized.

Copy link
Member

commented Aug 12, 2018

this is not adding a runtime error. It adds a runtime deprecation for only process.binding('uv')

The var name is a bit confusing as it suggests an allow list but then it issues a warning (Deprecation warnings can throw).

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

They throw only if --throw-deprecation is used, which is off by default.

Either way, we need to fix the process.binding('uv') regression... so perhaps I split the runtime deprecation specific part of this out to a separate PR but keep the rest as is?

@maclover7

This comment has been minimized.

Copy link
Member

commented Aug 12, 2018

@jasnell would it be possible to update per @devsnek's comment, and then to try and fast track this? Without a green CITGM (via this PR) we can't test the other binding removal PRs for regressions

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2018

We can't fast track a semver-major with a red x.

@jdalton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

I'm a fan of removing reliance on process but the jump to remove instead of deprecate, for many of these, is skipping a bit ahead. This PR (tweaked) or something like it is needed for proper deprecation. This includes deprecations for the existing removed items:

  • performance
  • serdes
  • trace_events
  • uv

I'll update my ✖️ above with this note

@devsnek

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@jdalton we only need to deprecate the ones with ecosystem usage. (like no one is using process.binding('serdes') because its exposed by v8) @ChALkeR said they were going to be looking into getting those numbers at some point.

@jdalton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@devsnek

we only need to deprecate the ones with ecosystem usage [...] were going to be looking into getting those numbers at some point.

Data is great for this!
It's also not horrible to leave them deprecated for a major release.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Unblocking and landing this specific PR gives us a way of actually deprecating these things incrementally.

@jdalton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@jasnell I 👍 the approach of your previous comment.

  • Remove the emitWarning bit
  • Allow all recently removed bindings through the allow-list (until we revisit with data)
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Allow all recently removed bindings

There is no need for all. trace_events and performance, for instance, are still experimental and still fall outside the normal semver rules.

@jdalton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

@jasnell

There is no need for all. trace_events and performance, for instance, are still experimental and still fall outside the normal semver rules.

Cool. You can address specifics in the follow-up commits in this PR.

@jasnell jasnell changed the title process: wrap process.binding for selective deprecation process: wrap process.binding for selective fallthrough to internalBinding Aug 13, 2018

@jasnell jasnell removed the semver-major label Aug 13, 2018

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Removed the deprecation warning. Will handle that in a separate PR.

Did not add the additional modules into the whitelist, prefer that to be done as separate PRs based on need.

@jdalton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Did not add the additional modules into the whitelist, prefer that to be done as separate PRs based on need.

This doesn't really satisfy the request. It's reasonable to add non-experimental bindings to the list until we can determine if they can safely be removed with data. I'm not a fan of a break-first-then-follow-up process. I dig the doc deprecate -> runtime warn -> then drop process.

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Please let me know which transitioned items you want to see landed... So far we've transitioned:

  • performance (experimental, outside semver)
  • serdes (internal, part of v8, which is explicitly outside semver)
  • trace_events (experimental, outside semver)
  • uv (added to the whitelist by this PR)

The other modules are handled by still open PRs that would need to be updated after this lands.

@jdalton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

I don't think we have data on usage yet so if the bindings aren't experimental they should be added to the allow-list until then (so serdes added to the allow list)

@jasnell jasnell referenced this pull request Aug 13, 2018

Closed

v8: move process.binding('v8') to internalBinding #22288

2 of 2 tasks complete
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

image

serdes is experimental... specifically, it is experimental within an existing module that falls outside of normal server rules.

@jdalton

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

Ah nice! Ok then!

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

jasnell added a commit that referenced this pull request Aug 15, 2018

process: wrap process.binding for selective fallthrough
Selectively fallthrough `process.binding()` to `internalBinding()`

Refs: #22163

PR-URL: #22269
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jasnell

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

Landed in 0257fd7

@jasnell jasnell closed this Aug 15, 2018

@targos targos added this to Don't land (for now) in v10.x Sep 23, 2018

@targos targos moved this from Don't land (for now) to Don't land (ever) in v10.x Sep 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.