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: update output examples in debugger.md #10944

Closed
wants to merge 1 commit into from
Closed

doc: update output examples in debugger.md #10944

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jan 21, 2017

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

doc, debugger

  • Update examples according to new debugger output (info added, lines reordered).
  • Replace x = 5; by explicit global.x = 5; to stress the intent to create a global variable (needed for access from debug REPL).
  • Add break marks (>) in output according to real output.
  • Update also the Inspector example.

Some updates are done due to previous changes in test/fixtures/break-in-module/main.js and test/fixtures/break-in-module/mod.js files.

@nodejs-github-bot nodejs-github-bot added debugger doc Issues and PRs related to the documentations. labels Jan 21, 2017
@vsemozhetbyt
Copy link
Contributor Author

/cc @nodejs/documentation

@vsemozhetbyt
Copy link
Contributor Author

@jasnell Could this be landed?

@jasnell
Copy link
Member

jasnell commented Jan 31, 2017

I'd like a bit more sign off. @nodejs/documentation

Copy link
Contributor

@joshgav joshgav left a comment

Choose a reason for hiding this comment

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

Two fixes recommended in comments:

  1. var x = 5 rather than global.x = 5
  2. Don't use a specific UUID in the chrome-devtools URL.

Thanks for the contribution!

@@ -203,7 +205,7 @@ $ node --inspect index.js
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/@60cd6e859b9f557d2312f5bf532f6aec5f284980/inspector.html?experiments=true&v8only=true&ws=localhost:9229/node
chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/dc9010dd-f8b8-4ac5-a510-c1a114ec7d29
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfotunately the UUID changes with each Node instance, so we shouldn't use a specific one here. Perhaps a placeholder like <uuid> would be sufficient?

@@ -28,7 +28,7 @@ enable a breakpoint at that position in the code:

```js
// myscript.js
x = 5;
global.x = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

var x = 5 and the same in the output examples would be more representative of usage IMO.

break in /home/indutny/Code/git/indutny/myscript.js:1
1 x = 5;
> 1 global.x = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

var x = 5

break in /home/indutny/Code/git/indutny/myscript.js:1
1 x = 5;
> 1 global.x = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

var x = 5

2 setTimeout(() => {
3 debugger;
debug> cont
< hello
break in /home/indutny/Code/git/indutny/myscript.js:3
1 x = 5;
1 global.x = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

var x = 5

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Feb 2, 2017

@joshgav

  1. I've replaced UUID with the placeholder.
  2. With var x = 5, I get this output:
node debug test.js

< Debugger listening on 127.0.0.1:5858
connecting to 127.0.0.1:5858 ... ok
break in e:\DOC\prg\js\node\-test\test.js:1
> 1 var x = 5;
  2 setTimeout(() => {
  3   debugger;

debug> cont

< hello
break in e:\DOC\prg\js\node\-test\test.js:3
  1 var x = 5;
  2 setTimeout(() => {
> 3   debugger;
  4   console.log('world');
  5 }, 1000);

debug> next

break in e:\DOC\prg\js\node\-test\test.js:4
  2 setTimeout(() => {
  3   debugger;
> 4   console.log('world');
  5 }, 1000);
  6 console.log('hello');

debug> repl

Press Ctrl + C to leave debug repl

> x
ReferenceError: x is not defined

@joshgav
Copy link
Contributor

joshgav commented Feb 3, 2017

@vsemozhetbyt

With var x = 5, I get this output

Ah I see, the example is stopping within a callback so that variable isn't there. I guess global.x = 5 makes sense then and in fact might help people better understand what's happening.

It would be great if you have a better idea than the "<uuid>" string 😄 , but there's not really another option at the moment I don't think?

LGTM

@vsemozhetbyt
Copy link
Contributor Author

@joshgav Sorry, I have not) Does anybody have?

@joyeecheung
Copy link
Member

Maybe use an example uuid string and then add a note/comment explaining that string is just an example, it would be generated on the fly?

@vsemozhetbyt
Copy link
Contributor Author

@joyeecheung Like this?

To start debugging, open the following URL in Chrome:
    chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/dc9010dd-f8b8-4ac5-a510-c1a114ec7d29

(The UUID in the end of the URL is generated on the fly, it will vary.)

@joyeecheung
Copy link
Member

@vsemozhetbyt Probably

(In the example above, the UUID dc9010dd-f8b8-4ac5-a510-c1a114ec7d29 at the end of the URL is generated on the fly, it varies in different debugging sessions)

@vsemozhetbyt
Copy link
Contributor Author

@joyeecheung Done.

@vsemozhetbyt
Copy link
Contributor Author

@jasnell @joshgav @joyeecheung Could we proceed?

@jasnell
Copy link
Member

jasnell commented Feb 9, 2017

Still LGTM. @joyeecheung PTAL

joshgav pushed a commit that referenced this pull request Feb 10, 2017
PR-URL: #10944
Reviewed-By: Josh Gavant <joshgavant@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@joshgav
Copy link
Contributor

joshgav commented Feb 10, 2017

added a period to end of a sentence and landed in ca8c30a

Thanks @vsemozhetbyt!

@joshgav joshgav closed this Feb 10, 2017
@vsemozhetbyt vsemozhetbyt deleted the debugger.md branch February 10, 2017 18:10
@italoacasas
Copy link
Contributor

This is not landing clearly in v7.x, any plans to backport?

krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
PR-URL: nodejs#10944
Reviewed-By: Josh Gavant <joshgavant@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@vsemozhetbyt
Copy link
Contributor Author

@gibfahn Done: #13751

gibfahn pushed a commit that referenced this pull request Jun 17, 2017
PR-URL: #10944
Backport-PR-URL: #13751
Reviewed-By: Josh Gavant <joshgavant@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #10944
Backport-PR-URL: #13751
Reviewed-By: Josh Gavant <joshgavant@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #10944
Backport-PR-URL: #13751
Reviewed-By: Josh Gavant <joshgavant@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants