Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
jasnell wants to merge 1 commit into from
Closed

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

jasnell wants to merge 1 commit into from

Conversation

@jasnell
Copy link
Member

@jasnelljasnell commented Aug 12, 2018

Refs: #22160

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@jasnelljasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 12, 2018
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 12, 2018

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

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Aug 12, 2018
@jasnelljasnell mentioned this pull request Aug 12, 2018
Closed
33 tasks
@BridgeARBridgeAR requested a review from a teamAugust 13, 2018 00:32
@BridgeAR
Copy link
Member

BridgeAR commented Aug 13, 2018

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

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 13, 2018
jdalton
jdalton suggested changes Aug 13, 2018
View reviewed changes
Copy link
Member

@jdaltonjdalton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Holding removals until migration is ironed out

jdalton
jdalton suggested changes Aug 13, 2018
View reviewed changes
Copy link
Member

@jdaltonjdalton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

[wrong button]

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 13, 2018

@jdalton ... The require('v8') and process.binding('v8') bindings are explicitly not covered by existing semver rules.

See https://nodejs.org/dist/latest-v10.x/docs/api/v8.html#v8_v8:

The APIs and implementation are subject to change at any time.

Therefore, this PR should not be blocked by the transition discussion.

@jdalton
Copy link
Member

jdalton commented Aug 13, 2018

Therefore, this PR should not be blocked by the transition discussion.

I'd dig it if we could get a rough wag at usage before skipping deprecation processes.

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 13, 2018

Technically the v8 module is not covered by the normal deprecation processes so they're not being skipped. However, once #22269 lands, I can add 'v8' to the whitelist for the time being.

@jasnelljasnellforce-pushed the v8-internal-binding branch from 01a0230 to a2d45fbCompareAugust 15, 2018 02:26
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 15, 2018

@jdalton ... added process.binding('v8') to the fallthrough whitelist

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 15, 2018

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

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 15, 2018

ping @nodejs/tsc

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 16, 2018

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

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 16, 2018

Another CI failure in linux-containered: https://ci.nodejs.org/job/node-test-commit-linux-containered/6330/

@jasnelljasnellforce-pushed the v8-internal-binding branch 2 times, most recently from 8840799 to 9a0dcd0CompareAugust 16, 2018 21:27
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 16, 2018

Trying CI yet again: https://ci.nodejs.org/job/node-test-pull-request/16500/

@Trott
Copy link
Member

Trott commented Aug 17, 2018

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16509/

This was referenced Aug 17, 2018
Closed
Closed
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

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

@jasnelljasnellforce-pushed the v8-internal-binding branch from 9a0dcd0 to e0a0987CompareAugust 18, 2018 13:33
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

Trying again: https://ci.nodejs.org/job/node-test-pull-request/16536/

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

Trying arm fanned again because of an unrelated failure... https://ci.nodejs.org/job/node-test-commit-arm-fanned/3012/

@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018•
edited
Loading

Arm... again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3023/

@jasnelljasnellforce-pushed the v8-internal-binding branch from e0a0987 to c5a3d31CompareAugust 18, 2018 20:13
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

Full CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/16555/

@Trott
Copy link
Member

Trott commented Aug 18, 2018

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16560/

jasnell added a commit to jasnell/node that referenced this pull request Aug 18, 2018
@jasnell
v8: move process.binding('v8') to internalBinding
892932f
PR-URL: nodejs#22288 Refs: nodejs#22160 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
MemberAuthor

jasnell commented Aug 18, 2018

Landed in 892932f

@jasnelljasnell closed this Aug 18, 2018
@joyeecheungjoyeecheung mentioned this pull request Aug 20, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes

@cjihrigcjihrigcjihrig approved these changes

@joyeecheungjoyeecheungjoyeecheung approved these changes

@BridgeARBridgeARBridgeAR approved these changes

@trivikrtrivikrtrivikr approved these changes

+5 more reviewers

@jdaltonjdaltonjdalton approved these changes

@TimothyGuTimothyGuTimothyGu approved these changes

@maclover7maclover7maclover7 approved these changes

@hiroppyhiroppyhiroppy approved these changes

@devsnekdevsnekdevsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.c++Issues and PRs that require attention from people who are familiar with C++.semver-majorPRs that contain breaking changes and should be released in the next major version.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

13 participants

@jasnell@nodejs-github-bot@BridgeAR@jdalton@Trott@addaleax@TimothyGu@cjihrig@maclover7@hiroppy@joyeecheung@devsnek@trivikr