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

src: update --inspect hint text #11207

Closed
wants to merge 2 commits into from

Conversation

@joshgav
Copy link
Member

commented Feb 6, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

src, inspector


  • Removes "experimental" warning.
  • Prints ws://_ip_:_port_:/_uuid_ for all IDs.
  • Refers to nodejs.org guide for more details.

Output is:

Debugger listening on ws://127.0.0.1:9229/11cb1a9c-5994-4a83-a681-f4213b0fdd12
For help see https://nodejs.org/en/docs/inspector

/cc @nodejs/diagnostics

src/inspector_socket_server.cc Outdated

fprintf(out, "\nFor more info go to %s\n\n", GUIDE_URL);

fprintf(out, MapsToString(this->GetListResponse()).c_str());

This comment has been minimized.

Copy link
@joyeecheung

joyeecheung Feb 7, 2017

Member

Maybe just fprintf(out, "http://%s:%d/json/list", this->host().c_str(), this->port()); instead of printing the whole response?

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2017

This seems noisy (both the output and the diff). Users almost certainly will never care about the array at the end. Couldn't you just add the piece that links to the debugging guide?

@joshgav

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

@cjihrig

Couldn't you just add the piece that links to the debugging guide?

Sure, my goal in the initial commit was to share a few options for review. A tricky part is that users may need the UUID or full ws://... URL if connecting manually.

@mavericken

This comment has been minimized.

Copy link

commented Feb 9, 2017

I think we should at least keep both the the documentation link and the chrome URL. Also, we should err on the side of providing more help, even if it is at the cost of being too verbose.

I have an idea I am reluctant to mention because I feel it would be overkill, but I'll do it anyways. If we want to eliminate noise and be helpful, a solution would be to link to a single HTML page ( perhaps with URL parameters) which displays any dynamic links or configurations for various tools.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

One observation I made was that people seem to be unaware of the automatic links in chrome://inspect and aren't that happy with having to copy&paste the URL (see the advent of chrome extensions that duplicate chrome://inspect). So I'm not sure printing the chrome-devtools:// url is doing anyone any favors. It seems to funnel people into a bad workflow.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2017

Do we want to update this PR and remove the experimental warnings or would that require another PR?

@joshgav

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2017

@jkrems I'll update this one in next couple days, thanks!

@joshgav joshgav force-pushed the joshgav:dash-dash-inspect-hint branch Apr 5, 2017

@joshgav

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2017

@jkrems @ofrobots @eugeneo updated to an official proposal, PTAL.

This is per next steps in Diag WG meeting: https://github.com/nodejs/diagnostics/blob/master/wg-meetings/2017-03-23.md#wip-inspector-hint-text-update-11207

This removes the chrome-devtools:// URL and only provides a ws://... URL. The copy-pastability of the chrome-devtools URL has been raised in the above meeting; would it be possible for chrome://inspect to accept the ws://... URL and transform it to the chrome-devtools URL? That would actually make copy-pasting easier since the URL is shorter.

Thanks!

@joshgav joshgav changed the title [WIP] inspector: hint text update src: update --inspect hint text Apr 5, 2017

@eugeneo

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2017

Chrome DevTools are looking at different options to help the users start debugging the Node (e.g. shorter/bookmarkable URL, easier discovery, etc) but there is no immediate solution that would be more convenient to copying the URL from the console to the browser address bar.

Even to copy/pasting the WS URL to chrome://inspect requires the users to read the guide and remember "chrome://inspect" URL itself. It is an extra step whenever the Node is restarted - and judging from the UUID introduction, this will be even bigger pain.

Removing the URL at this time will definitely break the workflow of many users. I believe it should be kept it a smoother workflow is implemented and is deployed in Chrome stable...

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

I think it would be helpful to discuss and gather thoughts on this in next week's CTC meeting, adding the label.

@joshgav Could you be specific about what you want CTC input on?

I personally don't feel qualified to weigh in on how --inspect is being used by end users and I defer to the judgment of people like you, @jkrems, and @eugeneo.

But if there is something narrow and specific, I'm happy to try to educate myself to be able to participate meaningfully.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

My personal vote would be for the ws://<host>:<ip>/<uuid> format that is implemented here. "Open this URL in Chrome" isn't a great experience past the very first interaction. I would be surprised if many users are using that flow and are remotely happy with it. From what I'm aware people are either migrating to Chrome extensions, use IDEs like vscode to start the node process, or go for chrome://inspect. All of these are reasonable, convenient workflows but none of them really needs a full DevTools URL printed to stderr.

(*) I have a particular bias against the DevTools URL because I keep hearing people complain about having to copy and paste the URL on every restart. They think it's the "normal"/"suggested"/"official" workflow and so they don't check the docs for better ways. I'd rather not keep re-posting the same answers to even more developers... ;)

@joshgav

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2017

@Trott @nodejs/ctc the following is where your opinions would be helpful, thanks! I think you can reflect these via approvals to this PR and/or rejections and comments in this thread. I tagged ctc-agenda cause I think a brief live discussion would be helpful. There's a possibility I might miss the meeting today cause of travel 😞 (will be back more consistently starting next week!), @jkrems any possibility you could attend the CTC meeting today as well to help explain if needed? Thanks!

