inspector: introduce inspector/promises API by ErickWendel · Pull Request #44250 · nodejs/node · GitHub

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();

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

@nodejs/inspector

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.

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/

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

Good progress!

LGTM, nice addition

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

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

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.

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

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.

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 👀

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

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.

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.

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 🎉

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

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

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

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

Landed in 0324529

Notable Changes: doc: * graduate webcrypto to stable (Filip Skokan) #44897 esm: * remove specifier resolution flag (Geoffrey Booth) #44859 http: * (SEMVER-MAJOR) use Keep-Alive by default in global agents (Paolo Insogna) #43522 build: * (SEMVER-MAJOR) remove dtrace & etw support (Ben Noordhuis) #43652 * (SEMVER-MAJOR) remove systemtap support (Ben Noordhuis) #43651 deps: * (SEMVER-MAJOR) deps: update V8 to 10.7.193.13 (Michaël Zasso) #44741 Deprecation and Removals: * deprecate url.parse() (Rich Trott) #44919 * (SEMVER-MAJOR) runtime deprecate exports double slash maps (Guy Bedford) #44495 * (SEMVER-MAJOR) runtime deprecate coercion to integer in `process.exit()` (Daeyeon Jeong) #44711 Semver-Major Commits: * [53f73d1] - (SEMVER-MAJOR) build: enable V8's trap handler on Windows (Michaël Zasso) #44741 * [06aaf8a] - (SEMVER-MAJOR) build: reset embedder string to "-node.0" (Michaël Zasso) #44741 * [aa3a572] - (SEMVER-MAJOR) build: remove dtrace & etw support (Ben Noordhuis) #43652 * [38f1e27] - (SEMVER-MAJOR) build: remove systemtap support (Ben Noordhuis) #43651 * [2849283] - (SEMVER-MAJOR) crypto: remove non-standard `webcrypto.Crypto.prototype.CryptoKey` (Antoine du Hamel) #42083 * [a1653ac] - (SEMVER-MAJOR) crypto: do not allow to call setFips from the worker thread (Sergey Petushkov) #43624 * [fd36a8d] - (SEMVER-MAJOR) deps: update llhttp to 8.1.0 (Paolo Insogna) #44967 * [89ecddd] - (SEMVER-MAJOR) deps: bump minimum ICU version to 71 (Michaël Zasso) #44741 * [66fe446] - (SEMVER-MAJOR) deps: V8: cherry-pick 0cccb6f27d78 (Michaël Zasso) #44741 * [88ed027] - (SEMVER-MAJOR) deps: V8: cherry-pick 7ddb8399f9f1 (Michaël Zasso) #44741 * [26c651c] - (SEMVER-MAJOR) deps: V8: cherry-pick 1b3a4f0c34a1 (Michaël Zasso) #44741 * [c8ff2df] - (SEMVER-MAJOR) deps: V8: cherry-pick b161a0823165 (Michaël Zasso) #44741 * [7a8fa2d] - (SEMVER-MAJOR) deps: fix V8 build on Windows with MSVC (Michaël Zasso) #44741 * [83b0aaa] - (SEMVER-MAJOR) deps: fix V8 build on SmartOS (Michaël Zasso) #44741 * [7a952e8] - (SEMVER-MAJOR) deps: silence irrelevant V8 warning (Michaël Zasso) #44741 * [6bd756d] - (SEMVER-MAJOR) deps: update V8 to 10.7.193.13 (Michaël Zasso) #44741 * [03fb789] - (SEMVER-MAJOR) events: add null check for the signal of EventTarget (Masashi Hirano) #43153 * [a4fa526] - (SEMVER-MAJOR) fs: add directory autodetection to fsPromises.symlink() (Livia Medeiros) #42894 * [bb4891d] - (SEMVER-MAJOR) fs: add validateBuffer to improve error (Hirotaka Tagawa / wafuwafu13) #44769 * [950a441] - (SEMVER-MAJOR) fs: remove coercion to string in writing methods (Livia Medeiros) #42796 * [41a6d82] - (SEMVER-MAJOR) fs: harden fs.readSync(buffer, options) typecheck (LiviaMedeiros) #42772 * [2275faa] - (SEMVER-MAJOR) fs: harden fs.read(params, callback) typecheck (LiviaMedeiros) #42772 * [29953a0] - (SEMVER-MAJOR) fs: harden filehandle.read(params) typecheck (LiviaMedeiros) #42772 * [4267b92] - (SEMVER-MAJOR) http: use Keep-Alive by default in global agents (Paolo Insogna) #43522 * [0324529] - (SEMVER-MAJOR) inspector: introduce inspector/promises API (Erick Wendel) #44250 * [8027099] - (SEMVER-MAJOR) lib: enable global CustomEvent by default (Daeyeon Jeong) #44860 * [f529f73] - (SEMVER-MAJOR) lib: brand check event handler property receivers (Chengzhong Wu) #44483 * [6de2673] - (SEMVER-MAJOR) lib: enable global WebCrypto by default (Antoine du Hamel) #42083 * [73ba883] - (SEMVER-MAJOR) lib: use private field in AbortController (Joyee Cheung) #43820 * [7dd2f41] - (SEMVER-MAJOR) module: runtime deprecate exports double slash maps (Guy Bedford) #44495 * [22c39b1] - (SEMVER-MAJOR) path: the dot will be added(path.format) if it is not specified in `ext` (theanarkh) #44349 * [587367d] - (SEMVER-MAJOR) perf_hooks: expose webperf global scope interfaces (Chengzhong Wu) #44483 * [364c0e1] - (SEMVER-MAJOR) perf_hooks: fix webperf idlharness (Chengzhong Wu) #44483 * [ada2d05] - (SEMVER-MAJOR) process: runtime deprecate coercion to integer in `process.exit()` (Daeyeon Jeong) #44711 * [e0ab8dd] - (SEMVER-MAJOR) process: make process.config read only (Sergey Petushkov) #43627 * [481a959] - (SEMVER-MAJOR) readline: remove `question` method from `InterfaceConstructor` (Antoine du Hamel) #44606 * [c9602ce] - (SEMVER-MAJOR) src: use new v8::OOMErrorCallback API (Michaël Zasso) #44741 * [19a70c1] - (SEMVER-MAJOR) src: override CreateJob instead of PostJob (Clemens Backes) #44741 * [fd52c62] - (SEMVER-MAJOR) src: use V8_ENABLE_SANDBOX macro (Michaël Zasso) #44741 * [c10988d] - (SEMVER-MAJOR) src: use non-deprecated V8 inspector API (Michaël Zasso) #44741 * [3efe901] - (SEMVER-MAJOR) src: update NODE_MODULE_VERSION to 111 (Michaël Zasso) #44741 * [77e5856] - (SEMVER-MAJOR) src: turn embedder api overload into default argument (Alena Khineika) #43629 * [dabda03] - (SEMVER-MAJOR) src: per-environment time origin value (Chengzhong Wu) #43781 * [2e49b99] - (SEMVER-MAJOR) src,test: disable freezing V8 flags on initialization (Clemens Backes) #44741 * [2b32985] - (SEMVER-MAJOR) stream: use null for the error argument (Luigi Pinca) #44312 * [36805e8] - (SEMVER-MAJOR) test: adapt test-repl for V8 update (Michaël Zasso) #44741 * [96ef257] - (SEMVER-MAJOR) test: adapt test-repl-pretty-*stack to V8 changes (Michaël Zasso) #44741 * [71c193e] - (SEMVER-MAJOR) test: adapt to new JSON SyntaxError messages (Michaël Zasso) #44741 * [b5f1564] - (SEMVER-MAJOR) test: rename always-opt flag to always-turbofan (Michaël Zasso) #44741 * [1acf033] - (SEMVER-MAJOR) test: fix test-hash-seed for new V8 versions (Michaël Zasso) #44741 * [57ff476] - (SEMVER-MAJOR) test: remove duplicate test (Luigi Pinca) #44051 * [77def91] - (SEMVER-MAJOR) tls,http2: send fatal alert on ALPN mismatch (Tobias Nießen) #44031 * [4860ad9] - (SEMVER-MAJOR) tools: update V8 gypfiles for 10.7 (Michaël Zasso) #44741 PR-URL: #44626 Co-authored-by: Ruy Adorno <ruyadorno@google.com>

nodejs-github-bot added inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. labels Aug 16, 2022

ErickWendel force-pushed the inspector/promisified-result-session-post branch from cdfdc2c to b1947d2 Compare August 16, 2022 15:26

This comment was marked as resolved.

Sign in to view

devsnek reviewed Aug 16, 2022

View reviewed changes

Comment thread lib/inspector.js Outdated

ErickWendel force-pushed the inspector/promisified-result-session-post branch from c204202 to 2efebf3 Compare August 16, 2022 16:45

ErickWendel self-assigned this Aug 17, 2022

ovflowd reviewed Aug 18, 2022

View reviewed changes

Comment thread lib/inspector.js Outdated

ljharb reviewed Aug 18, 2022

View reviewed changes

Comment thread doc/api/inspector.md Outdated

benjamingr reviewed Aug 18, 2022

View reviewed changes

Comment thread doc/api/inspector.md Outdated

ErickWendel marked this pull request as draft August 22, 2022 20:42

MoLow reviewed Aug 22, 2022

View reviewed changes

Comment thread lib/inspector/promises.js Outdated

ErickWendel marked this pull request as ready for review August 22, 2022 21:15

ErickWendel changed the title inspector: add promisified result for session.post when a callback is not provided inspector: introduce inspector/promises API Aug 22, 2022

benjamingr reviewed Aug 22, 2022

View reviewed changes

Comment thread doc/api/inspector.md Outdated

benjamingr reviewed Aug 22, 2022

View reviewed changes

ErickWendel requested review from MoLow, devsnek, ljharb and ovflowd and removed request for ovflowd August 23, 2022 17:30

ljharb reviewed Aug 23, 2022

View reviewed changes

Comment thread lib/inspector/promises.js Outdated

MoLow reviewed Aug 23, 2022

View reviewed changes

Comment thread doc/api/inspector.md Outdated

MoLow approved these changes Aug 23, 2022

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2022

This comment was marked as outdated.

Sign in to view

ovflowd requested changes Oct 13, 2022

View reviewed changes

aduh95 mentioned this pull request Oct 13, 2022

Enable Windows Arm64 tests in CI nodejs/build#3046

Closed

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 13, 2022

aduh95 approved these changes Oct 13, 2022

View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 13, 2022

nodejs-github-bot merged commit 0324529 into nodejs:main Oct 13, 2022

RafaelGSS mentioned this pull request Oct 14, 2022

2022-10-18, Version 19.0.0 (Current) #44626

Merged

github-actions Bot mentioned this pull request Oct 18, 2022

CI Reliability 2022-10-18 nodejs/reliability#403

Open

13 tasks

github-actions Bot mentioned this pull request Oct 19, 2022

CI Reliability 2022-10-19 nodejs/reliability#404

Open

15 tasks

fisker mentioned this pull request Oct 19, 2022

Updates sindresorhus/builtin-modules#17

Closed

github-actions Bot mentioned this pull request Oct 20, 2022

CI Reliability 2022-10-20 nodejs/reliability#405

Open

10 tasks

juanarbol mentioned this pull request Jan 25, 2023

doc: update output of example in inspector #46073

Merged

               It can be accessed using:
-              ```js
+              ```mjs

inspector: introduce inspector/promises API by ErickWendel · Pull Request #44250 · nodejs/node · GitHub

Uh oh!

Uh oh!

inspector: introduce inspector/promises API - #44250

inspector: introduce inspector/promises API#44250nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom ErickWendel:inspector/promisified-result-session-postErickWendel/node:inspector/promisified-result-session-postCopy head branch name to clipboard

Conversation

ErickWendel commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

ErickWendel commented Aug 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Aug 18, 2022

Uh oh!

benjamingr commented Aug 18, 2022

Uh oh!

Uh oh!

Uh oh!

ErickWendel commented Aug 22, 2022

Uh oh!

ErickWendel commented Aug 22, 2022

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Oct 13, 2022

Uh oh!

ovflowd left a comment

Choose a reason for hiding this comment

Uh oh!

ovflowd Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovflowd Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

ErickWendel Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

aduh95 Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErickWendel Oct 13, 2022

Choose a reason for hiding this comment

Uh oh!

ovflowd Nov 26, 2022

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 13, 2022

Uh oh!

nodejs-github-bot commented Oct 13, 2022

Uh oh!

nodejs-github-bot commented Oct 13, 2022

Uh oh!

nodejs-github-bot commented Oct 13, 2022

Uh oh!

nodejs-github-bot commented Oct 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

inspector: introduce inspector/promises API#44250
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
ErickWendel:inspector/promisified-result-session-post

ErickWendel commented Aug 16, 2022 •
edited
Loading

ErickWendel commented Aug 16, 2022 •
edited
Loading

ovflowd Oct 13, 2022 •
edited
Loading

aduh95 Oct 13, 2022 •
edited
Loading

aduh95 Oct 13, 2022 •
edited
Loading