inspector: introduce inspector/promises API

[ErickWendel]

This PR introduces the new inspector/promises API with the session.post function.

Before with Callbacks API:

const { Session } = require('inspector');
const session = new Session();
session.connect();

session.post('Profiler.enable', () => {
  session.post('Profiler.start', () => {
    session.post('Profiler.stop', (err, { profile }) => {
        fs.writeFileSync('./profile.cpuprofile', JSON.stringify(profile));
       session.disconnect();
    });
  });
});

After with Promises:

const { Session } = require('inspector/promises');
const session = new Session();
session.connect();

await session.post('Profiler.enable');
await session.post('Profiler.start');
 
const { profile } = await session.post('Profiler.stop')
fs.writeFileSync('./profile.cpuprofile', JSON.stringify(profile));

session.disconnect();

[ErickWendel]

Perhaps we should expose node:inspect/promises? Just for the sake of consistency

I thought about it, but the only function that makes sense using promises is the post function. When looking at the module there’re a lot of dependencies needed to be rewritten in case we wanna expose it as /promises

[Trott]

@nodejs/inspector

[benjamingr]

Overall big +1 on the idea

When looking at the module there’re a lot of dependencies needed to be rewritten in case we wanna expose it as /promises

Why? It can even live in the same file it'd just have to be exported as /promises.

I'm not sure why this should be the one and only API that works with "omit callback to get promise back" in Node.js (this specific approach was rejected for fs for example). Namely this would mean that someone forgetting to pass a callback would now stop getting an error and would get an unhandled rejection if the .post failed instead since they might be unaware of the new return value.

[ErickWendel]

Overall big +1 on the idea

When looking at the module there’re a lot of dependencies needed to be rewritten in case we wanna expose it as /promises

Why? It can even live in the same file it'd just have to be exported as /promises.

I'm not sure why this should be the one and only API that works with "omit callback to get promise back" in Node.js (this specific approach was rejected for fs for example). Namely this would mean that someone forgetting to pass a callback would now stop getting an error and would get an unhandled rejection if the .post failed instead since they might be unaware of the new return value.

I struggled a bit but I was able to do it o/

[ErickWendel]

I think I'll need a deep review of the docs as I'm not sure if I'm on the right path. I copied and pasted the whole API, changed the topics, and added sections for callbacks and promises APIs following the example seeing on fs

[benjamingr]

Good progress!

[MoLow]

LGTM, nice addition

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47248/

[ovflowd]

Just adding a blocker for docs 👀 as it doesn't follow the correct standard.

[ovflowd]

Doc-wise, this should be somewhere else, And marked with `> Stability 1 Experimental.

E.g.:

## Promises

<!-- introduced_in=v19.0.0 -->

> Stability: 1 - Experimental

<!-- source_link=lib/inspector/promises.js -->

[...] Description.

IMHO this is a doc-blocker.

[aduh95]

It is marked as experimental, see https://github.com/nodejs/node/pull/44250/files#diff-d6da7e100224e744520fde97ca99f743ad9eafc83d50ed92ed2b7870f0589f68R34

<!-- introduced_in=v19.0.0 --> would not be appropriate here, those are to reference the first version that included the markdown document in the docs – which is v8.0.0 for inspector.md. This PR does contain a YAML comment block that indicates when the promise API was (will be) introduced: https://github.com/nodejs/node/pull/44250/files#diff-d6da7e100224e744520fde97ca99f743ad9eafc83d50ed92ed2b7870f0589f68R37.

I disagree this is a doc blocker, also doc can always be fixed in a semver patch, while the module can only land in a semver-major.

[ovflowd]

Not saying this is a PR blocker, but felt for me like a doc blocker, if you disagree that's fine, I don't want to block this PR too much 👀

[ErickWendel]

I didn't get whether i should change it or not haha. Is it ready to be landed? cc @aduh95 @ovflowd

[aduh95]

If we push one more commit to that branch, the PR is extremely unlikely to land on v19.x and would need to wait for 20.x, so I'd say it's probably not completely ready, but that's fine because we land it as experimental.
I'm planing on landing it as soon as we get a passing CI.

[ErickWendel]

Perfect! @aduh95.

for the next PR @ovflowd I didn't get what you suggested to change. I plan to:

change connect and disconnect functions to be async (this might lead me to change something in cpp to get those events (no idea how to do it haha)) and what you suggested adding here.

[ovflowd]

Hmm, @ErickWendel not sure, but I was just making suggestions to the API docs you were writing xD

But it's all fine and this landed 🎉

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47257/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47265/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47266/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/47267/

[nodejs-github-bot]

Landed in 0324529