Repository: sindresorhus/execa (js)

Issue:
Doesn't always return a ChildProcess
We get a `childP.on is not a function` error in our logs occasionally with code like this:

```js
let childP = execa(...);
childP.on('message', ...);
```

After checking execa's code, I quickly found [this](https://github.com/sindresorhus/execa/blob/6c0a40036ad602a64b4b50d9bf2638ec57c73fa3/index.js#L230-L233), which changes any synchronously thrown errors to promise rejections.

I guess this was done to make it easier to catch all errors via `.catch()` but it makes it impossible to rely on the returned value and it isn't documented. It's ok if execa extends the error with other properties, but it should be thrown synchronously in this case.

Pull request:
Fix shape of exceptions rejected by `execa()`
Fixes #275.

When some options like `uid` are invalid, `child_process.spawn()` throws an exception. 

At the moment, we catch that exception and return a rejected promise instead. This makes the return value polymorphic: it's probably a `childProcess`, but it might not be. 

Instead we should keep the error synchronous, i.e. throw an exception just like `child_process.spawn()` does.

Patch:
diff --git a/index.js b/index.js
index 4482095..6f3d4b3 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;


Test patch:
diff --git a/test.js b/test.js
index 94a095b..18bee8c 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 => {