From 49c74ac805b20aa3585c6cff9e1cc279d94a8e29 Mon Sep 17 00:00:00 2001
From: ehmicky <ehmicky@users.noreply.github.com>
Date: Fri, 14 Jun 2019 23:25:56 -0700
Subject: [PATCH] Fix shape of exceptions rejected by `execa()` (#276)

---
 index.js | 50 ++++++++++++++++++++++++++++++++++----------------
 test.js  | 20 ++++++++++++++++++++
 2 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/index.js b/index.js
index 4482095cc..6f3d4b3a4 100644
--- a/index.js
+++ b/index.js
@@ -228,6 +228,30 @@ function joinCommand(file, args = []) {
 	return [file, ...args].join(' ');
 }
 
+function mergePromiseProperty(spawned, getPromise, property) {
+	Object.defineProperty(spawned, property, {
+		value(...args) {
+			return getPromise()[property](...args);
+		},
+		writable: true,
+		enumerable: false,
+		configurable: true
+	});
+}
+
+// The return value is a mixin of `childProcess` and `Promise`
+function mergePromise(spawned, getPromise) {
+	mergePromiseProperty(spawned, getPromise, 'then');
+	mergePromiseProperty(spawned, getPromise, 'catch');
+
+	// TODO: Remove the `if`-guard when targeting Node.js 10
+	if (Promise.prototype.finally) {
+		mergePromiseProperty(spawned, getPromise, 'finally');
+	}
+
+	return spawned;
+}
+
 function spawnedKill(kill, signal = 'SIGTERM', options = {}) {
 	const killResult = kill(signal);
 	setKillTimeout(kill, signal, options, killResult);
@@ -275,13 +299,15 @@ const execa = (file, args, options) => {
 	try {
 		spawned = childProcess.spawn(parsed.file, parsed.args, parsed.options);
 	} catch (error) {
-		return Promise.reject(makeError({error, stdout: '', stderr: '', all: ''}, {
-			command,
-			parsed,
-			timedOut: false,
-			isCanceled: false,
-			killed: false
-		}));
+		return mergePromise(new childProcess.ChildProcess(), () =>
+			Promise.reject(makeError({error, stdout: '', stderr: '', all: ''}, {
+				command,
+				parsed,
+				timedOut: false,
+				isCanceled: false,
+				killed: false
+			}))
+		);
 	}
 
 	const kill = spawned.kill.bind(spawned);
@@ -403,21 +429,13 @@ const execa = (file, args, options) => {
 
 	spawned.all = makeAllStream(spawned);
 
-	// eslint-disable-next-line promise/prefer-await-to-then
-	spawned.then = (onFulfilled, onRejected) => handlePromise().then(onFulfilled, onRejected);
-	spawned.catch = onRejected => handlePromise().catch(onRejected);
 	spawned.cancel = () => {
 		if (spawned.kill()) {
 			isCanceled = true;
 		}
 	};
 
-	// TODO: Remove the `if`-guard when targeting Node.js 10
-	if (Promise.prototype.finally) {
-		spawned.finally = onFinally => handlePromise().finally(onFinally);
-	}
-
-	return spawned;
+	return mergePromise(spawned, handlePromise);
 };
 
 module.exports = execa;
diff --git a/test.js b/test.js
index 94a095b62..18bee8c17 100644
--- a/test.js
+++ b/test.js
@@ -301,6 +301,15 @@ test('execa() returns a promise with kill() and pid', t => {
 	t.is(typeof pid, 'number');
 });
 
+test('child_process.spawn() propagated errors have correct shape', t => {
+	const cp = execa('noop', {uid: -1});
+	t.notThrows(() => {
+		cp.catch(() => {});
+		cp.unref();
+		cp.on('error', () => {});
+	});
+});
+
 test('child_process.spawn() errors are propagated', async t => {
 	const {failed} = await t.throwsAsync(execa('noop', {uid: -1}));
 	t.true(failed);
@@ -664,6 +673,17 @@ test('removes exit handler on exit', async t => {
 	t.false(included);
 });
 
+test('promise methods are not enumerable', t => {
+	const descriptors = Object.getOwnPropertyDescriptors(execa('noop'));
+	// eslint-disable-next-line promise/prefer-await-to-then
+	t.false(descriptors.then.enumerable);
+	t.false(descriptors.catch.enumerable);
+	// TOOD: Remove the `if`-guard when targeting Node.js 10
+	if (Promise.prototype.finally) {
+		t.false(descriptors.finally.enumerable);
+	}
+});
+
 // TOOD: Remove the `if`-guard when targeting Node.js 10
 if (Promise.prototype.finally) {
 	test('finally function is executed on success', async t => {