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

trace_events: move trace_events to internalBinding #22159

Closed
wants to merge 1 commit into from
Closed

trace_events: move trace_events to internalBinding #22159

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 6, 2018

Move process.binding('trace_events') to internalBinding('trace_events')

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@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/445/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Aug 6, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

There is some minor overlap with #22029

@jasnell
Copy link
Member Author

jasnell commented Aug 6, 2018

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

@jasnell jasnell mentioned this pull request Aug 6, 2018
Closed
33 tasks
@maclover7 maclover7 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 6, 2018
@maclover7
Copy link
Contributor

CITGM (sqlite3 and winston have been flaky as of late): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1488/

maclover7
maclover7 approved these changes Aug 7, 2018
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

CITGM results look okay, LGTM if CI passes

@jasnell jasnell force-pushed the trace-events-internal-binding branch from 4a91a3f to 13d5db5 Compare August 9, 2018 17:17
@jasnell
Copy link
Member Author

jasnell commented Aug 9, 2018

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

jasnell added a commit that referenced this pull request Aug 9, 2018
@jasnell
trace_events: move trace_events to internalBinding
c1e2d6b 
PR-URL: #22159
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
@jasnell
Copy link
Member Author

jasnell commented Aug 9, 2018

Landed in c1e2d6b

@jasnell jasnell closed this Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@maclover7 maclover7 maclover7 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
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. test Issues and PRs related to the tests. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasnell @nodejs-github-bot @maclover7 @targos @joyeecheung @trivikr