Repository: sindresorhus/execa (js)

Issue:
Improve stream error handling
When `maxBuffer` is reached, the rejected error has a different shape than usual:

```js
{
	"name": "MaxBufferError",
	"bufferedData": "...",
	"stream": "stdout"
}
```

Shouldn't it have the same shape? For example:

```js
// No `code`, `exitCode`, `exitCodeName`, `signal`
{
	// If available
	stdout,
	stderr, 
	all,

	// This indicates a stream error
	stream: "stdout",
	// This indicates a stream error due to `maxBuffer`
	maxBuffer: true|false,

	failed: true,
	timedOut: false,
	isCanceled: false,
	
	command: "...",
}
```

`stdout`, `stderr` and `all` could either:
  a) be present only if the whole stream finished before the error
  b) show the partially `bufferedData`

The error message could be made more descriptive as well.

I can submit a PR if you think that's a good idea.

Pull request description:
Improve error handling of streams
Fixes #228. The issue was that errors on stdin/stdout/stderr were not reported the same way as other errors.

It also adds:
  - `error.stdout` or `error.stderr` set with the buffered data when that stream failed
  - `error.stream` set with the name of the failing stream
  - better `error.message` on stream errors

# Issues

If one stream errors, we do not want to wait for other streams to complete, for the same reason as #270, i.e. those streams might take a long time to complete (or never complete). However this creates the following issues:
1) we can only get the buffered data of the failing stream, not the other streams. `get-stream` does not seem to allow to retrieve the currently buffered data.
2) several streams might fail at the same time when they rely on the same underlying file/TTY. In that case, there's a race condition where `error.stream` will be set tot the first stream to report an error.
3) `all` will always fail right before `stdout` or `stderr`, taking precedence over them.

Possible solutions for each of those issues:
1) Make the other streams fail by emitting an `error` event on them. Not implemented yet.
2) Wait for one macrotask (`setImmediate()`) and check if other streams have failed. Not implemented yet.
3) Ignore errors on `all`. This PR currently implements this.

Patch:
diff --git a/index.d.ts b/index.d.ts
index 973c99a..e5d6402 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -41,6 +41,8 @@ declare namespace execa {
 		/**
 		Buffer the output from the spawned process. When buffering is disabled you must consume the output of the `stdout` and `stderr` streams because the promise will not be resolved/rejected until they have completed.
 
+		If the spawned process fails, `error.stdout`, `error.stderr`, and `error.all` will contain the buffered data.
+
 		@default true
 		*/
 		readonly buffer?: boolean;
diff --git a/index.js b/index.js
index 9d55dbc..4482095 100644
--- a/index.js
+++ b/index.js
@@ -106,6 +106,20 @@ function makeAllStream(spawned) {
 	return mixed;
 }
 
+async function getBufferedData(stream, streamPromise) {
+	if (!stream) {
+		return;
+	}
+
+	stream.destroy();
+
+	try {
+		return await streamPromise;
+	} catch (error) {
+		return error.bufferedData;
+	}
+}
+
 function getStream(process, stream, {encoding, buffer, maxBuffer}) {
 	if (!process[stream]) {
 		return;
@@ -129,11 +143,7 @@ function getStream(process, stream, {encoding, buffer, maxBuffer}) {
 		ret = _getStream.buffer(process[stream], {maxBuffer});
 	}
 
-	return ret.catch(error => {
-		error.stream = stream;
-		error.message = `${stream} ${error.message}`;
-		throw error;
-	});
+	return ret;
 }
 
 function makeError(result, options) {
@@ -163,6 +173,10 @@ function makeError(result, options) {
 		error.all = result.all;
 	}
 
+	if ('bufferedData' in error) {
+		delete error.bufferedData;
+	}
+
 	error.failed = true;
 	error.timedOut = timedOut;
 	error.isCanceled = isCanceled;
@@ -325,35 +339,23 @@ const execa = (file, args, options) => {
 		}
 	}), cleanup);
 
-	function destroy() {
-		if (spawned.stdout) {
-			spawned.stdout.destroy();
-		}
-
-		if (spawned.stderr) {
-			spawned.stderr.destroy();
-		}
-
-		if (spawned.all) {
-			spawned.all.destroy();
-		}
-	}
-
 	const handlePromise = () => {
-		const processComplete = Promise.all([
-			processDone,
-			getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}),
-			getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}),
-			getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2})
-		]);
+		const stdoutPromise = getStream(spawned, 'stdout', {encoding, buffer, maxBuffer});
+		const stderrPromise = getStream(spawned, 'stderr', {encoding, buffer, maxBuffer});
+		const allPromise = getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2});
 
 		const finalize = async () => {
 			let results;
 			try {
-				results = await processComplete;
+				results = await Promise.all([processDone, stdoutPromise, stderrPromise, allPromise]);
 			} catch (error) {
-				const {stream, code, signal} = error;
-				results = [{error, stream, code, signal}, '', '', ''];
+				const {code, signal} = error;
+				results = await Promise.all([
+					{error, code, signal},
+					getBufferedData(spawned.stdout, stdoutPromise),
+					getBufferedData(spawned.stderr, stderrPromise),
+					getBufferedData(spawned.all, allPromise)
+				]);
 			}
 
 			const [result, stdout, stderr, all] = results;
@@ -392,8 +394,7 @@ const execa = (file, args, options) => {
 			};
 		};
 
