Repository: sindresorhus/execa
Pull request #181 (2019-03-01 09:45:10)

Improve `execa.shell()` and remove dead code
The logic in `execa.shell()` can be replaced by simply using the `shell: true` option.

I looked it through and I think the reason this logic was here in the first place was for older Node.js versions.

`handleShell()` does exactly what [the `shell: true` option](https://github.com/nodejs/node/blob/master/lib/child_process.js#L474) does in core Node.js, except the last one is a little better:
   - it supports Android (which uses a different location for `/bin/sh`)
   - it supports the `%COMSPEC%` environment variable being something else than `cmd.exe`. The current `execa` code adds `cmd.exe`-specific flags even when the chosen shell is not `cmd.exe`.
   - it adds the `/d` flag with `cmd.exe` which disables registry AutoRun commands

The `handleShell()` function also sets a `__winShell` flag which [bypasses](https://github.com/sindresorhus/execa/blob/master/index.js#L28) `cross-spawn._parse()`. But `cross-spawn._parse()` with `shell: true` is [effectively a noop with Node.js >= 6](https://github.com/moxystudio/node-cross-spawn/blob/master/lib/parse.js#L71) which is [what this library supports](https://github.com/sindresorhus/execa/blob/master/package.json#L12).


Patch:
diff --git a/index.js b/index.js
index 0610a58..b4b5e34 100644
--- a/index.js
+++ b/index.js
@@ -14,8 +14,6 @@ const stdio = require('./lib/stdio');
 const TEN_MEGABYTES = 1000 * 1000 * 10;
 
 function handleArgs(command, args, options) {
-	let parsed;
-
 	options = Object.assign({
 		extendEnv: true,
 		env: {}
@@ -25,21 +23,7 @@ function handleArgs(command, args, options) {
 		options.env = Object.assign({}, process.env, options.env);
 	}
 
-	if (options.__winShell === true) {
-		delete options.__winShell;
-		parsed = {
-			command,
-			args,
-			options,
-			file: command,
-			original: {
-				command,
-				args
-			}
-		};
-	} else {
-		parsed = crossSpawn._parse(command, args, options);
-	}
+	const parsed = crossSpawn._parse(command, args, options);
 
 	options = Object.assign({
 		maxBuffer: TEN_MEGABYTES,
@@ -102,24 +86,7 @@ function handleOutput(options, value) {
 }
 
 function handleShell(fn, command, options) {
-	let file = '/bin/sh';
-	let args = ['-c', command];
-
-	options = Object.assign({}, options);
-
-	if (process.platform === 'win32') {
-		options.__winShell = true;
-		file = process.env.comspec || 'cmd.exe';
-		args = ['/s', '/c', `"${command}"`];
-		options.windowsVerbatimArguments = true;
-	}
-
-	if (options.shell) {
-		file = options.shell;
-		delete options.shell;
-	}
-
-	return fn(file, args, options);
+	return fn(command, Object.assign({}, options, {shell: true}));
 }
 
 function getStream(process, stream, {encoding, buffer, maxBuffer}) {
@@ -316,6 +283,10 @@ module.exports = (command, args, options) => {
 
 	spawned.then = (onfulfilled, onrejected) => handlePromise().then(onfulfilled, onrejected);
 	spawned.catch = onrejected => handlePromise().catch(onrejected);
+	// eslint-disable-next-line no-use-extend-native/no-use-extend-native
+	if (Promise.prototype.finally) {
+		spawned.finally = onfinally => handlePromise().finally(onfinally);
+	}
 
 	return spawned;
 };


Test patch:
diff --git a/test.js b/test.js
index 05ad533..1c11ada 100644
--- a/test.js
+++ b/test.js
@@ -519,3 +519,39 @@ test('removes exit handler on exit', async t => {
 	const included = ee.listeners('exit').includes(listener);
 	t.false(included);
 });
+
+// eslint-disable-next-line no-use-extend-native/no-use-extend-native
+if (Promise.prototype.finally) {
+	test('finally function is executed on success', async t => {
+		let called = false;
+		const {stdout} = await m('noop', ['foo']).finally(() => {
+			called = true;
+		});
+		t.is(called, true);
+		t.is(stdout, 'foo');
+	});
+
+	test('finally function is executed on failure', async t => {
+		let called = false;
+		const err = await t.throws(m('exit', ['2']).finally(() => {
+			called = true;
+		}));
+		t.is(called, true);
+		t.is(typeof err.stdout, 'string');
+		t.is(typeof err.stderr, 'string');
+	});
+
+	test('throw in finally function bubbles up on success', async t => {
+		const result = await t.throws(m('noop', ['foo']).finally(() => {
+			throw new Error('called');
+		}));
+		t.is(result.message, 'called');
+	});
+
+	test('throw in finally bubbles up on error', async t => {
+		const result = await t.throws(m('exit', ['2']).finally(() => {
+			throw new Error('called');
+		}));
+		t.is(result.message, 'called');
+	});
+}