Repository: sindresorhus/execa
Pull request #360 (2019-08-09 08:56:09)

Fix errors being thrown when `detached: true` or `cleanup: false` is used
Using either the `detached: true` and `cleanup: false` always throws an error:

```js
> await execa('echo',{cleanup: false})
Thrown:
TypeError: Cannot destructure property `error` of 'undefined' or 'null'.
    at handlePromise (/home/ether/code/execa/index.js:107:10)
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
> await execa('echo',{detached: true})
Thrown:
TypeError: Cannot destructure property `error` of 'undefined' or 'null'.
    at handlePromise (/home/ether/code/execa/index.js:107:10)
    at processTicksAndRejections (internal/process/task_queues.js:85:5)
```

This was introduced by one my PRs [here](https://github.com/sindresorhus/execa/pull/337/files#diff-afe5430c779eb982786eccb39864cea0R86).

This is a quite critical bug, we should release this. However the current code contains breaking changes: users of the `all` property must now use the `all: true` option. So this would be a major release.

Additionally some tests that check `t.throwsAsync()` were incorrect but previously successful due to that bug being present (since it made `execa` fail but for the wrong reason). I have fixed those tests (mostly Windows issues).

sindresorhus: > This is a quite critical bug, we should release this. However the current code contains breaking changes: users of the all property must now use the all: true option. So this would be a major release.

I don't think we should release the `all` change yet. I would prefer to resolve https://github.com/sindresorhus/execa/pull/351 first. What we could do is to create a branch based on v2.0.3, apply the one-line fix, and do a v2.0.4 release to an alternative dist-tag. ?

Patch:
diff --git a/lib/kill.js b/lib/kill.js
index c23d6a5..dc1c77c 100644
--- a/lib/kill.js
+++ b/lib/kill.js
@@ -85,7 +85,7 @@ const setupTimeout = (spawned, {timeout, killSignal = 'SIGTERM'}, spawnedPromise
 // `cleanup` option handling
 const setExitHandler = (spawned, {cleanup, detached}, timedPromise) => {
 	if (!cleanup || detached) {
-		return;
+		return timedPromise;
 	}
 
 	const removeExitHandler = onExit(() => {


Test patch:
diff --git a/test/fixtures/sub-process-exit b/test/fixtures/sub-process-exit
index 2b350a2..ea05f0e 100755
--- a/test/fixtures/sub-process-exit
+++ b/test/fixtures/sub-process-exit
@@ -4,4 +4,14 @@ const execa = require('../..');
 
 const cleanup = process.argv[2] === 'true';
 const detached = process.argv[3] === 'true';
-execa('node', ['./test/fixtures/noop'], {cleanup, detached});
+
+const runChild = async () => {
+	try {
+		await execa('node', ['./test/fixtures/noop'], {cleanup, detached});
+	} catch (error) {
+		console.error(error);
+		process.exit(1);
+	}
+};
+
+runChild();
diff --git a/test/stream.js b/test/stream.js
index fc388ad..c0f8ba7 100644
--- a/test/stream.js
+++ b/test/stream.js
@@ -172,15 +172,18 @@ test('buffer: false > promise rejects when process returns non-zero', async t =>
 
 const BUFFER_TIMEOUT = 1e3;
 
-test.serial('buffer: false > promise does not resolve when output is big and is not read', async t => {
-	const {timedOut} = await t.throwsAsync(execa('max-buffer', {buffer: false, timeout: BUFFER_TIMEOUT}));
-	t.true(timedOut);
-});
+// On Unix (not Windows), a process won't exit if stdout has not been read.
+if (process.platform !== 'win32') {
+	test.serial('buffer: false > promise does not resolve when output is big and is not read', async t => {
+		const {timedOut} = await t.throwsAsync(execa('max-buffer', {buffer: false, timeout: BUFFER_TIMEOUT}));
+		t.true(timedOut);
+	});
 
-test.serial('buffer: false > promise does not resolve when output is big and "all" is used but not read', async t => {
-	const cp = execa('max-buffer', {buffer: false, all: true, timeout: BUFFER_TIMEOUT});
-	cp.stdout.resume();
-	cp.stderr.resume();
-	const {timedOut} = await t.throwsAsync(cp);
-	t.true(timedOut);
-});
+	test.serial('buffer: false > promise does not resolve when output is big and "all" is used but not read', async t => {
+		const cp = execa('max-buffer', {buffer: false, all: true, timeout: BUFFER_TIMEOUT});
+		cp.stdout.resume();
+		cp.stderr.resume();
+		const {timedOut} = await t.throwsAsync(cp);
+		t.true(timedOut);
+	});
+}
diff --git a/test/test.js b/test/test.js
index 90089cf..96a4a71 100644
--- a/test/test.js
+++ b/test/test.js
@@ -74,24 +74,22 @@ test('stripFinalNewline in sync mode on failure', t => {
 });
 
 test('preferLocal: true', async t => {
-	await t.notThrowsAsync(execa('ava', ['--version'], {preferLocal: true, env: {PATH: ''}}));
+	await t.notThrowsAsync(execa('ava', ['--version'], {preferLocal: true, env: {Path: '', PATH: ''}}));
 });
 
 test('preferLocal: false', async t => {
-	await t.throwsAsync(execa('ava', ['--version'], {preferLocal: false, env: {PATH: ''}}), ENOENT_REGEXP);
+	await t.throwsAsync(execa('ava', ['--version'], {preferLocal: false, env: {Path: '', PATH: ''}}), ENOENT_REGEXP);
 });
 
 test('preferLocal: undefined', async t => {
-	await t.throwsAsync(execa('ava', ['--version'], {env: {PATH: ''}}), ENOENT_REGEXP);
+	await t.throwsAsync(execa('ava', ['--version'], {env: {Path: '', PATH: ''}}), ENOENT_REGEXP);
 });
 
 test('localDir option', async t => {
 	const command = process.platform === 'win32' ? 'echo %PATH%' : 'echo $PATH';
 	const {stdout} = await execa(command, {shell: true, preferLocal: true, localDir: '/test'});
-	const envPaths = stdout.split(path.delimiter).map(envPath =>
-		envPath.replace(/\\/g, '/').replace(/^[^/]+/, '')
-	);
-	t.true(envPaths.some(envPath => envPath === '/test/node_modules/.bin'));
+	const envPaths = stdout.split(path.delimiter);
+	t.true(envPaths.some(envPath => envPath.endsWith('.bin')));
 });
 
 test('stdin errors are handled', async t => {