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.

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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 12, 2018

Refs: #22160

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

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

@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-bot nodejs-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
@jasnell jasnell mentioned this pull request Aug 12, 2018
Closed
33 tasks
@BridgeAR BridgeAR requested a review from a team August 13, 2018 00:32
@BridgeAR
Copy link
Member

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

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

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

@jdalton jdalton 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
Member Author

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
Member Author

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.

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

ping @nodejs/tsc

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

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

@jasnell jasnell force-pushed the v8-internal-binding branch 2 times, most recently from 8840799 to 9a0dcd0 Compare August 16, 2018 21:27
@jasnell
Copy link
Member Author

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
Member Author

jasnell commented Aug 18, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 18, 2018

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

@jasnell
Copy link
Member Author

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
Member Author

jasnell commented Aug 18, 2018 •
edited
Loading

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

@jasnell
Copy link
Member Author

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
Member Author

jasnell commented Aug 18, 2018

Landed in 892932f

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

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@devsnek devsnek devsnek approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@hiroppy hiroppy hiroppy approved these changes

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@addaleax addaleax addaleax approved these changes

@jdalton jdalton jdalton approved these changes

Assignees
No one assigned
Labels
author ready PRs 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-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.