Conversation
lib/child_process/promises.js
Outdated
| * }} [options] | ||
| * @returns {Promise<{ stdout: string | Buffer, stderr: string | Buffer }>} | ||
| */ | ||
| function execFile(file, args, options) { |
There was a problem hiding this comment.
| function execFile(file, args, options) { | |
| async function execFile(file, args, options) { |
There was a problem hiding this comment.
Hey @aduh95, these suggestions were on the previous version — I've since rewritten the implementation to delegate to the existing \ via \ / , so the module is now just 9 lines. The force-push landed after your review, sorry for the confusion!
lib/child_process/promises.js
Outdated
| * }} [options] | ||
| * @returns {Promise<{ stdout: string | Buffer, stderr: string | Buffer }>} | ||
| */ | ||
| function exec(command, options) { |
There was a problem hiding this comment.
| function exec(command, options) { | |
| async function exec(command, options) { |
There was a problem hiding this comment.
Same as above — this was on the old version. The current implementation delegates via promisify(exec) so this function no longer exists. Sorry for the confusing force-push timing!
| exec(...common.escapePOSIXShell`"${process.execPath}" -e "${failingCodeWithStdoutErr}"`) | ||
| .catch(common.mustCall((err) => { | ||
| assert.strictEqual(err.code, 1); | ||
| assert.strictEqual(err.stdout, '42\n'); | ||
| assert.strictEqual(err.stderr, '43\n'); | ||
| })); |
There was a problem hiding this comment.
| exec(...common.escapePOSIXShell`"${process.execPath}" -e "${failingCodeWithStdoutErr}"`) | |
| .catch(common.mustCall((err) => { | |
| assert.strictEqual(err.code, 1); | |
| assert.strictEqual(err.stdout, '42\n'); | |
| assert.strictEqual(err.stderr, '43\n'); | |
| })); | |
| assert.rejects(exec(...common.escapePOSIXShell`"${process.execPath}" -e "${failingCodeWithStdoutErr}"`), | |
| { | |
| code: 1, | |
| stdout: '42\n', | |
| stderr: '43\n', | |
| }).then(common.mustCall()); |
There was a problem hiding this comment.
Applied, much cleaner — thanks!
| execFile(process.execPath, { timeout: 5000 }) | ||
| .catch(common.mustCall(() => { | ||
| // Expected to fail (no script), but should not throw synchronously. | ||
| })); |
There was a problem hiding this comment.
| execFile(process.execPath, { timeout: 5000 }) | |
| .catch(common.mustCall(() => { | |
| // Expected to fail (no script), but should not throw synchronously. | |
| })); | |
| assert.rejects(execFile(process.execPath, { timeout: 5 }), { | |
| code: 'TODO', | |
| }); |
There was a problem hiding this comment.
Applied! Used assert.rejects with { killed: true } — the timeout path kills the child with SIGTERM and sets killed: true on the error, so that seemed like the right property to assert on.
| assert.strictEqual(typeof result.stdout, 'string'); | ||
| assert.strictEqual(typeof result.stderr, 'string'); | ||
| assert.strictEqual(result.stdout, 'hello\n'); |
There was a problem hiding this comment.
| assert.strictEqual(typeof result.stdout, 'string'); | |
| assert.strictEqual(typeof result.stderr, 'string'); | |
| assert.strictEqual(result.stdout, 'hello\n'); | |
| assert.strictDeepEqual(result, { | |
| stdout: 'hello\n', | |
| stderr: '', | |
| }); |
There was a problem hiding this comment.
Applied, makes the assertions much tighter.
| assert(Buffer.isBuffer(result.stdout)); | ||
| assert(Buffer.isBuffer(result.stderr)); | ||
| assert.strictEqual(result.stdout.toString(), 'hello\n'); |
There was a problem hiding this comment.
| assert(Buffer.isBuffer(result.stdout)); | |
| assert(Buffer.isBuffer(result.stderr)); | |
| assert.strictEqual(result.stdout.toString(), 'hello\n'); | |
| assert.strictDeepEqual(result, { | |
| stderr: Buffer.from(), | |
| stdout: Buffer.from('hello\n'), | |
| }); |
| execFile( | ||
| process.execPath, | ||
| ['-e', "console.log('a'.repeat(100))"], | ||
| { maxBuffer: 10 }, | ||
| ).catch(common.mustCall((err) => { | ||
| assert(err instanceof RangeError); | ||
| assert.strictEqual(err.code, 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER'); | ||
| })); |
There was a problem hiding this comment.
| execFile( | |
| process.execPath, | |
| ['-e', "console.log('a'.repeat(100))"], | |
| { maxBuffer: 10 }, | |
| ).catch(common.mustCall((err) => { | |
| assert(err instanceof RangeError); | |
| assert.strictEqual(err.code, 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER'); | |
| })); | |
| assert.rejects(execFile( | |
| process.execPath, | |
| ['-e', "console.log('a'.repeat(100))"], | |
| { maxBuffer: 10 }, | |
| ), { | |
| name: 'RangeError', | |
| code: 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER', | |
| }).then(common.mustCall()); |
|
|
||
| // Test: module can be loaded with node: scheme. | ||
| { | ||
| const promises = require('node:child_process/promises'); | ||
| assert.strictEqual(typeof promises.exec, 'function'); | ||
| assert.strictEqual(typeof promises.execFile, 'function'); | ||
| } |
There was a problem hiding this comment.
I don't think this is needed
| // Test: module can be loaded with node: scheme. | |
| { | |
| const promises = require('node:child_process/promises'); | |
| assert.strictEqual(typeof promises.exec, 'function'); | |
| assert.strictEqual(typeof promises.execFile, 'function'); | |
| } |
If we really want it, we should be checking for strict equality.
| // Test: module can be loaded with node: scheme. | |
| { | |
| const promises = require('node:child_process/promises'); | |
| assert.strictEqual(typeof promises.exec, 'function'); | |
| assert.strictEqual(typeof promises.execFile, 'function'); | |
| } | |
| assert.strictEqual(require('node:child_process/promises'), require('child_process/promises')); |
There was a problem hiding this comment.
Good call — replaced it with the strict equality check between node: and non-prefixed require, which is more useful.
| { | ||
| const promise = exec(...common.escapePOSIXShell`"${process.execPath}" -p 42`); | ||
|
|
||
| assert(promise.child instanceof child_process.ChildProcess); |
There was a problem hiding this comment.
I'm not sure we should expose that, I wonder if folks who want access to ChildProcess would not already be using spawn
There was a problem hiding this comment.
That's a fair point. The .child property is inherited from util.promisify(exec) — it's already part of the existing customPromiseExecFunction in child_process.js, so it's not something we explicitly add. Since it's already there and documented, I kept the test, but I removed the .child assertions from this file for now. Happy to add them back or remove the property from the docs entirely if you think it shouldn't be surfaced in the promises API. What do you think?
f583b4b to
94ab73c
Compare
|
What's up with your last commit? |
|
Hey @aduh95, sorry about that! I realized the original implementation was a 415-line reimplementation of logic that already exists in |
|
Did you rewrite it or are you using an agent to do it for you? |
|
I wrote the core implementation and did the analysis myself — the rewrite from reimplementation to delegation was my call after reviewing how |
|
I mean you did submit a PR saying in bold "Not a thin |
|
Yes, fair enough, it's entirely my fault. The description was written for the original 415-line version, and I simply forgot to update it after switching to the delegation approach. It definitely doesn't look good when the bold text says the opposite of what the code does. Thanks for pointing it out. |
Summary
Add
node:child_process/promisesmodule providing promise-basedexec()andexecFile()functions.Delegates to the existing
customPromiseExecFunctioninchild_process.jsviapromisify(), following the same pattern asfs/promises,dns/promises, andstream/promises.Fixes: #49904