process: wait promise resolve before print result #52077

H4ad · 2024-03-14T01:38:29Z

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?

nodejs-github-bot · 2024-03-14T01:38:34Z

Review requested:

@nodejs/loaders

izaakschroeder · 2024-03-14T01:49:19Z

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

mcollina

lgtm

nodejs-github-bot · 2024-03-14T05:55:26Z

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

mertcanaltin

lgtm

lib/internal/process/execution.js

nodejs-github-bot · 2024-03-14T16:00:49Z

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

mcollina

lgtm

atlowChemi · 2024-03-15T07:10:27Z

@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

H4ad · 2024-03-15T10:04:21Z

I have no idea but I will investigate tonight, thanks

atlowChemi · 2024-03-15T12:54:48Z

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

test/parallel/test-cli-eval.js

aduh95

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.

H4ad · 2024-03-16T02:40:07Z

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 { }.

aduh95 · 2024-03-16T09:51:52Z

lib/internal/process/execution.js

+      if (isPromise(result)) {
+        PromisePrototypeThen(result, (resultValue) => log(resultValue));
+      } else {
+        log(result);
+      }


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.

nodejs-github-bot · 2024-03-18T15:55:17Z

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

H4ad · 2024-03-20T02:05:01Z

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

aduh95

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

aduh95

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.

aduh95 · 2024-03-20T22:27:09Z