Repository: sindresorhus/execa
Pull request #225 (2019-05-07 20:08:59)

Improve `cleanup` option (code quality, tests, documentation)
This improves the `cleanup` option, but does not change its behavior:
  - small code simplification
  - added tests for `options.detached: true` (before only `options.detached: false` was tested)
  - fix tests not properly cleaning up created processes (i.e. memory leak)
  - improve option description on `README`

ehmicky: Fixed.

I've also added some tests when the child process exists before the parent process, with the `cleanup` and `detached` options. This was not tested before.
ehmicky: Fixed

Patch:
diff --git a/fixtures/sub-process b/fixtures/sub-process
index e721378..727aa00 100755
--- a/fixtures/sub-process
+++ b/fixtures/sub-process
@@ -3,5 +3,6 @@
 const execa = require('..');
 
 const cleanup = process.argv[2] === 'true';
-const subprocess = execa('node', ['./fixtures/forever'], {cleanup});
+const detached = process.argv[3] === 'true';
+const subprocess = execa('node', ['./fixtures/forever'], {cleanup, detached});
 process.send(subprocess.pid);
diff --git a/fixtures/sub-process-exit b/fixtures/sub-process-exit
new file mode 100755
index 0000000..3ea8173
--- /dev/null
+++ b/fixtures/sub-process-exit
@@ -0,0 +1,7 @@
+#!/usr/bin/env node
+'use strict';
+const execa = require('..');
+
+const cleanup = process.argv[2] === 'true';
+const detached = process.argv[3] === 'true';
+execa('node', ['./fixtures/noop'], {cleanup, detached});
diff --git a/index.d.ts b/index.d.ts
index da9dc5a..08708a4 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -66,6 +66,11 @@ declare namespace execa {
 		/**
 		If `true`, runs `command` inside of a shell. Uses `/bin/sh` on UNIX and `cmd.exe` on Windows. A different shell can be specified as a string. The shell should understand the `-c` switch on UNIX or `/d /s /c` on Windows.
 
+		We recommend against using this option since it is:
+		- not cross-platform, encouraging shell-specific syntax.
+		- slower, because of the additional shell interpretation.
+		- unsafe, potentially allowing command injection.
+
 		@default false
 		*/
 		readonly shell?: boolean | string;
@@ -101,7 +106,9 @@ declare namespace execa {
 		readonly reject?: boolean;
 
 		/**
-		Keep track of the spawned process and `kill` it when the parent process exits.
+		Kill the spawned process when the parent process exits unless either:
+			- the spawned process is [`detached`](https://nodejs.org/api/child_process.html#child_process_options_detached)
+			- the parent process is terminated abruptly, for example, with `SIGKILL` as opposed to `SIGTERM` or a normal exit
 
 		@default true
 		*/
@@ -388,51 +395,6 @@ declare const execa: {
 	stderr(file: string, options?: execa.Options): Promise<string>;
 	stderr(file: string, options?: execa.Options<null>): Promise<Buffer>;
 
-	/**
-	Execute a command through the system shell.
-
-	Prefer `execa()` whenever possible, as it's both faster and safer.
-
-	@param command - The command to execute.
-	@returns A [`child_process` instance](https://nodejs.org/api/child_process.html#child_process_class_childprocess).
-
-	@example
-	```
-	import execa from 'execa';
-
-	(async => {
-		// Run a shell command
-		const {stdout} = await execa.shell('echo unicorns');
-		//=> 'unicorns'
-
-		// Catching an error
-		try {
-			await execa.shell('exit 3');
-		} catch (error) {
-			console.log(error);
-			//{
-			//	message: 'Command failed with exit code 3 (ESRCH): exit 3',
-			//	code: 3,
-			//	exitCode: 3,
-			//	exitCodeName: 'ESRCH',
-			//	stdout: '',
-			//	stderr: '',
-			//	all: '',
-			//	failed: true,
-			//	command: 'exit 3',
-			//	timedOut: false,
-			//	killed: false
-			//}
-		}
-	})();
-	```
-	*/
-	shell(command: string, options?: execa.Options): execa.ExecaChildProcess;
-	shell(
-		command: string,
-		options?: execa.Options<null>
-	): execa.ExecaChildProcess<Buffer>;
-
 	/**
 	Execute a file synchronously.
 
@@ -457,45 +419,6 @@ declare const execa: {
 		file: string,
 		options?: execa.SyncOptions<null>
 	): execa.ExecaSyncReturnValue<Buffer>;
-
-	/**
-	Execute a command synchronously through the system shell.
-
-	This method throws an `Error` if the command fails.
-
-	@param command - The command to execute.
-	@returns The same result object as [`child_process.spawnSync`](https://nodejs.org/api/child_process.html#child_process_child_process_spawnsync_command_args_options).
-
-	@example
-	```
-	import execa from 'execa';
-
-	try {
-		execa.shellSync('exit 3');
-	} catch (error) {
-		console.log(error);
-		//{
-		//	message: 'Command failed with exit code 3 (ESRCH): exit 3',
-		//	code: 3,
-		//	exitCode: 3,
-		//	exitCodeName: 'ESRCH',
-		//	stdout: '',
-		//	stderr: '',
-		//	failed: true,
-		//	command: 'exit 3',
-		//	timedOut: false
-		//}
-	}
-	```
-	*/
-	shellSync(
-		command: string,
-		options?: execa.Options
-	): execa.ExecaSyncReturnValue;
-	shellSync(
-		command: string,
-		options?: execa.Options<null>
-	): execa.ExecaSyncReturnValue<Buffer>;
 };
 
 export = execa;
diff --git a/index.js b/index.js
index 8088f76..de3f22b 100644
--- a/index.js
+++ b/index.js
@@ -93,11 +93,6 @@ function handleArgs(command, args, options = {}) {
 
 	options.stdio = stdio(options);
 
-	if (options.detached) {
-		// #115
-		options.cleanup = false;
-	}
-
 	if (process.platform === 'win32' && path.basename(command, '.exe') === 'cmd') {
 		// #116
 		args.unshift('/q');
@@ -127,10 +122,6 @@ function handleOutput(options, value) {
 	return value;
 }
 
-function handleShell(fn, command, options) {
-	return fn(command, {...options, shell: true});
-}
-
 function makeAllStream(spawned) {
 	if (!spawned.stdout && !spawned.stderr) {
 		return;
@@ -264,8 +255,9 @@ const execa = (command, args, options) => {
 		return Promise.reject(error);
 	}
 
+	// #115
 	let removeExitHandler;
-	if (parsed.options.cleanup) {
+	if (parsed.options.cleanup && !parsed.options.detached) {
 		removeExitHandler = onExit(() => {
 			spawned.kill();
 		});
@@ -446,8 +438,6 @@ module.exports.stderr = async (...args) => {
 	return stderr;
 };
 
-module.exports.shell = (command, options) => handleShell(execa, command, options);
-
 module.exports.sync = (command, args, options) => {
 	const parsed = handleArgs(command, args, options);
 	const joinedCommand = joinCommand(command, args);
@@ -483,5 +473,3 @@ module.exports.sync = (command, args, options) => {
 		timedOut: false
 	};
 };
-
-module.exports.shellSync = (command, options) => handleShell(execa.sync, command, options);
diff --git a/readme.md b/readme.md
index 971fa6d..6c2e31a 100644
--- a/readme.md
+++ b/readme.md
@@ -52,28 +52,25 @@ const execa = require('execa');
 	execa('echo', ['unicorns']).stdout.pipe(process.stdout);
 
 
-	// Run a shell command
-	const {stdout} = await execa.shell('echo unicorns');
-	//=> 'unicorns'
-
 	// Catching an error
 	try {
-		await execa.shell('exit 3');
+		await execa('wrong command');
 	} catch (error) {
 		console.log(error);
 		/*
 		{
-			message: 'Command failed with exit code 3 (ESRCH): exit 3',
-			code: 3,
-			exitCode: 3,
-			exitCodeName: 'ESRCH',
+			message: 'spawn wrong command ENOENT',
+			errno: 'ENOENT',
+			code: 'ENOENT',
+			syscall: 'spawn wrong command',
+			path: 'wrong command',
+			killed: false,
 			stdout: '',
 			stderr: '',
-			all: '',
 			failed: true,
-			command: 'exit 3',
-			timedOut: false,
-			killed: false
+			signal: null,
+			cmd: 'wrong command',
+			timedOut: false
 		}
 		*/
 	}
@@ -93,20 +90,16 @@ const execa = require('execa');
 
 // Catching an error with a sync method
 try {
-	execa.shellSync('exit 3');
+	execa.sync('wrong command');
 } catch (error) {
 	console.log(error);
 	/*
 	{
-		message: 'Command failed with exit code 3 (ESRCH): exit 3',
-		code: 3,
-		exitCode: 3,
-		exitCodeName: 'ESRCH',
-		stdout: '',
-		stderr: '',
-		failed: true,
-		command: 'exit 3',
-		timedOut: false
+		message: 'spawnSync wrong command ENOENT',
+		errno: 'ENOENT',
+		code: 'ENOENT',
+		syscall: 'spawnSync wrong command',
+		path: 'wrong command',
 	}
 	*/
 }
@@ -128,7 +121,7 @@ Arguments should not be escaped nor quoted. Exception: inside `command`, spaces
 
 Think of this as a mix of `child_process.execFile` and `child_process.spawn`.
 
-As opposed to [`execa.shell()`](#execashellcommand-options), no shell interpreter (Bash, `cmd.exe`, etc.) is used, so shell features such as variables substitution (`echo $PATH`) are not allowed.
+Unless the [`shell`](#shell) option is used, no shell interpreter (Bash, `cmd.exe`, etc.) is used, so shell features such as variables substitution (`echo $PATH`) are not allowed.
 
 Returns a [`child_process` instance](https://nodejs.org/api/child_process.html#child_process_class_childprocess) which is enhanced to be a `Promise`.
 
@@ -148,14 +141,6 @@ Same as `execa()`, but returns only `stdout`.
 
 Same as `execa()`, but returns only `stderr`.
 
-### execa.shell(command, [options])
-
-Execute a command through the system shell. Prefer `execa()` whenever possible, as it's faster, safer and more cross-platform.
-
-Returns a [`child_process` instance](https://nodejs.org/api/child_process.html#child_process_class_childprocess).
-
-The `child_process` instance is enhanced to also be promise for a result object with `stdout` and `stderr` properties.
-
 ### execa.sync(file, [arguments], [options])
 ### execa.sync(command, [options])
 
@@ -167,12 +152,6 @@ It does not have the `.all` property that `execa()` has because the [underlying
 
 This method throws an `Error` if the command fails.
 
-### execa.shellSync(command, [options])
-
-Execute a command synchronously through the system shell.
-
-Returns the same result object as [`child_process.spawnSync`](https://nodejs.org/api/child_process.html#child_process_child_process_spawnsync_command_args_options).
-
 ### options
 
 Type: `object`
@@ -236,6 +215,11 @@ Default: `false`
 
 If `true`, runs `command` inside of a shell. Uses `/bin/sh` on UNIX and `cmd.exe` on Windows. A different shell can be specified as a string. The shell should understand the `-c` switch on UNIX or `/d /s /c` on Windows.
 
+We recommend against using this option since it is:
+- not cross-platform, encouraging shell-specific syntax.
+- slower, because of the additional shell interpretation.
+- unsafe, potentially allowing command injection.
+
 #### stripFinalNewline
 
 Type: `boolean`<br>
@@ -277,7 +261,9 @@ Setting this to `false` resolves the promise with the error instead of rejecting
 Type: `boolean`<br>
 Default: `true`
 
-Keep track of the spawned process and `kill` it when the parent process exits.
+Kill the spawned process when the parent process exits unless either:
+	- the spawned process is [`detached`](https://nodejs.org/api/child_process.html#child_process_options_detached)
+	- the parent process is terminated abruptly, for example, with `SIGKILL` as opposed to `SIGTERM` or a normal exit
 
 #### encoding
 


Test patch:
diff --git a/index.test-d.ts b/index.test-d.ts
index e89d966..fbddbf5 100644
--- a/index.test-d.ts
+++ b/index.test-d.ts
@@ -128,15 +128,6 @@ expectType<Buffer>(await execa.stderr('unicorns', {encoding: null}));
 expectType<string>(await execa.stderr('unicorns', ['foo'], {encoding: 'utf8'}));
 expectType<Buffer>(await execa.stderr('unicorns', ['foo'], {encoding: null}));
 
-expectType<ExecaChildProcess<string>>(execa.shell('unicorns'));
-expectType<ExecaReturnValue<string>>(await execa.shell('unicorns'));
-expectType<ExecaReturnValue<string>>(
-	await execa.shell('unicorns', {encoding: 'utf8'})
-);
-expectType<ExecaReturnValue<Buffer>>(
-	await execa.shell('unicorns', {encoding: null})
-);
-
 expectType<ExecaSyncReturnValue<string>>(execa.sync('unicorns'));
 expectType<ExecaSyncReturnValue<string>>(
 	execa.sync('unicorns', {encoding: 'utf8'})
@@ -150,11 +141,3 @@ expectType<ExecaSyncReturnValue<string>>(
 expectType<ExecaSyncReturnValue<Buffer>>(
 	execa.sync('unicorns', ['foo'], {encoding: null})
 );
-
-expectType<ExecaSyncReturnValue<string>>(execa.shellSync('unicorns'));
-expectType<ExecaSyncReturnValue<string>>(
-	execa.shellSync('unicorns', {encoding: 'utf8'})
-);
-expectType<ExecaSyncReturnValue<Buffer>>(
-	execa.shellSync('unicorns', {encoding: null})
-);
diff --git a/test.js b/test.js
index d4356d9..c95b31d 100644
--- a/test.js
+++ b/test.js
@@ -137,11 +137,6 @@ test('trim string arguments', async t => {
 	t.is(stdout, 'foo\nbar');
 });
 
-test('execa.shell()', async t => {
-	const {stdout} = await execa.shell('node fixtures/noop foo');
-	t.is(stdout, 'foo');
-});
-
 test('execa.sync()', t => {
 	const {stdout} = execa.sync('noop', ['foo']);
 	t.is(stdout, 'foo');
@@ -165,23 +160,6 @@ test('skip throwing when using reject option in execa.sync()', t => {
 	t.is(typeof error.stderr, 'string');
 });
 
-test('execa.shellSync()', t => {
-	const {stdout} = execa.shellSync('node fixtures/noop foo');
-	t.is(stdout, 'foo');
-});
-
-test('execa.shellSync() includes stdout and stderr in errors for improved debugging', t => {
-	t.throws(() => {
-		execa.shellSync('node fixtures/error-message.js');
-	}, {message: STDERR_STDOUT_REGEXP, code: 1});
-});
-
-test('skip throwing when using reject option in execa.shellSync()', t => {
-	const error = execa.shellSync('node fixtures/error-message.js', {reject: false});
-	t.is(typeof error.stdout, 'string');
-	t.is(typeof error.stderr, 'string');
-});
-
 test('stripEof option (legacy)', async t => {
 	const {stdout} = await execa('noop', ['foo'], {stripEof: false});
 	t.is(stdout, 'foo\n');
@@ -485,8 +463,19 @@ test(command, ' foo bar', 'foo', 'bar');
 test(command, ' baz quz', 'baz', 'quz');
 test(command, '');
 
-async function spawnAndKill(t, signal, cleanup, isKilled) {
-	const subprocess = execa('sub-process', [cleanup], {stdio: ['ignore', 'ignore', 'ignore', 'ipc']});
+// When child process exits before parent process
+async function spawnAndExit(t, cleanup, detached) {
+	await t.notThrowsAsync(execa('sub-process-exit', [cleanup, detached]));
+}
+
+test('spawnAndExit', spawnAndExit, false, false);
+test('spawnAndExit cleanup', spawnAndExit, true, false);
+test('spawnAndExit detached', spawnAndExit, false, true);
+test('spawnAndExit cleanup detached', spawnAndExit, true, true);
+
+// When parent process exits before child process
+async function spawnAndKill(t, signal, cleanup, detached, isKilled) {
+	const subprocess = execa('sub-process', [cleanup, detached], {stdio: ['ignore', 'ignore', 'ignore', 'ipc']});
 
 	const pid = await pEvent(subprocess, 'message');
 	t.true(Number.isInteger(pid));
@@ -498,6 +487,10 @@ async function spawnAndKill(t, signal, cleanup, isKilled) {
 
 	t.false(isRunning(subprocess.pid));
 	t.is(isRunning(pid), !isKilled);
+
+	if (!isKilled) {
+		process.kill(pid, 'SIGKILL');
+	}
 }
 
 // Without `options.cleanup`:
@@ -507,17 +500,14 @@ async function spawnAndKill(t, signal, cleanup, isKilled) {
 // With `options.cleanup`, subprocesses are always killed
 //   - `options.cleanup` with SIGKILL is a noop, since it cannot be handled
 const exitIfWindows = process.platform === 'win32';
-test('cleanup true - SIGTERM', spawnAndKill, 'SIGTERM', 'true', true);
-test('cleanup false - SIGTERM', spawnAndKill, 'SIGTERM', 'false', exitIfWindows);
-test('cleanup true - SIGKILL', spawnAndKill, 'SIGKILL', 'true', exitIfWindows);
-test('cleanup false - SIGKILL', spawnAndKill, 'SIGKILL', 'false', exitIfWindows);
-
-test('execa.shell() supports the `shell` option', async t => {
-	const {stdout} = await execa.shell('node fixtures/noop foo', {
-		shell: process.platform === 'win32' ? 'cmd.exe' : '/bin/bash'
-	});
-	t.is(stdout, 'foo');
-});
+test('spawnAndKill SIGTERM', spawnAndKill, 'SIGTERM', false, false, exitIfWindows);
+test('spawnAndKill SIGKILL', spawnAndKill, 'SIGKILL', false, false, exitIfWindows);
+test('spawnAndKill cleanup SIGTERM', spawnAndKill, 'SIGTERM', true, false, true);
+test('spawnAndKill cleanup SIGKILL', spawnAndKill, 'SIGKILL', true, false, exitIfWindows);
+test('spawnAndKill detached SIGTERM', spawnAndKill, 'SIGTERM', false, true, false);
+test('spawnAndKill detached SIGKILL', spawnAndKill, 'SIGKILL', false, true, false);
+test('spawnAndKill cleanup detached SIGTERM', spawnAndKill, 'SIGTERM', true, true, false);
+test('spawnAndKill cleanup detached SIGKILL', spawnAndKill, 'SIGKILL', true, true, false);
 
 if (process.platform !== 'win32') {
 	test('write to fast-exit process', async t => {
@@ -559,6 +549,18 @@ test('do not extend environment with `extendEnv: false`', async t => {
 	]);
 });
 
+test('can use `options.shell: true`', async t => {
+	const {stdout} = await execa('node fixtures/noop foo', {shell: true});
+	t.is(stdout, 'foo');
+});
+
+test('can use `options.shell: string`', async t => {
+	const {stdout} = await execa('node fixtures/noop foo', {
+		shell: process.platform === 'win32' ? 'cmd.exe' : '/bin/bash'
+	});
+	t.is(stdout, 'foo');
+});
+
 test('use extend environment with `extendEnv: true` and `shell: true`', async t => {
 	process.env.TEST = 'test';
 	const command = process.platform === 'win32' ? 'echo %TEST%' : 'echo $TEST';