child_process: Pass child in execFile resolve/reject too
#34345
Conversation
nodejs-github-bot
added
the
child_process
doc/api/child_process.md
Outdated
| console.log(`stdout: ${stdout}`); | ||
| console.error(`stderr: ${stderr}`); | ||
| console.log('stdout:', stdout); | ||
| console.error('stderr:', stderr); |
There was a problem hiding this comment.
No unrelated changes, please. Can you back this out?
There was a problem hiding this comment.
@bnoordhuis I was trying to create several examples that show how to do the same thing with different functions, you'll see I used the same code in 326-335: https://github.com/nodejs/node/pull/34345/files#diff-fd8da87c3b5a1cec8e34fc76659ac6a0R326-R335 — is this change still too much, all considered?
There was a problem hiding this comment.
I changed it back, but there's another example that's already using the console.log('stdout:', stdout); form — is it more important to leave these unchanged, or can I touch them up for consistency?
There was a problem hiding this comment.
If it's clearly related to the changes you're making anyway, then it's fine (but even then you should aim to make the most minimal change.) Apparently, it wasn't obvious to me yesterday that they were related. :-)
| @@ -12,7 +12,9 @@ const execFile = promisify(child_process.execFile); | |||
|
|
|||
| assert(promise.child instanceof child_process.ChildProcess); | |||
| promise.then(common.mustCall((obj) => { | |||
| assert.deepStrictEqual(obj, { stdout: '42\n', stderr: '' }); | |||
There was a problem hiding this comment.
Note this is technically a breaking change — the test currently does not permit the addition of any properties. I changed this to multiple assertions, since (afaict) deepStrictEqual would be difficult to use on an instance of ChildProcess.
| @@ -209,8 +209,7 @@ encoding, `Buffer` objects will be passed to the callback instead. | |||
| const { exec } = require('child_process'); | |||
| exec('cat *.js missing_file | wc -l', (error, stdout, stderr) => { | |||
| if (error) { | |||
| console.error(`exec error: ${error}`); | |||
| return; | |||
| console.error(`exit(${error.code}): ${error}`); | |||
There was a problem hiding this comment.
Needs a return; but... it's still a mostly unrelated change. Can you back it out or move it to a separate commit?
|
Idea: Would it make sense to add a |
This PR adds a
childproperty to the resolve & reject values to a promisify'dexecFile&exec. This simplifies the ability to get at theChildProcessinstance when it's only necessary once the process has exited. (For example, to retrieve the process id).As discussed in #34234.
Still needed are tests.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes