Bringing node-inspect into the fold

[jkrems]

EDIT: Please refer to nodejs/node#11421 for the latest status.

This issue tracks outstanding questions & tasks that have to be resolved to bring node-inspect into the nodejs org.

• Ensure that the test coverage matches (or exceeds) the suite for node debug
• Link to diagnostic GOVERNANCE document
• Passes node core's eslint config
• TSC approval for project acceptance in Foundation: Bring 'node-inspect' (CLI Debugger) under the Foundation TSC#190
• Initial PR to node: deps: Add node-inspect node#10187
• Support debugging a pid, depends on Debug already running process using v8-inspector? node#8464
• Add deprecation notice to current debugger docs: doc: Deprecate old debug protocol node#10320
• Add node-inspect and more details about --inspect to debugger docs: guides: debugging getting started guide nodejs.org#1131
• Add runtime deprecation to node --debug: lib: deprecate node --debug at runtime node#10970

Context & History

• V8 5.7 removes support for the old protocol
• Issue: Port debugger to new protocol
• node-inspect repository
• Meeting notes of initial diagnostics discussion
• Diagnostics WG discussion at node interactive
• nodejs/CTC issue
• Discussion in CTC meeting

[mhdawson]

one more step to add at the end is to open an issue in the TSC repo requesting the ok for the move.

[mhdawson]

I think the diagnostics WG repo should have a GOVERNANCE.md and CONTRIBUTING.md as all repos should. @joshgav is that something you can look at fixing ?

[joshgav]

Thanks @jkrems! I hope to look over soon and help with questions.

@mhdawson - yes absolutely, will copy from some other repos shortly. Thanks!

[gibfahn]

Regarding using tap (by which I assume you mean https://www.npmjs.com/package/tap), we already use it for the npm tests, and I think there are possibly plans to use it more for other things, so I'm pretty sure that should be fine.

[MylesBorins]

@jkrems as a standalone repo there is no problem with using tap... we use it for citgm :D

[gibfahn]

For linting, I personally like standardjs, but it might be better to keep consistency with the other node repos (so people coming from other areas within the nodejs org don't have to keep track of multiple linting styles).

[jkrems]

I think the current lint settings match what node core is doing for the most part. I couldn't find a published version of the node core config and copy&paste of the file(s) didn't seem like an improvement over the current situation. :)

Updated the implementation, filling some of the last gaps & adding tap tests. node-inspect@1.7.0 is published to npm if you want to play around with it.

[cjihrig]

I couldn't find a published version of the node core config

I know @geek published https://www.npmjs.com/package/node-style. It looks like it's probably stale, but could be updated if you wanted to use it.

[Fishrock123]

shouldn't uh, this just land in core itself if it is to replace node debug?

[jkrems]

@Fishrock123 The feedback I got when I proposed that was:

This is definitely interesting but I'm wondering if it would be better to develop this as a standalone client for the time being. Once it's functional, then we can look to see how much sense it makes to bringing it in to core.

See: nodejs/node-eps#42 (comment)

[ofrobots]

I think this should be in core. Now that V8_inspector debug protocol has landed in V8 directly, the old debug protocol (that has been deprecated for a long while) is likely to be removed soon-ish. This project would be needed in core at that point or we will have to deprecate/remove node debug from core. Might as well put it in core now.

[jkrems]

I'd be happy to unblock anyone who would want to port the current code to node core. The license already is the same (it's a derived work of a node core file) and the code has no external dependencies. The biggest task would be to port the test suite but that should hopefully be relatively straight-forward. :)

[jkrems]

One argument for it landing in node core itself in some shape or form: node 6.9 is the second 6.x release in a row that breaks it (because details of the protocol / expectations changed).

[Fishrock123]

Hmmm ok, read through the other threads. Could you describe a bit what benefit this may have in the foundation?

[jkrems]

@Fishrock123 To clarify your question: Benefit of having an officially supported CLI debugger or benefit of keeping it in its own repository?

I don't really have a good answer for the former (other than "It's the status quo and the people using it might dislike the removal").

If you were asking for "benefits of a separate repo": I think others are better equipped to answer that question. From the transcript it looks like the reason came down to "if there's no good reason to have it in core, don't put it in core":

@mhdawson: Why does this need to be in nodejs/node?

@jkrems: In theory it’s similar to readable-stream and doesn’t require anything in nodejs/node.

@mhdawson: nodereport isn’t in core, but we made it a project in Node.js. Should we do the same in this case?

No opposition.

Having it as a separate repo would not prevent us from bringing it into core in the future. Potentially it would even allow us to have development in a separate repo and roll new releases into core from time to time. That could allow relatively fast development of the CLI debugger under the stewardship of the diagnostics WG with new releases available via npm immediately. The precedent would be readable-stream and npm, both of which ship with core but are also available stand-alone.

I'm going to defer further work until there's agreement on the path forward. :)

[jkrems]

Published node-inspect@v1.9.1 which works with the latest version of node 6, exposes Debugger.* and Runtime.* directly in the repl, and doesn't crash when you run exec console.profile() (but doesn't store the profiles anywhere yet).

[jkrems]

Just gave it a quick try for verification purposes: Integrating the current code into node itself takes 5 lines.

> ./node inspect test/fixtures/empty.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/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/9fcdb074-adfe-4beb-bad6-18948fcf0a87
< Debugger attached.
break in test/fixtures/empty.js:2
  1 (function (exports, require, module, __filename, __dirname) {
> 2 });
debug>

