Skip to content
This repository was archived by the owner on Feb 1, 2022. It is now read-only.

Turn inspect_client.js into a public API #81

Closed
mmarchini opened this issue Apr 4, 2020 · 5 comments
Closed

Turn inspect_client.js into a public API #81

mmarchini opened this issue Apr 4, 2020 · 5 comments

Comments

@mmarchini
Copy link

Even though the API is already accessible today, it's under internal/, so it shouldn't be used by dependencies. I think it makes sense to expose this API though, as it is useful to fulfill the use case described here. As an example, here's a short script that allows users to connect to a remote process and take a CPU Profile:

#!/usr/bin/env node
'use strict';

const InspectorClient = require('node-inspect/lib/internal/inspect_client.js');

async function main() {
  try {
    const client = new InspectorClient();
    await client.connect(9229, 'localhost');

    await client.callMethod("Profiler.enable");
    await client.callMethod("Profiler.start");
    await new Promise((res) => setTimeout(res, 1000));
    const { profile } = await client.callMethod("Profiler.stop");
    await client.callMethod("Profiler.disable");
    await new Promise((res) => process.stdout.write(JSON.stringify(profile), res));
    client.reset();
  } catch (e) {
    console.error(e);
    process.exit(1);
  }
}

main();

This is the easier way to non-interactively run inspector methods on a remote Node.js instance. The only alternative available is to use a Websocket client such as ws or websocat (and both offer subpar experience with the inspector protocol compared to this API), since the Inspector API available on core doesn't allow users to connect to a remote process.

It's also worth noting the API is fairly stable, there has been no substantial changes to that file since 2016.

@jkrems
Copy link
Collaborator

jkrems commented Apr 4, 2020

Just in case: Are you aware of https://github.com/cyrus-and/chrome-remote-interface and other more general libraries? The code in this project is fairly minimal by design (since it has to be bundled in the node binary) so any API exposed will never be as nice as the existing packages for accessing the debugger protocol.

@mmarchini
Copy link
Author

Yes, if you see the use case I described above, one of the requirements is no dependency on external libraries, and node-inspect fulfills that requirement. Another requirement is that we have a way to trigger common production diagnostic tasks with core only (the reasoning is explained in the issue), and having this public API would make it easier to implement those triggers on core. IMO the fact that this is a minimal implement is a strength, not an issue.

The alternative to fulfill those requirements is adding websockets to core, which I'm trying to avoid.

@jkrems
Copy link
Collaborator

jkrems commented Apr 6, 2020

Ah, interesting. From the code example I thought you meant the public interface of the npm package, not a new public interface inside of node. Given the lack of active maintenance of this module[1], I don't think adding a non-trivial public interface that ships with core would be a great idea. If somebody steps up and says "yes, I want to own this and I have people who will review changes to it!" the situation would be different. But as-is this feels like something that either needs to live in the existing built-in module (and could be used by node-inspect) or should be its own 3rd party package. And in that last case, there seems to be plenty of great packages to choose from already.

@mmarchini
Copy link
Author

From the code example I thought you meant the public interface of the npm package, not a new public interface inside of node

The idea is to start on the package, and eventually move to core once it proves stable and useful.

Given the lack of active maintenance of this module[1], I don't think adding a non-trivial public interface that ships with core would be a great idea. If somebody steps up and says "yes, I want to own this and I have people who will review changes to it!" the situation would be different.

Totally understandable. I could own this but I don't have people who will review it.

But as-is this feels like something that either needs to live in the existing built-in module (and could be used by node-inspect) or should be its own 3rd party package

Yes, long term that's what I'm thinking. But short term I would like to test it as a npm module, like we've done for other features in the past.

And in that last case, there seems to be plenty of great packages to choose from already.

Sure, but then again, that doesn't meet the requirement of "being able to communicate with the inspector protocol with scripts without depending on any npm package" (neither does having the API on node-inspect, but that's more acceptable since node-inspect is a foundation project, doesn't have any dependencies and is already in core).

I'll investigate some options from here. I might either copy that file or require it anyway on a CLI tooling I have in mind to trigger profilers and snapshots on remote Node.js instances.

@mmarchini
Copy link
Author

Closing for now as I implemented a CLI without using node-inspect. I might bring this up again in the future though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants