Skip to content

doc: add code example to inspector.url() method - #29496

Closed
juanarbol wants to merge 5 commits into
nodejs:masterfrom
juanarbol:inspect-docs
Closed

doc: add code example to inspector.url() method#29496
juanarbol wants to merge 5 commits into
nodejs:masterfrom
juanarbol:inspect-docs

Conversation

@juanarbol

Copy link
Copy Markdown
Member
Checklist

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. inspector Issues and PRs related to the V8 inspector protocol labels Sep 8, 2019
Comment threaddoc/api/inspector.md Outdated
Comment threaddoc/api/inspector.md Outdated
Comment threaddoc/api/inspector.md Outdated
Comment threaddoc/api/inspector.md Outdated
Comment threaddoc/api/inspector.md Outdated
Comment threaddoc/api/inspector.md Outdated
Comment threaddoc/api/inspector.md Outdated
@juanarbol

Copy link
Copy Markdown
MemberAuthor

ping @Trott

@Trott

Copy link
Copy Markdown
Member

@TrottTrott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 25, 2019
@Trott

Copy link
Copy Markdown
Member

@nodejs/documentation How do we feel about this sample code? It's not really a runnable code sample. It's really just "Here's what the output would be with three completely different invocations of Node.js." I'm OK with this because it's probably still an improvement over the current doc. But there's probably an even better way to show this that doesn't suggest that the value can change within a single process when it won't?

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 25, 2019
@juanarbol

juanarbol commented Sep 25, 2019

Copy link
Copy Markdown
MemberAuthor

@Trott I'm not part of documentation working group, but I could change that JS sintax to Shell, so instead of:

// Node.js process was called with --inspect flag.require('inspector').url()
Debugger listeningonws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72Forhelpsee https://nodejs.org/en/docs/inspector
ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72

Could be more like:

$ node --inspect -p "require('inspector').url()"Debugger listening on ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72For help see https://nodejs.org/en/docs/inspectorws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72

@Trott

Copy link
Copy Markdown
Member

@Trott I'm not part of documentation working group, but I could change that JS sintax to Shell, so instead of:

// Node.js process was called with --inspect flag.require('inspector').url()
Debugger listeningonws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72Forhelpsee https://nodejs.org/en/docs/inspector
ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72

Could be more like:

$ node --inspect -p "require('inspector').url()"
Debugger listening on ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72
For help see https://nodejs.org/en/docs/inspector
ws://127.0.0.1:9229/136e8c71-8901-43d8-b666-1d74a161cd72

I could see that being a good option, especially if the commands in question work verbatim on Windows.

@juanarbol

Copy link
Copy Markdown
MemberAuthor

I'm not a Windows expert, but, I'd love to see the @nodejs/documentation opinions before making a new change, but yeah! I can change this for making this better.

@Trott

Copy link
Copy Markdown
Member

I'm not a Windows expert, but, I'd love to see the @nodejs/documentation opinions before making a new change, but yeah! I can change this for making this better.

I'm not a Windows expert, but I think those commands would work on Windows too. @nodejs/platform-windows

@Trott

This comment has been minimized.

Comment threaddoc/api/inspector.md Outdated
@TrottTrott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 27, 2019
@juanarbol

Copy link
Copy Markdown
MemberAuthor

I found something: "Return the URL of the active inspector, or undefined if there is none.", and by the way, In default options, I see that, by default, debugger is 127.0.0.1:9229, maybe, maybe... say that could improves documentation:

"Return the URL of the active inspector, 127.0.0.1:9229 by default, or undefined if there is none." With better sintax, of course.

@juanarbol

Copy link
Copy Markdown
MemberAuthor

Pong @Trott

Comment threaddoc/api/inspector.md Outdated
Return the URL of the active inspector, or `undefined` if there is none.

```console
$ node --inspect -p "require('inspector').url()"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since built-in modules are automatically loaded when Node.js is run this way, we could consider simplifying to something like this here and below?:

Suggested change
$ node --inspect -p "require('inspector').url()"
$ node --inspect -p 'inspector.url()'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And adding to the suggested change above, we can make the output easier to read by making it clear what part of the output is from inspector.url() since there are a couple other lines of output that might confuse the reader?:

Suggested change
$ node --inspect -p "require('inspector').url()"
$ node --inspect -p '`\ninspector.url() result: ${inspector.url()}\n`'```

That makes the command line a little bit harder to read, but the output easier to read, so...
¯\(ツ)

@Trott

Trott commented Oct 3, 2019

Copy link
Copy Markdown
Member

My comments above are suggestions only. If someone wants to run this on CI and land it as it is right now, I'm fine with that.

@juanarbol

Copy link
Copy Markdown
MemberAuthor

Ping @Trott, I think this is ready, is it?

@BridgeAR
BridgeAR requested a review from TrottDecember 9, 2019 22:47
@nodejs-github-bot

nodejs-github-bot commented Dec 9, 2019

Copy link
Copy Markdown
Collaborator

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 9, 2019
@targos

Copy link
Copy Markdown
Member

Landed in 4113601

@targostargos closed this Dec 11, 2019
targos pushed a commit that referenced this pull request Dec 11, 2019
PR-URL: #29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 13, 2019
PR-URL: #29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorinsMylesBorins mentioned this pull request Dec 13, 2019
targos pushed a commit that referenced this pull request Jan 14, 2020
PR-URL: #29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targostargos mentioned this pull request Jan 15, 2020
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #29496
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorinsMylesBorins mentioned this pull request Feb 8, 2020
@juanarbol
juanarbol deleted the inspect-docs branch January 19, 2021 16:24
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.inspectorIssues and PRs related to the V8 inspector protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@juanarbol@Trott@nodejs-github-bot@targos@lpinca@cjihrig@gireeshpunathil@BridgeAR