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.

Sign up for GitHub

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

Jump to bottom

doc: add example for running with v8-inspector #8845

Closed
wants to merge 1 commit into from
Closed

doc: add example for running with v8-inspector #8845

wants to merge 1 commit into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Sep 29, 2016

Checklist
  • make -j8 test (UNIX) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc debugger

Description of change

Add example to show what running Node.js with --inspect
should look like.

Some IDEs do not show the link when running with --inspect.
This example hints to what the full output looks like.

@fhinkel fhinkel added the doc Issues and PRs related to the documentations. label Sep 29, 2016
@nodejs-github-bot nodejs-github-bot added debugger doc Issues and PRs related to the documentations. labels Sep 29, 2016
@mscdex mscdex added inspector Issues and PRs related to the V8 inspector protocol and removed debugger labels Sep 29, 2016
addaleax
addaleax approved these changes Sep 29, 2016
View reviewed changes
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.

LGTM, thank you!

doc/api/debugger.md
@@ -193,4 +193,13 @@ e.g. `--inspect=9222` will accept DevTools connections on port 9222.
To break on the first line of the application code, provide the `--debug-brk`
flag in addition to `--inspect`.

```txt
$ ./node --inspect index.js
Copy link
Member

@addaleax addaleax Sep 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I guess “normal” users don’t call it with the ./? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed :)

lpinca
lpinca reviewed Sep 29, 2016
View reviewed changes
doc/api/debugger.md
@@ -193,4 +193,13 @@ e.g. `--inspect=9222` will accept DevTools connections on port 9222.
To break on the first line of the application code, provide the `--debug-brk`
flag in addition to `--inspect`.

```txt
$ ./node --inspect index.js
Copy link
Member

@lpinca lpinca Sep 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: I would just use node without ./. It's unlikely that there is a node executable in the folder where the command is run.

Edit: @addaleax won the race condition here 😄.

lpinca
lpinca reviewed Sep 29, 2016
View reviewed changes
doc/api/debugger.md
Debugger listening on port 9229.
Warning: This is an experimental feature and could change at any time.
To start debugging, open the following URL in Chrome:
chrome-devtools://devtools/remote/serve_file/@60cd6e859b9f557d2312f5bf532f6a
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe it's better to not split this line as it is a URL. I'm not sure though because if it isn't split scrolling is required on small screens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed, no line break in url.

@lpinca
Copy link
Member

lpinca commented Sep 29, 2016

@fhinkel while you are at it s/Add/add/ in the first line of commit message.

@fhinkel
doc: add example for running with v8-inspector
0c406a1 
Add example to show what running Node.js with `--inspect`
should look like.

Some IDEs do not show the link when running with `--inspect`.
This example hints to what the full output looks like.
@fhinkel fhinkel changed the title doc: Add example for running with v8-inspector doc: add example for running with v8-inspector Sep 29, 2016
lpinca
lpinca approved these changes Sep 29, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

cjihrig
cjihrig approved these changes Sep 29, 2016
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe this should go after the first paragraph of the V8 inspector section.

@MylesBorins
Copy link
Contributor

wasn't one of the reasons for not documenting it because it was an experimental feature? /cc @nodejs/ctc

I'm not neccessarily opposed to this, but I do think we should be careful about the support story in experimental features

@cjihrig
Copy link
Contributor

cjihrig commented Sep 29, 2016

It's already documented, and comes with a big bold warning that it's experimental. I don't think this PR adds any additional support commitment.

@MylesBorins
Copy link
Contributor

I retract my original statement then. This example is a great thing to include

LGTM

addaleax
addaleax approved these changes Sep 29, 2016
View reviewed changes
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.

LGTM! :)

@jasnell
Copy link
Member

jasnell commented Sep 30, 2016

LGTM

jasnell pushed a commit that referenced this pull request Sep 30, 2016
@fhinkel @jasnell
doc: add example for running with v8-inspector
bdb8012 
Add example to show what running Node.js with `--inspect`
should look like.

Some IDEs do not show the link when running with `--inspect`.
This example hints to what the full output looks like.

PR-URL: #8845
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 30, 2016

Landed in bdb8012. Thank you!

@jasnell jasnell closed this Sep 30, 2016
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
@fhinkel
doc: add example for running with v8-inspector
3eb93e1 
Add example to show what running Node.js with `--inspect`
should look like.

Some IDEs do not show the link when running with `--inspect`.
This example hints to what the full output looks like.

PR-URL: #8845
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
jasnell pushed a commit that referenced this pull request Oct 6, 2016
@fhinkel @jasnell
doc: add example for running with v8-inspector
13623ce 
Add example to show what running Node.js with `--inspect`
should look like.

Some IDEs do not show the link when running with `--inspect`.
This example hints to what the full output looks like.

PR-URL: #8845
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@fhinkel @lpinca @MylesBorins @cjihrig @jasnell @addaleax @gibfahn @mscdex @nodejs-github-bot