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

doc: deprecate vm.runInDebugContext #12243

Closed
wants to merge 1 commit into
base: master
from

Conversation

@joshgav
Member

joshgav commented Apr 5, 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)

src, debugger


Per https://github.com/nodejs/diagnostics/blob/master/wg-meetings/2017-03-23.md#3-deprecate-vmrunindebugcontext-api:

vm.runInDebugContext will go away at end of 2017, so we should deprecate now in preparation for Node 10 (April 2018).

@jasnell should this be included in 8.0.0? Sorry it's late...

cc @ofrobots @eugeneo

@joshgav joshgav added this to the 8.0.0 milestone Apr 5, 2017

@nodejs-github-bot nodejs-github-bot added the vm label Apr 5, 2017

Show outdated Hide outdated doc/api/deprecations.md Outdated
Show outdated Hide outdated doc/api/deprecations.md Outdated
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Apr 5, 2017

Member

FYI, https://github.com/strongloop/strong-agent/blob/7c3109b140678b98bda19ff914d9937c886e21e3/lib/dyninst.js#L19-L27, @ofrobots

I think it was added because it can be useful, and had that specific use of accessing Debug, I'm not sure if it was ever used much or at all for anything else.

Member

sam-github commented Apr 5, 2017

FYI, https://github.com/strongloop/strong-agent/blob/7c3109b140678b98bda19ff914d9937c886e21e3/lib/dyninst.js#L19-L27, @ofrobots

I think it was added because it can be useful, and had that specific use of accessing Debug, I'm not sure if it was ever used much or at all for anything else.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Apr 5, 2017

Member

If its going to break, I'm +1 for deprecating in 8.x, to give people a chance to work around the upcoming disappearance.

Member

sam-github commented Apr 5, 2017

If its going to break, I'm +1 for deprecating in 8.x, to give people a chance to work around the upcoming disappearance.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 5, 2017

Member

If this is dependent in any way on the V8 debugger, then it absolutely would need to be pulled in 8.0.0 since the debugger is going to be pulled.

Member

jasnell commented Apr 5, 2017

If this is dependent in any way on the V8 debugger, then it absolutely would need to be pulled in 8.0.0 since the debugger is going to be pulled.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 5, 2017

Member

Regarding the DEP00NN number, we should actually get some tooling in to handle these like the REPLACEME tags. The number should be assigned either when the PR lands or when the release is cut.

Member

jasnell commented Apr 5, 2017

Regarding the DEP00NN number, we should actually get some tooling in to handle these like the REPLACEME tags. The number should be assigned either when the PR lands or when the release is cut.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 5, 2017

Contributor

This is not dependent on the JSON Debug Protocol (--debug) which is gone in V8 5.8. This is dependent on the in process debug context which is slated to go away by EOY (i.e. will not be available in Node 10.x). Getting this deprecation landed in 8.x gives people time to migrate away.

@sam-github Speaking of migration, the idea is that V8-inspector can be used for in process debugging. The first step in enabling this is #11431. We are planning on a few follow-on PRs that will make the inspector available to userspace to allow functionality equivalent to what the Debug context is used for today.

Contributor

ofrobots commented Apr 5, 2017

This is not dependent on the JSON Debug Protocol (--debug) which is gone in V8 5.8. This is dependent on the in process debug context which is slated to go away by EOY (i.e. will not be available in Node 10.x). Getting this deprecation landed in 8.x gives people time to migrate away.

@sam-github Speaking of migration, the idea is that V8-inspector can be used for in process debugging. The first step in enabling this is #11431. We are planning on a few follow-on PRs that will make the inspector available to userspace to allow functionality equivalent to what the Debug context is used for today.

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Apr 6, 2017

Member

