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.

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 from

Conversation

joshgav
Copy link
Contributor

@joshgav 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 debugger semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 5, 2017
@joshgav joshgav added this to the 8.0.0 milestone Apr 5, 2017
@nodejs-github-bot nodejs-github-bot added the vm Issues and PRs related to the vm subsystem. label Apr 5, 2017
@@ -582,6 +582,16 @@ deprecated.
*Note*: `OutgoingMessage.prototype._renderHeaders` was never documented as
an officially supported API.

<a id="DEP0068"></a>
### DEP0064: vm.runInDebugContext(string)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, need to update string to DEP0068 or whatever the final number is.

@@ -582,6 +582,16 @@ deprecated.
*Note*: `OutgoingMessage.prototype._renderHeaders` was never documented as
an officially supported API.

<a id="DEP0068"></a>
### DEP0064: vm.runInDebugContext(string)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these match?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed :) fixed.

@sam-github
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
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
Copy link
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
Copy link
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
Copy link
Member

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

@ofrobots
Copy link
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
Copy link
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
Copy link
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.

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.

Blocking this before #12243 (comment) is resolved.

@ofrobots
Copy link
Contributor

ofrobots commented Apr 6, 2017

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

@joshgav
Copy link
Contributor Author

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
Copy link
Member

@joshgav That'd work for me.

@joshgav
Copy link
Contributor Author

joshgav commented Apr 7, 2017

@TimothyGu done, PTAL.

@refack
Copy link
Contributor

refack commented Apr 9, 2017

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

@addaleax
Copy link
Member

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

@sam-github
Copy link
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.

Type: Runtime

The DebugContext is being removed from V8 5.8+ along with the legacy debugger
interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to mention when v8 5.8 is expected to land? Most people don't know what version of v8 is in specific node versions, so I suggest adding something like "Deprecated to warn users that this function will have to be deleted when Node.js next updates V8." (or something of the like).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually not accurate. The Debug context is slated to go away at the end of the year which would probably be either M63 or M64. It might be better to state the Node version (10.0) that this is expected to land in.

The legacy debugger interface is gone in V8 5.8.

@ofrobots
Copy link
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?

@sam-github
Copy link
Contributor

@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
Copy link
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.

@joshgav
Copy link
Contributor Author

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
Copy link
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.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

this needs a rebase :-)

@joshgav joshgav changed the title src: deprecate vm.runInDebugContext doc: deprecate vm.runInDebugContext May 3, 2017
@joshgav
Copy link
Contributor Author

joshgav commented May 3, 2017

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

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: _tbd_
Reviewed-By: _tbd_
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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… ¯_(ツ)_/¯

@joshgav
Copy link
Contributor Author

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
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 pushed a commit to anchnk/node that referenced this pull request May 6, 2017
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 mentioned this pull request May 11, 2017
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. semver-major PRs that contain breaking changes and should be released in the next major version. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.