Repository navigation
Bringing node-inspect into the fold #67
Description
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-inspectand more details about--inspectto debugger docs: guides: debugging getting started guide nodejs.org#1131 - Add runtime deprecation to
node --debug: lib: deprecate node --debug at runtime node#10970
Activity
mhdawson commentedon Oct 11, 2016
one more step to add at the end is to open an issue in the TSC repo requesting the ok for the move.
mhdawson commentedon Oct 11, 2016
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 commentedon Oct 11, 2016
Thanks @jkrems! I hope to look over soon and help with questions.
@mhdawson - yes absolutely, will copy from some other repos shortly. Thanks!
gibfahn commentedon Oct 11, 2016
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 commentedon Oct 11, 2016
@jkrems as a standalone repo there is no problem with using tap... we use it for citgm :D
gibfahn commentedon Oct 11, 2016
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 commentedon Oct 16, 2016
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
taptests.node-inspect@1.7.0is published to npm if you want to play around with it.cjihrig commentedon Oct 16, 2016
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 commentedon Oct 17, 2016
shouldn't uh, this just land in core itself if it is to replace
node debug?hybrist commentedon Oct 17, 2016
@Fishrock123 The feedback I got when I proposed that was:
See: nodejs/node-eps#42 (comment)
ofrobots commentedon Oct 17, 2016
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 debugfrom core. Might as well put it in core now.hybrist commentedon Oct 17, 2016
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 commentedon Oct 21, 2016
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 commentedon Oct 22, 2016
Hmmm ok, read through the other threads. Could you describe a bit what benefit this may have in the foundation?
hybrist commentedon Oct 24, 2016
@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":
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-streamandnpm, 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. :)
7 remaining items
Trott commentedon Nov 30, 2016
@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?
hybrist commentedon Nov 30, 2016
Same! Going to head down to the 4th floor.
hybrist commentedon Dec 13, 2016
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 commentedon Dec 13, 2016
@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 commentedon Dec 13, 2016
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 commentedon Jan 3, 2017
@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.
hybrist commentedon Jan 3, 2017
The reason I punted on moving to an issue in
nodejs/nodeis 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 ofnodejs/nodeand also it would feel weird to hijack @ofrobots' issue like that ^^).hybrist commentedon Jan 23, 2017
@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
--inspectin node 8? Would we delay the switch until node 9?ofrobots commentedon Feb 16, 2017
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:
circularlydependent 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).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.process.debugPortcurrently corresponds to the old debugger and there is no equivalent for inspector. Once we make debug an alias for inspect, this can be fixed.hybrist commentedon Feb 16, 2017
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 commentedon Feb 16, 2017
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 commentedon Feb 16, 2017
We should probably have a tracking issue for finishing up Inspector work generally, perhaps we can use the thread @jkrems started?
hybrist commentedon Feb 17, 2017
@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.