Add result.signalName
#3
Conversation
index.js
Outdated
|
|
||
| // TODO: Pass in `stdin` and `stdout` strings here. | ||
| const waitForExit = async subprocess => { | ||
| const [exitCode, signalName] = await once(subprocess, 'close'); |
There was a problem hiding this comment.
Note: once() throws on error event (which is what we want).
index.js
Outdated
| // TODO: Pass in `stdin` and `stdout` strings here. | ||
| const waitForExit = async subprocess => { | ||
| const [exitCode, signalName] = await once(subprocess, 'close'); | ||
| return {exitCode, signalName}; |
There was a problem hiding this comment.
I double checked the core Node.js code, and just listening for the close and error events catches every possible exit situation. 👍
Also, unlike the exit event, the close event waits for IPC and file descriptors to close.
test.js
Outdated
| @@ -6,6 +6,7 @@ test('can be awaited', async t => { | |||
| // TODO | |||
| // t.is(result.stdout, '🦄'); | |||
| t.is(result.exitCode, 0); | |||
| t.is(result.signalName, null); | |||
There was a problem hiding this comment.
Can we make this undefined instead? exitCode too.
18d031e to
b77520f
Compare
|
Done. |
| const [exitCode] = await once(subprocess, 'close'); | ||
| return {...getOutput(result), exitCode}; | ||
| await once(subprocess, 'close'); | ||
| return getOutput(subprocess, result); |
There was a problem hiding this comment.
We should throw on non-0 exit code or on signal termination. I will do this in a follow-up PR. (see #8)
| test('result.exitCode|signalName on "error" event after spawn', async t => { | ||
| const {exitCode, signalName} = await t.throwsAsync(nanoSpawn('node', {signal: AbortSignal.abort()})); | ||
| t.is(exitCode, undefined); | ||
| t.is(signalName, 'SIGTERM'); |
There was a problem hiding this comment.
Those above are the 6 main ways a Node.js subprocess exits.
| const getOutput = ({exitCode, signalCode}, {stdout, stderr}) => ({ | ||
| // `exitCode` can be a negative number (`errno`) when the `error` event is emitted on the subprocess | ||
| ...(exitCode === null || exitCode < 0 ? {} : {exitCode}), | ||
| ...(signalCode === null ? {} : {signalName: signalCode}), |
There was a problem hiding this comment.
We use subprocess.exitCode and subprocess.signalCode instead of the values passed to the close event emitter. That's because the close event is not emitted when an error event was emitted before spawn.
But it turns out those are the exact same values.
See source: https://github.com/nodejs/node/blob/8b0c699f2aafdf81bc4834b9bd2a4923741d3a6f/lib/internal/child_process.js#L1104
| // Then, `subprocess.pid` is `undefined` and `close` is never emitted. | ||
| // - After `spawn`, e.g. for the `signal` option. | ||
| // Then, `subprocess.pid` is set and `close` is always emitted. | ||
| if (subprocess.pid !== undefined) { |
There was a problem hiding this comment.
This is the relevant code: https://github.com/nodejs/node/blob/main/lib/internal/child_process.js#L396-L426
| // - After `spawn`, e.g. for the `signal` option. | ||
| // Then, `subprocess.pid` is set and `close` is always emitted. | ||
| if (subprocess.pid !== undefined) { | ||
| await Promise.allSettled([once(subprocess, 'close')]); |
There was a problem hiding this comment.
Promise.allSettled() is here because the error event can technically be emitted twice on the subprocess.
| // - After `spawn`, e.g. for the `signal` option. | ||
| // Then, `subprocess.pid` is set and `close` is always emitted. | ||
| if (subprocess.pid !== undefined) { | ||
| await Promise.allSettled([once(subprocess, 'close')]); |
There was a problem hiding this comment.
We wait for close so we can get the termination signalName, if any.
This adds
result.signalName.