[jkrems]

Let me know if you need anything else from my side. :)

[bnoordhuis]

Is there a CTC or TSC champion for this?

[joshgav]

@bnoordhuis @jkrems I've been reviewing and testing this and can help with logistics and some more review and updates. Still would be ideal to have a CTC member involved.

Although the CLI Debugger was integrated in core in the past, perhaps we should consider decoupling it at this inflection point, and maintaining it in a separate repo. That would allow it to get more attention I think, and make it less risky to add and innovate in it. For example, I've been thinking of adding some sort of "profile" command which could run a brief CPU profile and dump the output.

Also, the CLI debugger could potentially be opened against a web page or other implementor of the Inspector Protocol besides Node.

Even if it isn't in the core repo, we could bundle it with the default distribution.

[Trott]

Talked with a handful of folks at Node Interactive today about this. Apart from the important issue of bringing node-inspect into the nodejs org, is this a correct summary of the status?:

• Current Node.js CLI debugger uses a V8 API that is going away in the foreseeable future.
• Obvious options are to port it to the supported API, remove CLI debugger from core entirely, or replace it with node-inspect.
• Main obstacle to progress at this point is uncertainty as to whether "remove CLI debugger and let people install an external module if they want it" vs. "let's get node-inspect into core" will be the path forward.

[jkrems]

@Trott Happy to chat in person if you're at node interactive as well!

[Trott]

@jkrems Yes, please. That would be great. I'm sitting quietly outside the empty conference rooms right now, if after-parties aren't your thing either. Otherwise, tomorrow some time?

[jkrems]

Same! Going to head down to the 4th floor.

[jkrems]

Updated the issue description to be more reflective of where we're at. @joshgav or @Trott: Are we tracking the overall project ("prepare for --debug removal") somewhere? I know some of it is captured in meeting notes and comments like this one but it might be good to have this is as... a dedicated issue? As part of this issue's TODO checkboxes? A Github project? Had to dig through multiple links & documents to remember half of what we discussed. :)

[Trott]

@jkrems I don't think it's being tracked centrally anywhere. Not sure what the best way to track it would be. I'm inclined to think a checklist in this issue would be workable, but @joshgav may be more plugged in to how to manage these sorts of things effectively, since not-letting-things-slip-through-the-cracks-on-collaborative-projects is part of his actual job (I think).

[bnoordhuis]

I'd open a tracking issue at nodejs/node. It affects core and an issue on the main bug tracker has better visibility (in theory anyway... it can be a black hole sometimes.)

[joshgav]

@jkrems @Trott I did indeed contemplate setting up a GitHub project to gather the various discussions on this topic and can look into it again. In fact though, Jan's summary at the top of this thread is a great start already, perhaps we can use it as the main tracking point; we all should have edit access to it.

[jkrems]

The reason I punted on moving to an issue in nodejs/node is that there already is one with valuable discussion (nodejs/node#7266) so it would feel like creating a duplicate. And for that issue I couldn't make the TODOs as easily visible (not an owner of nodejs/node and also it would feel weird to hijack @ofrobots' issue like that ^^).

[jkrems]

@joshgav Follow-up to the "do we have the old debugger in node 8" question: What would this mean for the kill signal? Would we still change it to start --inspect in node 8? Would we delay the switch until node 9?

[ofrobots]

it would feel weird to hijack @ofrobots' issue like that ^^

It would be good to have a single tracking issue for debug/inspect work. nodejs/node#7266 seems to be pretty stale IMO, so it would be fine to close it and reference it in a new tracking issue on nodejs/node.

From my point of view, from what I have been able to determine from various issues, the next steps are something like the following:

1. Make debug an alias for inspect. This is circularly dependent on the next item. I have started working on this. EDIT: WIP branch here: https://github.com/ofrobots/node/tree/debug-alias. I need to work through all the failing tests (help is most welcome).
2. Switch SIGUSR1 to start inspector. @eugeneo has is working on this and has a WIP branch here: https://github.com/eugeneo/node/commits/inspector-signal. This would be dependent on the old debugger moving out of the way. (Debug already running process using v8-inspector? node#8464). Debugging a pid should just work once these first two are implemented.
3. Add a JS API to allow in-process debuggers to work. This would be logical equivalent of accessing the Debug API from the debug context. (@eugeneo and @cristiancavalli may have some code for this already).
4. node-inspect doesn't currently work well with cluster. The problem is that we don't have a way of communicating the child process' inspect port to node-inspect. process.debugPort currently corresponds to the old debugger and there is no equivalent for inspector. Once we make debug an alias for inspect, this can be fixed.

[jkrems]

Closing in favor of nodejs/node#11421 in nodejs/node.

@ofrobots I tried to incorporate the points you brought up in the updated checklist. Feel free to edit/amend/editorialize. :)

[cristiancavalli]

Too @ofrobots third point (jic anyone's interested) here's the scaffolded prototype implementation of the JS wrapper to the inspector API for in-process communication:
https://github.com/cristiancavalli/in-process-inspector-wrapper

[joshgav]

We should probably have a tracking issue for finishing up Inspector work generally, perhaps we can use the thread @jkrems started?

[jkrems]

@joshgav Happy to turn nodejs/node#11421 into a generic "inspector stuff" tracking issue. In practice it's hard to decouple the two concerns anyhow.