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 old debug protocol #10320

Closed
wants to merge 1 commit into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Dec 17, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

As discussed in #10276, we'd want to add a soft deprecation of the --debug flag to the docs.

This just adds the deprecation to the "advanced usage" section, not to the overall page. I think we can actually keep supporting node debug <filename> since it doesn't leak the protocol used (as pid and url do, both of which could have been started with a different version of node).

@@ -179,12 +183,13 @@ process or via URI reference to the listening debugger:
* `node debug <URI>` - Connects to the process via the URI such as
localhost:5858

## V8 Inspector Integration for Node.js
### V8 Inspector Integration for Node.js

**NOTE: This is an experimental feature.**
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should remove the "experimental feature" note, especially if we're pointing people to it as an alternative to the deprecated debugger. That doesn't have to happen in this PR (although it could if there's consensus that it should happen).

Copy link
Contributor Author

@jkrems jkrems Dec 18, 2016

Choose a reason for hiding this comment

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

Yeah, I was thinking about what the "least worst" option would be. It seemed also bad to remove the doc warning while the runtime still writes one ("docs say go ahead but then the runtime punishes the user with scary warnings").

P.S.: The solution could be to remove the warning in this PR, but there's #8978 already touching those parts.

@Trott
Copy link
Member

Trott commented Dec 18, 2016

@nodejs/ctc This is something we should probably decide on sooner rather than later. We may need to create an exception to our deprecation policy for the debugger situation. We may not have much of a choice (unless we want to not upgrade V8 for 18 months).

@@ -167,6 +167,10 @@ breakpoint)

## Advanced Usage

### TCP-based protocol

> Stability: 0 - Deprecated: Use [V8 Inspector Integration][] instead. The debug protocol used by the `--debug` flag was removed from V8.
Copy link
Member

Choose a reason for hiding this comment

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

nit: Can you wrap this at 80 characters? You should be able to use multiple >s to continue the blockquote, the documentation generator should be able to handle that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it on multiple lines at first but line breaks inside of > were preserved which looked awkward in the rendered version.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkrems It's fixed in #11074

@jkrems
Copy link
Contributor Author

jkrems commented Dec 22, 2016

Rebased, only conflict was the changed link at the very bottom.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member

Trott commented Jan 4, 2017

Are there semver implications for a docs-only deprecation? I'd like to land this sooner rather than later...

@jasnell
Copy link
Member

jasnell commented Jan 4, 2017 via email

@Trott
Copy link
Member

Trott commented Jan 4, 2017

All deprecations are semver major, including docs only

Can we and should we make an exception for this one? If I'm not mistaken, we expect the debugger to be not-working come Node.js 8, so a docs-only deprecation now would seem to be the kind thing to do for our users as opposed to having it go away with no warning.

@Fishrock123
Copy link
Contributor

All deprecations are semver major, including docs only

heh, it's not like we've approved deprecations in-major before when we had no other choice (smalloc)

@jasnell
Copy link
Member

jasnell commented Jan 4, 2017

I'd rather not start getting into the habit of making exceptions for deprecations. If we want to treat docs only deprecations as anything less than semver-major, then we should establish that as policy and do all docs only deprecations as less than semver-major.

@targos
Copy link
Member

targos commented Jan 4, 2017

Correct me if I'm wrong, but I think that for this case, we must make an exception to our policy. We have to choose between:

  • a semver-minor docs only deprecation in 7.x, followed by the removal of the feature in 8.0
  • direct removal in 8.0 without deprecation

@Trott
Copy link
Member

Trott commented Jan 4, 2017

Consensus at CTC meeting today was that this can move forward.

(This and another issue highlighted how we need to more concretely document our semver policy and @jasnell is restarting an earlier effort to do that.)

Removing ctc-agenda label. Adding labels for hopefully landing this on v6 as well so that people who only use LTS versions have a chance of noticing the doc note.

@Trott Trott added dont-land-on-v4.x notable-change PRs with changes that should be highlighted in changelogs. and removed dont-land-on-v6.x labels Jan 4, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Jan 4, 2017
Due to changes in V8, the debug protocol will no longer work in Node.js
8.0.0. Note this in the documentation.

PR-URL: nodejs#10320
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott
Copy link
Member

Trott commented Jan 4, 2017

Landed in 59a61a2.

@jkrems Do we need to runtime deprecate the debug protocol too? Or are we thinking that there's a chance it can be an alias for inspect without too much fallout? Or another option? /cc @Fishrock123

@Trott Trott closed this Jan 4, 2017
@jkrems
Copy link
Contributor Author

jkrems commented Jan 4, 2017

@Trott The runtime deprecation was already kicked off by @joshgav here: #10276. But yes, warning at runtime (at least in 7.x, maybe/hopefully even in 6.x) would be super useful I think.

@jkrems jkrems deleted the jk-v8-protocol-deprecation branch January 4, 2017 17:46
@Trott
Copy link
Member

Trott commented Jan 10, 2017

Do we need to deprecate vm.runInDebugContext() too? Or is that likely to continue working?

@targos
Copy link
Member

targos commented Jan 10, 2017

I'm not sure. It's using the v8::Debug C++ API.
/cc @hashseed

@hashseed
Copy link
Member

hashseed commented Jan 10, 2017

vm.runInDebugContext uses the debug context. That is mostly unrelated to the legacy JSON protocol (which will be removed by V8 5.8). We will deprecate debug cont wi the API to access the debug context will be marked accordingly in include/v8.h, but not in the short term.

@evanlucas
Copy link
Contributor

This doesn't land cleanly on v7.x. Someone want to backport?

@jkrems
Copy link
Contributor Author

jkrems commented Feb 4, 2017

@evanlucas Backport = cherry-pick commit against 7.x, resolve conflicts, PR against 7.x branch? If so I could give it a shot.

@evanlucas
Copy link
Contributor

Yes, but Pr against v7.x-staging instead of v7.x

Thanks!

@jkrems
Copy link
Contributor Author

jkrems commented Feb 4, 2017

Resolved here: #11172

jkrems pushed a commit to jkrems/node that referenced this pull request Mar 20, 2017
Due to changes in V8, the debug protocol will no longer work in Node.js
8.0.0. Note this in the documentation.

PR-URL: nodejs#10320
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jkrems
Copy link
Contributor Author

jkrems commented Mar 20, 2017

The backport PR has been updated again. I really think we should get this into the published docs rather sooner than later, given how close we are to adding runtime deprecations (or even dropping support).

italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 21, 2017
Due to changes in V8, the debug protocol will no longer work in Node.js
8.0.0. Note this in the documentation.

PR-URL: nodejs#10320
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.