inspector,doc: update hint text, add guide #8978

joshgav · 2016-10-07T23:05:27Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

inspector, doc

Description of change

Updates hint text for --inspect to a generic websockets URL and reference to more info on interface and available clients. Includes an initial draft of the guide with that info.

Tests for new message included too.

Fixes #7182
/cc @nodejs/diagnostics

jasnell · 2016-10-07T23:24:34Z

doc/guides/debugging.md

+specification of the debugger's capabilities.
+
+When started with the `node debug` command, Node.js runs the specified user script in a
+child process with the `--debug-brk` flag and starts a debugger client which connects to that process.


nit: long lines

Fixed the long lines outside of tables and long URLs. Will switch to an embedded HTML table for better control. Not sure what to do about the long URLs...

jasnell · 2016-10-07T23:25:05Z

LGTM with a few formatting nits.

eugeneo · 2016-10-07T23:31:33Z

LGTM

mscdex · 2016-10-07T23:45:28Z

I guess technically this should be blocked until #8979 has landed?

joshgav · 2016-10-08T00:03:28Z

I need to fix a test for this too but wanted to get consensus on the text.

The chrome-devtools URL is still in the hint text at the moment, but I think we should move it to the guide too. @eugeneo is the URL with the hash what we should put there? Or something different? Thanks!

joshgav · 2016-10-11T02:28:08Z

Code part here including tests should now be done, PTAL. Still need to figure out how/where to put the guide, see nodejs/nodejs.org#901.

Latest push:

Fixes up hint text a bit more and moves chrome-devtools URL to referenced guide.
Adds more info to guide on getting started with Chrome DevTools or VS Code, and placeholders for a couple more.
Fixes inspector test for new text.

joshgav · 2016-10-15T00:31:11Z

@ofrobots

Is there an urgency to removing the copy-pastable link?

The main reason I think is to ensure users are aware that other tools can also be used with --inspect. Some users may mistakenly think only Chrome DevTools can be used, which would be unfortunate as we encourage and expand the Node tools ecosystem. Not to take anything away from CDT, which certainly is a great option!

Copying the link is still less friction-ful than opening a webpage with a guide.

Agreed, but does this mean some one-liner must always be included in the hint? Also, is it the responsibility of Node itself to remove this friction, or is it something to leave to the ecosystem?

Perhaps the one-liner should refer to the built-in debugger (node debug) to be more generic?

gibfahn · 2016-10-15T20:20:23Z

@joshgav Agreed that we should make it clear that a number of tools can be used with --inspect, but wouldn't the answer then be to include text that explains that there are other options? Is the worry that users will see the URL and ignore the rest of the text?

Also can the URL be used in other debugging tools? As I understand it the Chrome Debugging Protocol is going to be standardised as the protocol for node debugging. Presumably in the future browser based ones will open via a (generic) URL, and IDE based ones will usually be run directly from the IDE.

If (for example) someone was debugging their node app through VS Code, would they not just use the debugging tab in VS Code itself, rather than running node --inspect manually? Would they even see this message?

Let me know if I'm misunderstanding!

Intended as neutral, not rejection.

joshgav · 2016-10-19T15:00:41Z

@gibfahn

would the answer then be to include text that explains that there are other options?

This was the first option suggested in nodejs/diagnostics#68. We determined that this would clutter up the output too much and be unhelpful.

Also can the URL be used in other debugging tools?

This was the third option suggested - to standardize the scheme (i.e. currently chrome-devtools://) to work with other tools too. We decided this is a worthwhile long-term goal but wouldn't be possible in the near-term.

Would they even see this message?

The text is part of console output (actually stderr) so shows up in IDEs and when processes are started from the terminal.

Ultimately this PR is based on option 2 in nodejs/diagnostics#68, which we decided was the most straightforward to implement now: use a generic URL in the hint text and provide more detail in a linked wiki.

At this point I believe we have consensus on moving to the new text I proposed here eventually, but the question is when. I believe we should make this change now before we cut v7.

@nodejs/ctc - would like the CTC's thoughts on this - should we land now or later? If later, what milestone or date should we target?

Updates hint text for --inspect to be more generic. Instructions for various debugger clients moved into a referenced guide. PR-URL: nodejs#8978 Fixes: nodejs#7182 Reviewed-By: <tbd> Reviewed-By: <tbd>

mscdex · 2016-10-19T15:05:38Z

doc/guides/debugging.md

+
+* The following table clarifies the implications of various diag-related flags:
+
+<table cellpadding=0 cellspacing=0>


Seeing all this HTML makes me cringe a bit. What about using github's table markdown syntax?

evanlucas · 2016-10-19T15:11:49Z

I don't think we should land this until (stable) chrome allows listing debuggable node targets via chrome://inspect. So, -1 from me for landing in v7

joshgav · 2016-10-19T16:09:13Z

@mscdex

What about using github's table markdown syntax?

Would love that and did so originally, but lines exceeded 80 columns. Also I couldn't do a multiline cell. Would be happy to try switching back.

mhdawson · 2016-10-19T20:29:58Z

I also think we don't need to rush it into 7.

auchenberg · 2016-11-07T14:10:33Z

The community has now build inspect-process, which adds a inspect cli command that starts node --inspect and Chrome DevTools with the right url.

I see this as an argument for removing chrome-specific URL in Node core.

jkrems · 2016-12-17T18:55:21Z

doc/guides/debugging.md

+  * **Option 2**: Paste the following URL in Chrome,
+    replacing the `ws` parameter with your hostname and port as needed:
+
+    `chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=localhost:9229/node`


I guess this no longer works, given that /node is now a random UUID?

richardlau · 2016-12-18T05:18:36Z

src/inspector_agent.cc

-      port, GetWsUrl(port, id).c_str());
+  fprintf(stderr, "Debugger listening on \x1B[33mws://%s\x1b[0m\n"
+  "* Info on connecting at"
+    "\x1B[33mhttps://nodejs.org/en/docs/guides/debugging\x1B[0m.\n"


I don't think we can assume stderr supports ANSI escape codes.

joshgav · 2017-02-06T20:37:29Z

debugging guide moved to nodejs.org website location: nodejs/nodejs.org#1131

joshgav · 2017-02-06T22:36:33Z

hint text update discussion moved to #11207

joshgav · 2017-02-06T22:38:03Z

closing in favor of new, separate PRs:

debugging guide: guides: debugging getting started guide nodejs.org#1131
hint text: src: update --inspect hint text #11207

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Oct 7, 2016

jasnell reviewed Oct 7, 2016

View reviewed changes

mscdex added the blocked PRs that are blocked by other issues or PRs. label Oct 7, 2016

joshgav force-pushed the debugging_guide branch from 52ca7d9 to b478ddc Compare October 10, 2016 19:01

joshgav mentioned this pull request Oct 10, 2016

Discoverability: guides nodejs/nodejs.org#901

Closed

joshgav force-pushed the debugging_guide branch 12 times, most recently from af1430a to 8c9a1e5 Compare October 11, 2016 02:14

joshgav force-pushed the debugging_guide branch from 8c9a1e5 to bb5509c Compare October 11, 2016 15:58

joshgav force-pushed the debugging_guide branch from bb5509c to f3a5ce9 Compare October 11, 2016 16:01

joshgav changed the title ~~inspector,doc: update hint text, add guide [WIP]~~ inspector,doc: update hint text, add guide Oct 11, 2016

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

joshgav removed the blocked PRs that are blocked by other issues or PRs. label Oct 19, 2016

joshgav added 3 commits October 19, 2016 08:00

inspector,doc: update hint text, add guide

a5932b2

Updates hint text for --inspect to be more generic. Instructions for various debugger clients moved into a referenced guide. PR-URL: nodejs#8978 Fixes: nodejs#7182 Reviewed-By: <tbd> Reviewed-By: <tbd>

squash: add webstorm instructions

0d45247

squash: update chrome-devtools URL

b0d785c

joshgav force-pushed the debugging_guide branch from bcf84ba to b0d785c Compare October 19, 2016 15:01

mscdex reviewed Oct 19, 2016

View reviewed changes

jkrems reviewed Dec 17, 2016

View reviewed changes

jkrems mentioned this pull request Dec 18, 2016

doc: Deprecate old debug protocol #10320

Closed

2 tasks

richardlau reviewed Dec 18, 2016

View reviewed changes

gibfahn mentioned this pull request Dec 22, 2016

Mention chrome://inspect when using node --inspect #10410

Closed

jkrems mentioned this pull request Jan 3, 2017

Bringing node-inspect into the fold nodejs/diagnostics#67

Closed

9 tasks

joshgav mentioned this pull request Jan 14, 2017

Diag WG Meeting - 2017-01-19 nodejs/diagnostics#79

Closed

joyeecheung mentioned this pull request Jan 30, 2017

Bringing network debugging to V8 inspector #11072

Closed

joshgav mentioned this pull request Feb 6, 2017

guides: debugging getting started guide nodejs/nodejs.org#1131

Merged

joshgav mentioned this pull request Feb 6, 2017

src: update --inspect hint text #11207

Closed

4 tasks

joshgav closed this Feb 6, 2017

jkrems mentioned this pull request Feb 16, 2017

Switch the CLI debugger to V8 inspector #11421

Closed

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inspector,doc: update hint text, add guide #8978

inspector,doc: update hint text, add guide #8978

joshgav commented Oct 7, 2016 •

edited

Loading

jasnell Oct 7, 2016 •

edited

Loading

joshgav Oct 10, 2016

jasnell commented Oct 7, 2016

eugeneo commented Oct 7, 2016

mscdex commented Oct 7, 2016 •

edited

Loading

joshgav commented Oct 8, 2016

joshgav commented Oct 11, 2016

joshgav commented Oct 15, 2016

gibfahn commented Oct 15, 2016 •

edited

Loading

joshgav commented Oct 19, 2016

mscdex Oct 19, 2016

evanlucas commented Oct 19, 2016

joshgav commented Oct 19, 2016

mhdawson commented Oct 19, 2016

auchenberg commented Nov 7, 2016

jkrems Dec 17, 2016

richardlau Dec 18, 2016

joshgav commented Feb 6, 2017

joshgav commented Feb 6, 2017

joshgav commented Feb 6, 2017


		* The following table clarifies the implications of various diag-related flags:

		<table cellpadding=0 cellspacing=0>

inspector,doc: update hint text, add guide #8978

inspector,doc: update hint text, add guide #8978

Conversation

joshgav commented Oct 7, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

joshgav Oct 10, 2016

Choose a reason for hiding this comment

jasnell commented Oct 7, 2016

eugeneo commented Oct 7, 2016

mscdex commented Oct 7, 2016 • edited Loading

joshgav commented Oct 8, 2016

joshgav commented Oct 11, 2016

joshgav commented Oct 15, 2016

gibfahn commented Oct 15, 2016 • edited Loading

joshgav commented Oct 19, 2016

mscdex Oct 19, 2016

Choose a reason for hiding this comment

evanlucas commented Oct 19, 2016

joshgav commented Oct 19, 2016

mhdawson commented Oct 19, 2016

auchenberg commented Nov 7, 2016

jkrems Dec 17, 2016

Choose a reason for hiding this comment

richardlau Dec 18, 2016

Choose a reason for hiding this comment

joshgav commented Feb 6, 2017

joshgav commented Feb 6, 2017

joshgav commented Feb 6, 2017

joshgav commented Oct 7, 2016 •

edited

Loading

jasnell Oct 7, 2016 •

edited

Loading

mscdex commented Oct 7, 2016 •

edited

Loading

gibfahn commented Oct 15, 2016 •

edited

Loading