inspector,doc: update hint text, add guide by joshgav · Pull Request #8978 · nodejs/node · GitHub

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

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

LGTM with a few formatting nits.

LGTM

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

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!

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.

@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?

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

@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>

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

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

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

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

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.

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

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

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

hint text update discussion moved to #11207

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

hybrist reviewed Dec 17, 2016

View reviewed changes

hybrist 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

hybrist 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

hybrist mentioned this pull request Feb 16, 2017

Switch the CLI debugger to V8 inspector #11421

Closed

14 tasks

-                    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"


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

		<table cellpadding=0 cellspacing=0>

+                * **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`

inspector,doc: update hint text, add guide by joshgav · Pull Request #8978 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

inspector,doc: update hint text, add guide#8978joshgav wants to merge 3 commits intonodejs:masternodejs/node:masterfrom joshgav:debugging_guidejoshgav/node:debugging_guideCopy head branch name to clipboard

Conversation

joshgav commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshgav Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Oct 7, 2016

Uh oh!

eugeneo commented Oct 7, 2016

Uh oh!

mscdex commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshgav commented Oct 8, 2016

Uh oh!

joshgav commented Oct 11, 2016

Uh oh!

joshgav commented Oct 15, 2016

Uh oh!

gibfahn commented Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshgav commented Oct 19, 2016

Uh oh!

mscdex Oct 19, 2016

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Oct 19, 2016

Uh oh!

joshgav commented Oct 19, 2016

Uh oh!

mhdawson commented Oct 19, 2016

Uh oh!

auchenberg commented Nov 7, 2016

Uh oh!

hybrist Dec 17, 2016

Choose a reason for hiding this comment

Uh oh!

richardlau Dec 18, 2016

Choose a reason for hiding this comment

Uh oh!

joshgav commented Feb 6, 2017

Uh oh!

joshgav commented Feb 6, 2017

Uh oh!

joshgav commented Feb 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

inspector,doc: update hint text, add guide#8978
joshgav wants to merge 3 commits into
nodejs:masterfrom
joshgav:debugging_guide

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