Bringing node-inspect into the fold

[hybrist]

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).

[hybrist]

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?

[hybrist]

@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.

[hybrist]

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. :)

[hybrist]

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?