Skip to content

src: deprecate vm.runInDebugContext#12815

Closed
joshgav wants to merge 2 commits intonodejs:masterfrom
joshgav:deprecate-debugcontext-runtime
Closed

src: deprecate vm.runInDebugContext#12815
joshgav wants to merge 2 commits intonodejs:masterfrom
joshgav:deprecate-debugcontext-runtime

Conversation

@joshgav
Copy link
Contributor

@joshgav joshgav commented May 3, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Runtime deprecation of vm.runInDebugContext. To land in v9.x.

@joshgav joshgav added debugger vm Issues and PRs related to the vm subsystem. labels May 3, 2017
@nodejs-github-bot nodejs-github-bot added util Issues and PRs related to the built-in util module. vm Issues and PRs related to the vm subsystem. labels May 3, 2017
@joshgav joshgav mentioned this pull request May 3, 2017
Closed
4 tasks
@joshgav joshgav requested a review from ofrobots May 3, 2017 19:17
@Fishrock123
Copy link
Contributor

Fishrock123 commented May 3, 2017

Will there be no similar behavior?

TimothyGu
TimothyGu suggested changes May 4, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

The deprecation status in doc/api/deprecations.md also needs to be changed to Runtime.

doc/api/vm.md Outdated
Copy link
Member

@TimothyGu TimothyGu May 4, 2017

Choose a reason for hiding this comment

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

This change should go in separately with v8.x.

Copy link
Member

@jasnell jasnell May 4, 2017

Choose a reason for hiding this comment

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

This should be REPLACEME rather than listing a specific version here.

Copy link
Member

@addaleax addaleax Oct 22, 2017

Choose a reason for hiding this comment

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

@TimothyGu Done (but deprecated: v8.0.0 is correct since that’s when we docs-deprecated)

test/parallel/test-vm-debug-context.js Outdated
Copy link
Member

@TimothyGu TimothyGu May 4, 2017

Choose a reason for hiding this comment

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

Why const _?

Copy link
Member

@addaleax addaleax Oct 22, 2017

Choose a reason for hiding this comment

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

@TimothyGu done

@addaleax
Copy link
Member

addaleax commented May 23, 2017

ping @joshgav

@targos targos mentioned this pull request May 30, 2017
Merged
@fhinkel
Copy link
Member

fhinkel commented Aug 3, 2017

@joshgav can we close this in favor of #13295? I'm trying to figure out the status of deprecating vm.runInDebugContext() but PRs are sending me in circles.

@TimothyGu
Copy link
Member

TimothyGu commented Aug 31, 2017

@fhinkel Yes I believe so.

@TimothyGu TimothyGu closed this Aug 31, 2017
@targos
Copy link
Member

targos commented Oct 21, 2017

Should we reopen since #13295 won't be in v9?

@addaleax
Copy link
Member

addaleax commented Oct 22, 2017

@targos yup

@addaleax addaleax reopened this Oct 22, 2017
@addaleax addaleax added this to the 9.0.0 milestone Oct 22, 2017
@addaleax addaleax force-pushed the deprecate-debugcontext-runtime branch from 1894455 to 66fa03d Compare October 22, 2017 14:39
@addaleax
Copy link
Member

addaleax commented Oct 22, 2017 •
edited
Loading

I don’t think @joshgav is very active right now, so I’ve rebased and force-pushed this.

@nodejs/tsc This needs to be in Node 9, ptal

CI: https://ci.nodejs.org/job/node-test-commit/13371/

(edit: self-assignment to keep track, shouldn’t stop anybody from merging)

@addaleax addaleax self-assigned this Oct 22, 2017
@targos targos added semver-major PRs that contain breaking changes and should be released in the next major version. and removed dont-land-on-v4.x labels Oct 23, 2017
@targos
Copy link
Member

targos commented Oct 23, 2017

I just pushed a commit to change the deprecation type in the documentation.

@addaleax
Copy link
Member

addaleax commented Oct 23, 2017

Landed in 88e55fe

@addaleax addaleax closed this Oct 23, 2017
addaleax pushed a commit that referenced this pull request Oct 23, 2017
@joshgav @addaleax
vm: deprecate vm.runInDebugContext
88e55fe 
PR-URL: #12815
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@joshgav @addaleax
vm: deprecate vm.runInDebugContext
4192ffa 
PR-URL: nodejs/node#12815
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@joshgav @addaleax
vm: deprecate vm.runInDebugContext
09d80f2 
PR-URL: nodejs/node#12815
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@targos targos targos approved these changes

@ofrobots ofrobots Awaiting requested review from ofrobots

+1 more reviewer

@TimothyGu TimothyGu TimothyGu requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@addaleax addaleax

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

8 participants

@joshgav @Fishrock123 @addaleax @fhinkel @TimothyGu @targos @jasnell @nodejs-github-bot