Move node-inspect into core?

[jkrems]

Hello everybody,

When node-inspect was created, the theory was that maintaining it inside of core would add process overhead. What actually happened (partially because I struggled to find time) is that being in a separate repo just seems to slow things down. Reviews happen slowly and even after merge I often get around to creating the upstream PR only weeks later.

So I'm wondering: Would it be worth archiving nodejs/node-inspect and moving development into core? The code is already written so that it should slit nicely into the lib/internal tree with minor adjustments, the biggest chunk of work would be porting the tests that currently use tap.

[mmarchini]

cc @nodejs/diagnostics

even after merge I often get around to creating the upstream PR only weeks later.

We could probably solve this with an Action that opens PRs upstream once things land here, although it doesn't solve the review problem.

I'm also not sure if moving node-inspect into core will fix the review problem. If no one is looking at PRs for node-inspect, we might end with the same problem of unreviewed PRs on nodejs/node. Wondering if we should have a call for maintainers instead.

Besides review and upstream PRs, are there any other benefits merging this repo with core?

[jkrems]

Besides review and upstream PRs, are there any other benefits merging this repo with core?

The others could be:

1. Confusion. People find node-inspect and think it's where they should discuss issues with --inspect.
2. Code drift. We had multiple times where node core became incompatible with node-inspect and we only realized much later.

I would also say that since node-inspect isn't reliably published to npm and is basicaly unused in its stand-alone form, it's effectively just a couple of "random" files that are typically only used from inside of node but have their source of truth outside of node. Which seems awkward.

[Trott]

Two inter-related problems this would solve, I think:

1. People opening PRs in the node repo that need to be redirected to node-inspect. Ex: doc: Revise Github to GitHub #36359
2. Node.js PRs stalling while waiting for node-inspect to catch up. Ex: doc: update message to match actual output #35271

[Trott]

I'm in favor of this change, especially if it makes things easier for @jkrems.

[joyeecheung]

SGTM

[mhdawson]

SGTM

[mmarchini]

Overall SGTM. I added to diag-agenda since it falls under the Diagnostics WG charter.

[trraney328]

but how is this gonna work?

[mmarchini]

Can you elaborate your question?

[mmarchini]

After discussion in the Diagnostics WG, we believe moving the codebase to nodejs/node is the best path moving forward. @jkrems do you plan on driving this work? Or should we try to find a volunteer?

Here's a list of things we need to do to consider this done:

• Move node-inspect from deps to lib
• Decide what to do with the npm module
• Triage open issues on nodejs/node-inspect and move any relevant issues to nodejs/node
• Triage open PRs on nodejs/node-inspect and move any relevant PR to nodejs/node
• Archive the nodejs/node-inspect repostory

(if I missed anything reply to this commend and I'll add to the list)

cc @nodejs/tsc for awareness, we'll be archiving the nodejs/node-inspect repository and will merge the code into nodejs/node

[jkrems]

@jkrems do you plan on driving this work? Or should we try to find a volunteer?

I currently don't have a firm timeline for when I'd be able to do it. So if somebody else steps forward, it would likely speed things up.

That list looks good! I'm not sure I follow step 2 though. Afaik there's no (public) API that can be imported. So maybe the item should be to rather deprecate the package on npm?

[mmarchini]

With that being said, deprecating the package might be enough. I changed 2 to "Decide what to to with the npm module"

[jkrems]

Gotcha. I don't think it was really meant to be a public API and it's not documented. So I personally wouldn't want to support it in core where we can effectively hide modules in internal.

[mmarchini]

Gotcha. I don't think it was really meant to be a public API and it's not documented. So I personally wouldn't want to support it in core where we can effectively hide modules in internal.

I do see value in making it a public API, but that could be a second step. Worth noting that making the API internal is semver-major on Node.js.

[jkrems]

Worth noting that making the API internal is semver-major on Node.js.

Is that API exposed inside of nodejs? I didn't think it was possible to require the bundled version directly, outside of running node inspect? It's currently bundled as internal/deps/node-inspect/lib/_inspect which I wouldn't expect to resolve from user code.

[mmarchini]

oh, maybe I missed that. If the API is not exposed inside Node.js it should be fine.

[Trott]

I started the work of moving node-inspect into core last night. At this point, I think all that remains to be done is to go through the tests and get them working in core's test setup. (I've done it for two of the tests already. It's pretty straightforward.) Oh, that and a bunch of linting issues. (Our linter wants the code to switch to primordials, and things like that.)

Not sure when I'll have time to work on it again, but it shouldn't be too long before a PR is ready. I might open a draft PR soon just so people can comment on anything I might have Already Done Wrong.

[Trott]

Oh, hey, getting the tests working made me fix an incompatibility with Node.js 15.x. I guess we really do want this stuff in core and running on CI all the time. :-D

[Trott]

A wiser person than I am might wait to see if the tests pass in CI before posting this here, but uh, here's the PR: #38161

[richardlau]

Does this mean we can remove https://ci.nodejs.org/job/node-inspect/ ?

[Trott]

Does this mean we can remove https://ci.nodejs.org/job/node-inspect/ ?

I would think so, yes.

[jkrems]

I don't think we plan to land any more changes there. +1 for cleaning up the job.

[Trott]

I think the next step at this point is to deprecate the npm module. There seems to be consensus to do that here, but is that enough to take the action?

[mhdawson]

I think if we include the comment that it has been part of core for quite some time, and that is is now being maintained there, deprecating is simply reflecting the reality that there will not be another release (the has not been one for over a hear as well).

We should possibly have some governance in terms of how we deprecate modules maintained with the project. It might make sense to have an issue in the TSC repo, mostly as an FYI, last chance for objections.

[Trott]

I don't have publication access to node-inspect but if someone who does (/ping @jkrems!) could run this command, that would be great and we could move on to the next step.

npm deprecate node-inspect 'This module is part of Node.js core and does not need to be installed separately. It is now unmaintained.'

[jkrems]

$ npm show node-inspect
# ...

DEPRECATED ⚠️  - This module is part of Node.js core and does not need to be installed separately. It is now unmaintained.

I double-checked that undoing a deprecation is a thing. So if anybody objects, it's quick to fix this.

[Trott]

It also means we can update the message if the current one gives the wrong impression that we've moved it into Node.js core and its unmaintained there or something weird like that. :-D

[Trott]

Next step is to triage open issues in the node-inspect repo and move any relevant ones to the main repo. I'll get to that soon-ish I hope, but if someone else wants to do it, don't let me stop you!

[jkrems]

I went through the issues and they are all either closed or transferred to the main repo now. Pressing the big red button to archive now!

P.S.: If we want to amend anything, we can temporarily un-archive. But archiving now prevents backsliding in the issues.