-		// TODO: Use native "finally" syntax when targeting Node.js 10
-		return pFinally(finalize(), destroy);
+		return finalize();
 	};
 
 	crossSpawn._enoent.hookChildProcess(spawned, parsed.parsed);
diff --git a/readme.md b/readme.md
index 6f8087e..4ffbb35 100644
--- a/readme.md
+++ b/readme.md
@@ -282,6 +282,8 @@ Default: `true`
 
 Buffer the output from the spawned process. When buffering is disabled you must consume the output of the `stdout` and `stderr` streams because the promise will not be resolved/rejected until they have completed.
 
+If the spawned process fails, [`error.stdout`](#stdout), [`error.stderr`](#stderr), and [`error.all`](#all) will contain the buffered data.
+
 #### input
 
 Type: `string | Buffer | stream.Readable`


Test patch:
diff --git a/test.js b/test.js
index a572c3a..94a095b 100644
--- a/test.js
+++ b/test.js
@@ -65,11 +65,15 @@ test('stdout/stderr/all are undefined if ignored in sync mode', t => {
 	t.is(all, undefined);
 });
 
+const WRONG_COMMAND = process.platform === 'win32' ?
+	'\'wrong\' is not recognized as an internal or external command,\r\noperable program or batch file.' :
+	'';
+
 test('stdout/stderr/all on process errors', async t => {
 	const {stdout, stderr, all} = await t.throwsAsync(execa('wrong command'));
 	t.is(stdout, '');
-	t.is(stderr, '');
-	t.is(all, '');
+	t.is(stderr, WRONG_COMMAND);
+	t.is(all, WRONG_COMMAND);
 });
 
 test('stdout/stderr/all on process errors, in sync mode', t => {
@@ -77,9 +81,7 @@ test('stdout/stderr/all on process errors, in sync mode', t => {
 		execa.sync('wrong command');
 	});
 	t.is(stdout, '');
-	t.is(stderr, process.platform === 'win32' ?
-		'\'wrong\' is not recognized as an internal or external command,\r\noperable program or batch file.' :
-		'');
+	t.is(stderr, WRONG_COMMAND);
 	t.is(all, undefined);
 });
 
@@ -258,10 +260,16 @@ test('input option can be a Buffer - sync', t => {
 	t.is(stdout, 'testing12');
 });
 
+test('stdin errors are handled', async t => {
+	const child = execa('noop');
+	child.stdin.emit('error', new Error('test'));
+	await t.throwsAsync(child, /test/);
+});
+
 test('child process errors are handled', async t => {
 	const child = execa('noop');
 	child.emit('error', new Error('test'));
-	await t.throwsAsync(child, /Command failed.*\ntest/);
+	await t.throwsAsync(child, /test/);
 });
 
 test('opts.stdout:ignore - stdout will not collect data', async t => {
@@ -306,13 +314,17 @@ test('child_process.spawnSync() errors are propagated with a correct shape', t =
 });
 
 test('maxBuffer affects stdout', async t => {
-	await t.throwsAsync(execa('max-buffer', ['stdout', '11'], {maxBuffer: 10}), /stdout maxBuffer exceeded/);
 	await t.notThrowsAsync(execa('max-buffer', ['stdout', '10'], {maxBuffer: 10}));
+	const {stdout, all} = await t.throwsAsync(execa('max-buffer', ['stdout', '11'], {maxBuffer: 10}), /max-buffer stdout/);
+	t.is(stdout, '.'.repeat(10));
+	t.is(all, '.'.repeat(10));
 });
 
 test('maxBuffer affects stderr', async t => {
-	await t.throwsAsync(execa('max-buffer', ['stderr', '13'], {maxBuffer: 12}), /stderr maxBuffer exceeded/);
-	await t.notThrowsAsync(execa('max-buffer', ['stderr', '12'], {maxBuffer: 12}));
+	await t.notThrowsAsync(execa('max-buffer', ['stderr', '10'], {maxBuffer: 10}));
+	const {stderr, all} = await t.throwsAsync(execa('max-buffer', ['stderr', '11'], {maxBuffer: 10}), /max-buffer stderr/);
+	t.is(stderr, '.'.repeat(10));
+	t.is(all, '.'.repeat(10));
 });
 
 test('do not buffer stdout when `buffer` set to `false`', async t => {