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

guides: debugging getting started guide #1131

Merged
merged 1 commit into from
Feb 28, 2017
Merged

guides: debugging getting started guide #1131

merged 1 commit into from
Feb 28, 2017

Conversation

joshgav
Copy link
Contributor

@joshgav joshgav commented Feb 6, 2017

A guide describing the basics of starting Node.js in debug/inspect mode and how to attach any client. Includes a list of popular clients.

A potential goal is to reference this from the hint text when Node.js is started with the --inspect flag, see nodejs/node#8978.

/cc @nodejs/diagnostics @eugeneo

to and debug the running process.

A running Node.js process can also be instructed to start listening for
debugging messages by signaling it with `SIGUSR1` (on Linux and OS X).
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we in the process of deprecating --debug and friends? IIRC, the behavior of SIGUSR1 is going to change, too.

Copy link

Choose a reason for hiding this comment

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

Since this document will appear in the version-independent "Guides" section of the doc, makes sense for it to discuss both --debug and --inspect. But they should be in separate sections I think - inspect first, then debug. You never use both at the same time (I assume!), so having them separated seems right. Each section should also mention which node versions support that type of debug.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the article would benefit from different content depending on a version, why not include version in the URL of the hint text? ...debugging_getting_started#v7

The main "Guides" article would have no assumptions about the user's version, but if opened from a URL printed by a particular one, some content can be replaced.

Having that option, even if there's only one version that actually benefits from this, might save us some convoluted explanation while being totally optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions, I'm going to gather the --debug info in its own section toward the end.

@naugtur: the markdown headers get converted into HTML anchors (fragment identifiers, so it should be possible to refer directly to the --debug section. I'm not going to make the content dynamic based on the fragment here (too much work at the moment), but if you think that would be useful you could start another PR. Thanks!

@eugeneo
Copy link
Contributor

eugeneo commented Feb 6, 2017

LGTM from the Chrome DevTools team. Not sure I should use GitHub "Approve" as there seem to be comments from other parties.

Metadata about the target debuggee including its UUID can be retrieved via HTTP
(not websockets) from `http://[host:port]/json/list` with host and port
properly specified (default: 127.0.0.1:9229). This returns a JSON object like
the following; use its `webSocketDebuggerUrl` as the connection string including
Copy link
Member

Choose a reason for hiding this comment

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

The last sentence seems to lack something? Who uses that string? (Maybe The debugger client may use..)

Copy link
Contributor

@naugtur naugtur Feb 23, 2017

Choose a reason for hiding this comment

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

That part looks good to me, but

"connection string" is used just once, here, with no prior explanation. ws:.../<UUID> is called "full URL" in the paragraph above.

I suggest:
This returns a JSON object like the one below. Its webSocketDebuggerUrl field is the full URL a debugger needs to connect to the target process because it includes the UUID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing now. @naugtur thanks for the suggestion, I'm taking out the term "connection string."

Several commercial and open source tools can connect to Node's
debugger and/or inspector. Info on these follows.

* [Built-in Debugger](https://github.com/nodejs/node/blob/master/lib/_debugger.js)
Copy link
Member

Choose a reason for hiding this comment

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

I think another way to organize things is:

### Inspector Protocol

* node-inspect
* Chrome DevTools
* VSCode

### Debugger Protocol

* Built-in Debugger
* Chrome DevTools
* VSCode

So people can first look for a client that uses the protocol they prefer(hopefully the inspector protocol) and look for alternatives if their current tool doesn't support the new protocol, but this works fine too.

@prigara
Copy link

prigara commented Feb 7, 2017

@joshgav we'd like to add info about WebStorm. Should I create a new PR or can you just add it to this PR? Thanks!

WebStorm 2017.1+ and other JetBrains IDEs
Create a new Node.js debug configuration and hit Debug. --inspect will be used by default for Node.js 7+. To disable uncheck js.debugger.node.use.inspect in the IDE Registry.


Each Node process is marked with its own UUID similar in form to
`0f2c936f-b1cd-4ac9-aab3-f63b0f33d55e`, and debugger clients must specify this
ID to connect to the debugee. The full URL is
Copy link
Contributor

@naugtur naugtur Feb 23, 2017

Choose a reason for hiding this comment

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

totally ignorable hint: debugee might not be understandable for people less fluent in English. They might think it's yet another tool or component.

must specify this UUID to connect to matching process maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I'll find a better term.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't suppose there's a one word term other than debugee.

How about:

Each Node process is marked with its own UUID similar in form to
0f2c936f-b1cd-4ac9-aab3-f63b0f33d55e, and debugger clients must specify this
UUID to Indicate which process to connect to.

Note I also changed ID to UUID because ID is an undeclared reference ;)


## Default ports:

* Inpsector: 9229
Copy link
Member

Choose a reason for hiding this comment

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

Inspector

@joshgav
Copy link
Contributor Author

joshgav commented Feb 24, 2017

Updated:

  • Refactored to move legacy --debug info to its own section at the end and focus on Inspector and --inspect.
  • Added WebStorm info - thanks @prigara!
  • Updated instructions for some clients and did a general edit pass.

IMO this is ready to land. Could those who previously reviewed please take a look and give a GitHub approval or LGTM? Thank you!

@joshgav joshgav added docs and removed diag-agenda labels Feb 24, 2017
@naugtur
Copy link
Contributor

naugtur commented Feb 24, 2017

Somehow after you re-arranged it seems much cleaner/shorter. LGTM, even with the debuggee.

PR-URL: #1131
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@chromium.org>
@joshgav joshgav merged commit b2cdfc8 into nodejs:master Feb 28, 2017
@joshgav
Copy link
Contributor Author

joshgav commented Feb 28, 2017

Landed in b2cdfc8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants