process: wait promise resolve before print result by H4ad · Pull Request #52077 · nodejs/node · GitHub

Inspired by https://twitter.com/jarredsumner/status/1767795689575285214

Wait for Promise resolve when printing any code using --print or --eval.

$ ./out/Release/node --print 'fetch("https://api.github.com/repos/bigskysoftware/htmx").then(r => r.json())'
{
  id: 255378816,
  node_id: 'MDEwOlJlcG9zaXRvcnkyNTUzNzg4MTY=',
  name: 'htmx',
  full_name: 'bigskysoftware/htmx',
  private: false,
  owner: {
    login: 'bigskysoftware',
    id: 48798027,
    node_id: 'MDEyOk9yZ2FuaXphdGlvbjQ4Nzk4MDI3',
    avatar_url: 'https://avatars.githubusercontent.com/u/48798027?v=4',
    gravatar_id: '',
    url: 'https://api.github.com/users/bigskysoftware',
    html_url: 'https://github.com/bigskysoftware',
    followers_url: 'https://api.github.com/users/bigskysoftware/followers',
    following_url: 'https://api.github.com/users/bigskysoftware/following{/other_user}',
    gists_url: 'https://api.github.com/users/bigskysoftware/gists{/gist_id}',
    starred_url: 'https://api.github.com/users/bigskysoftware/starred{/owner}{/repo}',
    subscriptions_url: 'https://api.github.com/users/bigskysoftware/subscriptions',
    organizations_url: 'https://api.github.com/users/bigskysoftware/orgs',
    repos_url: 'https://api.github.com/users/bigskysoftware/repos',
    events_url: 'https://api.github.com/users/bigskysoftware/events{/privacy}',
    received_events_url: 'https://api.github.com/users/bigskysoftware/received_events',
    type: 'Organization',
    site_admin: false
  },
  html_url: 'https://github.com/bigskysoftware/htmx',
  description: '</> htmx - high power tools for HTML',
  fork: false,
...

When throwing an exception, the output is:

$ ./out/Release/node --print 'Promise.resolve().then(() => { throw new Error("123"); });'
[eval]:1
Promise.resolve().then(() => { throw new Error("123"); });
                                     ^

Error: 123
    at [eval]:1:38

Node.js v22.0.0-pre

Since I'm changing the output, this probably should be marked as major, right?

Review requested:

@nodejs/loaders

Not sure what else this effects… but conceptually seems very reasonable to me! 😄

lgtm

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

lgtm

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

lgtm

@H4ad some windows runs failed with a related issue: (I think it might be because of the space character ending the command?)

12:12:01 not ok 126 parallel/test-cli-eval
12:12:01   ---
12:12:01   duration_ms: 1887.13100
12:12:01   severity: fail
12:12:01   exitcode: 1
12:12:01   stack: |-
12:12:01     node:assert:1000
12:12:01         throw newErr;
12:12:01         ^
12:12:01     
12:12:01     AssertionError [ERR_ASSERTION]: ifError got unwanted exception: Command failed: "c:\workspace\node-test-binary-windows-js-suites\node\Release\node.exe" --print  'Promise.resolve().then(() => 10)'
12:12:01     [eval]:1
12:12:01     
12:12:01     'Promise.resolve().then(()
12:12:01     
12:12:01     ^^^^^^^^^^^^^^^^^^^^^^^^^^
12:12:01     
12:12:01     
12:12:01     
12:12:01     SyntaxError: Invalid or unexpected token
12:12:01     
12:12:01         at makeContextifyScript (node:internal/vm:185:14)
12:12:01     
12:12:01         at node:internal/process/execution:109:22
12:12:01     
12:12:01         at [eval]-wrapper:6:24
12:12:01     
12:12:01         at runScript (node:internal/process/execution:103:62)
12:12:01     
12:12:01         at evalScript (node:internal/process/execution:140:3)
12:12:01     
12:12:01         at node:internal/main/eval_string:51:3
12:12:01     
12:12:01     
12:12:01     
12:12:01     Node.js v22.0.0-pre
12:12:01     
12:12:01     
12:12:01         at genericNodeError (node:internal/errors:984:15)
12:12:01         at wrappedFn (node:internal/errors:538:14)
12:12:01         at ChildProcess.exithandler (node:child_process:422:12)
12:12:01         at ChildProcess.emit (node:events:519:28)
12:12:01         at maybeClose (node:internal/child_process:1105:16)
12:12:01         at Socket.<anonymous> (node:internal/child_process:457:11)
12:12:01         at Socket.emit (node:events:519:28)
12:12:01         at Pipe.<anonymous> (node:net:337:12) {
12:12:01       generatedMessage: false,
12:12:01       code: 'ERR_ASSERTION',
12:12:01       actual: Error: Command failed: "c:\workspace\node-test-binary-windows-js-suites\node\Release\node.exe" --print  'Promise.resolve().then(() => 10)'
12:12:01       [eval]:1
12:12:01     
12:12:01       'Promise.resolve().then(()
12:12:01     
12:12:01       ^^^^^^^^^^^^^^^^^^^^^^^^^^
12:12:01     
12:12:01       
12:12:01     
12:12:01       SyntaxError: Invalid or unexpected token
12:12:01     
12:12:01           at makeContextifyScript (node:internal/vm:185:14)
12:12:01     
12:12:01           at node:internal/process/execution:109:22
12:12:01     
12:12:01           at [eval]-wrapper:6:24
12:12:01     
12:12:01           at runScript (node:internal/process/execution:103:62)
12:12:01     
12:12:01           at evalScript (node:internal/process/execution:140:3)
12:12:01     
12:12:01           at node:internal/main/eval_string:51:3
12:12:01     
12:12:01       
12:12:01     
12:12:01       Node.js v22.0.0-pre
12:12:01     
12:12:01       
12:12:01           at genericNodeError (node:internal/errors:984:15)
12:12:01           at wrappedFn (node:internal/errors:538:14)
12:12:01           at ChildProcess.exithandler (node:child_process:422:12)
12:12:01           at ChildProcess.emit (node:events:519:28)
12:12:01           at maybeClose (node:internal/child_process:1105:16)
12:12:01           at Socket.<anonymous> (node:internal/child_process:457:11)
12:12:01           at Socket.emit (node:events:519:28)
12:12:01           at Pipe.<anonymous> (node:net:337:12) {
12:12:01         code: 1,
12:12:01         killed: false,
12:12:01         signal: null,
12:12:01         cmd: `"c:\\workspace\\node-test-binary-windows-js-suites\\node\\Release\\node.exe" --print  'Promise.resolve().then(() => 10)'`
12:12:01       },
12:12:01       expected: null,
12:12:01       operator: 'ifError'
12:12:01     }
12:12:01     
12:12:01     Node.js v22.0.0-pre

I have no idea but I will investigate tonight, thanks

@H4ad actually this happens to me on a release as well with the single quotes, but works fine with double quotes 🙂

I think we should keep Promise { <result> } notation. Also, we need to take the never-settling promises into account (i.e. node -p 'new Promise(()=>{})')

Hopefully, if --experimental-detect-module becomes the default, we can enable node -p 'await promise' and avoid the need for a new flag or for "magic" behavior.

Also, we need to take the never-settling promises into account (i.e. node -p 'new Promise(()=>{})')

This is an issue that you will have once we have top level await, so we can address this issue in other PR.

I think we should keep Promise { } notation.

Well, we should keep it or not?

I don’t think it should log intermediate results like Promise; if the user wants that, they can use --eval to get more detailed and explicit output.

Currently, the implementation didn't differ if it came from --print or not, so my change will affect both --print and --eval.

But if we try to follow the suggestion from Geoffrey, we will have the following result for Promise.resolve(10):

--print: 10
--eval: Promise { 10 }.

Being honest, this suggestion could make more sense but looks weird to have different behavior/output for different flags.

Hopefully, if --experimental-detect-module becomes the default, we can enable node -p 'await promise' and avoid the need for a new flag or for "magic" behavior.

This is a good point, once we have top level await, this magic behavior will be unnecessary but still useful to have.

In this way, maybe the suggestion by Geoffrey makes more sense.

Edit

Actually, my take about --print and --eval didn't make sense, my change only affects --print and should only affect this flag.

So, I still prefer to keep the final result WITHOUT the Promise { }.

Here's my suggestion: we're still logging the raw input, but we wait for the process to exit to do so. I see two upsides for doing this:

We no longer need a special case for promises.
it will still show the value with which the promise was fulfilled (if applicable).
it handles never-settling promises just right, and other edge cases (e.g. node -p 'setTimeout(process.exit,100, 0);timers.promises.setTimeout(200)')

Suggested change

      
                  if (isPromise(result)) {
          
                    PromisePrototypeThen(result, (resultValue) => log(resultValue));
          
                  } else {
          
                    log(result);
          
                  }
          
                  process.on('exit', () => {
          
                    log(result);
          
                  });

The output will be:

I mean, this PR was initiated by the inspiration from another runtime, do we really want to have different behavior with them in this utility?

If we change to:

process.on('exit', async () => {
  log(await result)
})

Works great and also handles timeouts.

Please consider copy pasting your console output rather than sharing screenshots, as screenshots might be harder to interpret for e.g. folks using screen readers, and it makes it much harder to copy/paste if someone wants to reproduce what you did.

do we really want to have different behavior with them in this utility?

I think we do.

If we all agree that my suggestion is an improvement, we could start by landing that, and let's start another discussion to try to find consensus – or if you prefer I can open a PR with my suggestion.

If we all agree that my suggestion is an improvement, we could start by landing that

I liked the idea of putting the print on process.on('exit'), I will keep that, but how we want to print the results is something we need to decide and I think we should decide in this PR to land everything together.

Initially, @mcollina raised the same concern but later dismissed their request change for that, currently, I think you are the only one who thinks we should print the Promise { result } now.

I don't see the point in waiting for a decision, on the contrary we should make small incremental improvements. I'll open a PR with my suggestion if that's OK.

If you remove the Request Change for this PR, I'm okay with you creating another PR.

I push the changes with process.on

I've opened #52137, I suggest we fast-track it, and you can rebase you PR on top of it and we can see better what diff your PR introduces for the various cases I suggested there.

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

Force pushed/rebased just to include the tests added by @aduh95.

Can you keep the test cases in the same order so git shows the diff in a more straight forward way?

Like I said above, I don't think the current PR is improving things if there are cases where nothing at all get printed to the stdout.
I also think the auto-await feature is not desirable: -p is used as a debugging feature, and therefor indicate when the value is a promise / thenable. Adding .then(console.log) is an existing workaround for folks who try to get promise result from a script.

I don't think it's acceptable.

Same here.

Same here. I would consider that change semver-major if we stick with it.

Same here.

My concerns would be addressed if we removed the await here.

Suggested change

      
                  process.on('exit', async () => {
          
                    log(await result);
          
                  process.on('exit', () => {
          
                    log(result);

Closing in favor of #52172

Co-authored-by: Vinícius Lourenço <contact@viniciusl.com.br> Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> PR-URL: #52172 Refs: #52077 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. labels Mar 14, 2024

mcollina added semver-major PRs that contain breaking changes and should be released in the next major version. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 14, 2024

mcollina approved these changes Mar 14, 2024

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2024

mertcanaltin approved these changes Mar 14, 2024

View reviewed changes

mcollina requested changes Mar 14, 2024

View reviewed changes

Comment thread lib/internal/process/execution.js Outdated

atlowChemi approved these changes Mar 14, 2024

View reviewed changes

targos approved these changes Mar 14, 2024

View reviewed changes

mcollina approved these changes Mar 14, 2024

View reviewed changes

atlowChemi reviewed Mar 15, 2024

View reviewed changes

Comment thread test/parallel/test-cli-eval.js Outdated

Comment thread test/parallel/test-cli-eval.js Outdated

aduh95 reviewed Mar 15, 2024

View reviewed changes

Comment thread test/parallel/test-cli-eval.js Outdated

aduh95 reviewed Mar 15, 2024

View reviewed changes

Comment thread test/parallel/test-cli-eval.js Outdated

aduh95 requested changes Mar 15, 2024

View reviewed changes

aduh95 reviewed Mar 16, 2024

View reviewed changes

H4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 18, 2024

H4ad force-pushed the print-handle-promises branch from dc57999 to a808430 Compare March 20, 2024 02:04

H4ad requested a review from aduh95 March 20, 2024 02:06

aduh95 reviewed Mar 20, 2024

View reviewed changes

process: wait promise resolve before print result

dbcf465

H4ad force-pushed the print-handle-promises branch from a808430 to dbcf465 Compare March 20, 2024 22:22

H4ad requested a review from aduh95 March 20, 2024 22:23

aduh95 requested changes Mar 20, 2024

View reviewed changes

aduh95 mentioned this pull request Mar 21, 2024

process: wait for 'exit' before printing result #52172

Merged

H4ad closed this Mar 25, 2024

H4ad deleted the print-handle-promises branch March 25, 2024 12:26

                     signal: null,
                     stderr: '',
-                    stdout: 'Promise { <pending> }\n',
+                    stdout: '',

                     signal: null,
                     stderr: '',
-                    stdout: 'Promise { <pending> }\n',
+                    stdout: '',

                     signal: null,
                     stderr: '',
-                    stdout: 'Promise { <rejected> 1 }\n',
+                    stdout: '',

                     signal: null,
                     stderr: '',
-                    stdout: 'Promise { <pending> }\n',
+                    stdout: '',

+                  assert.strictEqual(result.code, 1);
+                  assert.strictEqual(result.signal, null);
+                  assert.strictEqual(result.stdout, '');

process: wait promise resolve before print result by H4ad · Pull Request #52077 · nodejs/node · GitHub

Uh oh!

Uh oh!

process: wait promise resolve before print result - #52077

process: wait promise resolve before print result#52077H4ad wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom H4ad:print-handle-promisesH4ad/node:print-handle-promisesCopy head branch name to clipboard

Conversation

H4ad commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 14, 2024

Uh oh!

izaakschroeder commented Mar 14, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 14, 2024

Uh oh!

mertcanaltin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 14, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

atlowChemi commented Mar 15, 2024

Uh oh!

H4ad commented Mar 15, 2024

Uh oh!

atlowChemi commented Mar 15, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

H4ad commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Edit

Uh oh!

aduh95 Mar 16, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Mar 18, 2024

Uh oh!

H4ad commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Mar 20, 2024 • 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!

process: wait promise resolve before print result#52077
H4ad wants to merge 1 commit into
nodejs:mainfrom
H4ad:print-handle-promises

H4ad commented Mar 14, 2024 •
edited
Loading

H4ad commented Mar 16, 2024 •
edited
Loading

aduh95 Mar 16, 2024 •
edited
Loading

H4ad commented Mar 20, 2024 •
edited
Loading

aduh95 left a comment •
edited
Loading

aduh95 Mar 20, 2024 •
edited
Loading