Without further ado 😉 here's that explanation:

The current hint text (in master) is:

Debugger listening on 127.0.0.1:9229.
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/eff03f13-55c9-4fef-8bf4-f938376ac9d6

The hint text proposed in this PR is:

Debugger listening on ws://127.0.0.1:9229/11cb1a9c-5994-4a83-a681-f4213b0fdd12
For help see https://nodejs.org/en/docs/guides/debugging_getting_started

The main questions to be reviewed by the CTC are:

  1. Whether or not to include To start debugging...chrome-devtools://... in the hint text or remove it.
  2. Whether to provide the entire ws://host:port/uuid URL or just host:port in the first line.

My opinion is that the message proposed in this PR is easier to read and understand, mostly avoids the >80 column overflow with the long chrome-devtools URL, (see e.g. #7141) and provides enough information to get started without being overly verbose.

Although the extra For help see... line isn't strictly necessary, I think it aligns with our spirit of being as helpful as possible to our users. It also helps address usability concerns due to removal of the DevTools URL. Finally, it's easier to update that doc for changes in clients rather than introduce changes here in the runtime (e.g. if Chrome DevTools itself introduces a new syntax).

Removing the chrome-devtools:// text will also reduce the somewhat incorrect association of Node/V8's Inspector with Chrome DevTools and other tools, reflected in issues such as #11591, #10410, #9382, and #8438; and as alluded to by @jkrems above (comment).

Including the UUID in the default output will also be helpful to some users, see #11496, #11701, #9185, and #10578. For the same reason prepending the scheme (i.e. ws) makes it more obvious that WebSocket rather than HTTP is the protocol in use.


Since Inspector is just now emerging from experimental status this change isn't yet semver-major, but I think it would be after 8.0.0, so want to land this before the final release, one of the reasons I tagged ctc-agenda.

Thanks for your help!

@joshgav joshgav force-pushed the joshgav:dash-dash-inspect-hint branch Apr 19, 2017

@joshgav

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2017

Cherry-picked update to node-inspect landed in #12363.

New CI: https://ci.nodejs.org/job/node-test-pull-request/7516/

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

I'm +1 on the proposed changed hint text in this PR.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

any possibility you could attend the CTC meeting today as well to help explain if needed?

@joshgav I could join for the first couple of minutes but have some conflicting meetings. With the delay of the release (and some CTC members already chiming in here) it seems less urgent..?

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

Maybe we can get https://nodejs.org/en/inspector or similar rather than https://nodejs.org/en/docs/guides/debugging_getting_started?

@ofrobots

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

Maybe we can get https://nodejs.org/en/inspector or similar rather than https://nodejs.org/en/docs/guides/debugging_getting_started?

+1. The debugging_getting_started guide indeed seem to be a big wall of text. A different doc with very clear links to how to open various different debuggers (VSCode, DevTools, etc.) would be a lot more usable.

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

From the @nodejs/website perspective, https://nodejs.org/en/docs/inspector might be slightly better information architecture than https://nodejs.org/en/inspector. Either is 👍 by me, FWIW.

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

I'm +1 for something like https://nodejs.org/en/docs/inspector. Seems better than the guide.

joshgav added 2 commits Apr 5, 2017
src: update --inspect hint text
* Removes "experimental" warning.
* Prints ws://_ip_:_port_:/_uuid_ for all IDs.
* Refers to nodejs.org guide for more details.

PR-URL: #11207
Reviewed-By: _tbd_
Reviewed-By: _tbd_

@joshgav joshgav force-pushed the joshgav:dash-dash-inspect-hint branch to 5c9335a May 3, 2017

@joshgav

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

Removed lots of text from the doc per @ofrobots' feedback and moved to https://nodejs.org/en/docs/inspector in nodejs/nodejs.org#1216, PTAL.

Updated link in this PR too.

I'll land this once the updated doc lands in nodejs.org. If any are still opposed please comment now!

@joshgav

This comment has been minimized.

Copy link
Member Author

commented May 4, 2017

Thanks all. This PR landed in 6ade7f3.

The docs PR landed in nodejs/nodejs.org@13eaf4c

@joshgav joshgav closed this May 4, 2017

joshgav added a commit that referenced this pull request May 4, 2017
src: update --inspect hint text
* Removes "experimental" warning.
* Prints ws://_ip_:_port_:/_uuid_ for all IDs.
* Refers to nodejs.org guide for more details.

PR-URL: #11207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <mhdawson@ibm.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
joshgav referenced this pull request in nodejs/nodejs.org May 4, 2017
docs: move and update inspector help
PR-URL: #1216
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
@refack refack referenced this pull request May 5, 2017
3 of 3 tasks complete
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
src: update --inspect hint text
* Removes "experimental" warning.
* Prints ws://_ip_:_port_:/_uuid_ for all IDs.
* Refers to nodejs.org guide for more details.

PR-URL: nodejs#11207
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <mhdawson@ibm.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@jasnell jasnell referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.