Before deprecating the function, I think we should at least make sure our code base is no longer using it (#11875)

Member

TimothyGu commented Apr 6, 2017

Before deprecating the function, I think we should at least make sure our code base is no longer using it (#11875)

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 6, 2017

Contributor

@TimothyGu Indeed. However It is hard to do that before 8.x because we need some semver major changes to land before the alternative API is possible. I do think you have a valid point however, it is not nice to deprecate an API before the alternative is really possible.

This would leave us with NOT deprecating this at 8.0 which would give us less than 2 major cycles before it gets removed in 10.x.

Contributor

ofrobots commented Apr 6, 2017

@TimothyGu Indeed. However It is hard to do that before 8.x because we need some semver major changes to land before the alternative API is possible. I do think you have a valid point however, it is not nice to deprecate an API before the alternative is really possible.

This would leave us with NOT deprecating this at 8.0 which would give us less than 2 major cycles before it gets removed in 10.x.

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Apr 6, 2017

Member

@ofrobots To be clear I'm not against this PR, since whether we warn or not the API is going to be removed from V8 very very soon. What concerns me more is the fact that none of our publicly-exposed functions should be printing a deprecation warning. Well, except for that deprecated function itself.

For example, until modern V8 APIs that support what we need are in, I'm fine with a function accessible only internally that does not print a deprecation warning, and an externally accessible function that does.

Member

TimothyGu commented Apr 6, 2017

@ofrobots To be clear I'm not against this PR, since whether we warn or not the API is going to be removed from V8 very very soon. What concerns me more is the fact that none of our publicly-exposed functions should be printing a deprecation warning. Well, except for that deprecated function itself.

For example, until modern V8 APIs that support what we need are in, I'm fine with a function accessible only internally that does not print a deprecation warning, and an externally accessible function that does.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 6, 2017

Member

Good point, we cannot have core generate deprecation notices from it's own use of APIs. That's just bad form. Having an internal/*.js method that does the right thing with the public facing API generating the deprecation is a good compromise that we use in other places.

Member

jasnell commented Apr 6, 2017

Good point, we cannot have core generate deprecation notices from it's own use of APIs. That's just bad form. Having an internal/*.js method that does the right thing with the public facing API generating the deprecation is a good compromise that we use in other places.

@TimothyGu TimothyGu referenced this pull request Apr 6, 2017

Merged

util: use V8 C++ API for inspecting Promises #12254

2 of 2 tasks complete
@TimothyGu

TimothyGu requested changes Apr 6, 2017 edited

Blocking this before #12243 (comment) is resolved.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 6, 2017

Contributor

PR for JavaScript bindings for the inspector is now open: #12263.

Contributor

ofrobots commented Apr 6, 2017

PR for JavaScript bindings for the inspector is now open: #12263.

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Apr 7, 2017

Member

@TimothyGu

none of our publicly-exposed functions should be printing a deprecation warning

Would it be reasonable to wrap ensureDebugIsInitialized with a process.noDeprecation = true statement in the meantime?

Member

joshgav commented Apr 7, 2017

@TimothyGu

none of our publicly-exposed functions should be printing a deprecation warning

Would it be reasonable to wrap ensureDebugIsInitialized with a process.noDeprecation = true statement in the meantime?

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Apr 7, 2017

Member

@joshgav That'd work for me.

Member

TimothyGu commented Apr 7, 2017

@joshgav That'd work for me.

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Apr 7, 2017

Member

@TimothyGu done, PTAL.

Member

joshgav commented Apr 7, 2017

@TimothyGu done, PTAL.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Apr 9, 2017

Member

Sorry to join in late, can I ask what will be the alternative?

Member

refack commented Apr 9, 2017

Sorry to join in late, can I ask what will be the alternative?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 10, 2017

Member

@refack Depending on your use case, better native APIs (e.g. for inspecting Promises) or something like #12263

Member

addaleax commented Apr 10, 2017

@refack Depending on your use case, better native APIs (e.g. for inspecting Promises) or something like #12263

Show outdated Hide outdated doc/api/vm.md Outdated

@ofrobots ofrobots referenced this pull request Apr 10, 2017

Closed

inspector: JavaScript bindings for the inspector #12263

4 of 4 tasks complete
@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 10, 2017

Contributor

While we brushed the internal uses of runInDebugContext under the rug, it is still pretty bad form that we are deprecating the API without providing the ecosystem with an alternative.

Contributor

ofrobots commented Apr 10, 2017

While we brushed the internal uses of runInDebugContext under the rug, it is still pretty bad form that we are deprecating the API without providing the ecosystem with an alternative.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Apr 10, 2017

Member

@ofrobots any suggestions? We could not deprecate it... but then it will simply disappear without a full deprecation cycle, and v8 doesn't have an alternative yet if I understood the comments correctly, so we seem to be stuck between a rock and a hard place.

Member

sam-github commented Apr 10, 2017

@ofrobots any suggestions? We could not deprecate it... but then it will simply disappear without a full deprecation cycle, and v8 doesn't have an alternative yet if I understood the comments correctly, so we seem to be stuck between a rock and a hard place.

Show outdated Hide outdated doc/api/deprecations.md Outdated
@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 11, 2017

Contributor

@ofrobots any suggestions? We could not deprecate it... but then it will simply disappear without a full deprecation cycle, and v8 doesn't have an alternative yet if I understood the comments correctly, so we seem to be stuck between a rock and a hard place.

It seems that the API was experimental and it is not clear if the 2-major deprecation policy applies to V8 APIs anyway. IMO we should allow something like #12263 to happen and add the deprecation message around the time 8.x goes LTS. This will give allow the ecosystem to migrate. Perhaps this can be one of the new 'pending deprecations' for now?

Contributor

ofrobots commented Apr 11, 2017

@ofrobots any suggestions? We could not deprecate it... but then it will simply disappear without a full deprecation cycle, and v8 doesn't have an alternative yet if I understood the comments correctly, so we seem to be stuck between a rock and a hard place.

It seems that the API was experimental and it is not clear if the 2-major deprecation policy applies to V8 APIs anyway. IMO we should allow something like #12263 to happen and add the deprecation message around the time 8.x goes LTS. This will give allow the ecosystem to migrate. Perhaps this can be one of the new 'pending deprecations' for now?

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Apr 11, 2017

Member

@ofrobots We skip the 2-major rule when forced to by external deps, but you anticipate this API going away in 10.x?

If its going to disappear in node 10.x, then we could docs-only deprecate in 8.x, and runtime deprecate in 9.x, and outright delete in 10.x, which is as smooth as any deprecation cycle can be.

Member

sam-github commented Apr 11, 2017

@ofrobots We skip the 2-major rule when forced to by external deps, but you anticipate this API going away in 10.x?

If its going to disappear in node 10.x, then we could docs-only deprecate in 8.x, and runtime deprecate in 9.x, and outright delete in 10.x, which is as smooth as any deprecation cycle can be.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 11, 2017

Contributor

Good point. I agree that this should be a doc-only deprecation in 8.x. /cc @joshgav.

At the CTC-V8 meeting in February, the V8 team indicated that debug context will be removed by EOY, which lines up with Node 10.x.

Contributor

ofrobots commented Apr 11, 2017

Good point. I agree that this should be a doc-only deprecation in 8.x. /cc @joshgav.

At the CTC-V8 meeting in February, the V8 team indicated that debug context will be removed by EOY, which lines up with Node 10.x.

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav Apr 14, 2017

Member

@ofrobots @sam-github okay will update to docs-only deprecation, mention 10.x instead of a V8 version, and include @addaleax's suggestions above.

@ofrobots

we are deprecating the API without providing the ecosystem with an alternative

Is #12263 a sufficient alternative?

Member

joshgav commented Apr 14, 2017

@ofrobots @sam-github okay will update to docs-only deprecation, mention 10.x instead of a V8 version, and include @addaleax's suggestions above.

@ofrobots

we are deprecating the API without providing the ecosystem with an alternative

Is #12263 a sufficient alternative?

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Apr 17, 2017

Contributor

Is #12263 a sufficient alternative?

That's the hope. I think all the ecosystem uses can be addressed by it, but it is a sufficiently different API and mechanism. It would be good to offer this as an experimental API and get feedback from the ecosystem on whether it addresses all the use-cases.

Contributor

ofrobots commented Apr 17, 2017

Is #12263 a sufficient alternative?

That's the hope. I think all the ecosystem uses can be addressed by it, but it is a sufficiently different API and mechanism. It would be good to offer this as an experimental API and get feedback from the ecosystem on whether it addresses all the use-cases.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 18, 2017

Member

this needs a rebase :-)

Member

jasnell commented Apr 18, 2017

this needs a rebase :-)

@joshgav joshgav changed the title from src: deprecate vm.runInDebugContext to doc: deprecate vm.runInDebugContext May 3, 2017

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav May 3, 2017

Member

Split runtime deprecation into #12815 to land on 9.x. Updated this PR to only be a docs deprecation.

Member

joshgav commented May 3, 2017

Split runtime deprecation into #12815 to land on 9.x. Updated this PR to only be a docs deprecation.

doc: deprecate vm.runInDebugContext
Docs-only deprecation for v8.0.0.
Runtime deprecation planned for v9.0.0.
Removal planned for v10.0.0.

PR-URL: #12243
Reviewed-By: _tbd_
Reviewed-By: _tbd_
@addaleax

Yeah, looks good to me. It’s a bit unfortunate we have to go with just “An alternative is in development” but I guess that’s just the truth… ¯_(ツ)_/¯

@refack

refack approved these changes May 3, 2017

@joshgav

This comment has been minimized.

Show comment
Hide comment
@joshgav

joshgav May 3, 2017

Member

Landed in eb535c5. Thanks!

Member

joshgav commented May 3, 2017

Landed in eb535c5. Thanks!

@joshgav joshgav closed this May 3, 2017

joshgav added a commit that referenced this pull request May 3, 2017

doc: deprecate vm.runInDebugContext
Docs-only deprecation for v8.0.0.
Runtime deprecation planned for v9.0.0.
Removal planned for v10.0.0.

PR-URL: #12243
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

anchnk added a commit to anchnk/node that referenced this pull request May 6, 2017

doc: deprecate vm.runInDebugContext
Docs-only deprecation for v8.0.0.
Runtime deprecation planned for v9.0.0.
Removal planned for v10.0.0.

PR-URL: nodejs#12243
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@refack refack referenced this pull request Jul 14, 2017

Closed

[wip] src, inspector: support opted-in VM contexts #14231

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment