Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add .all property with interleaved stdout and stderr #171

Merged
merged 19 commits into from
Mar 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
6c46865
Added result.all intermixed stdout/stderr (concat for sync)
tomsotte Jan 20, 2019
4069b17
Removed `all` stream from execa.sync(); fix creating mixed stream for…
tomsotte Mar 7, 2019
97cde71
Added `all` stream documentation and why it's not possible in execa.s…
tomsotte Mar 7, 2019
23799a0
More predictable noop-132 fixture for testing `all` stream output
tomsotte Mar 7, 2019
40aa251
Merge with upstream/master; fix rename `m` -> `execa` in test.js
tomsotte Mar 8, 2019
3832328
Merge branch 'master' into iss1
tomsotte Mar 8, 2019
e35dcdf
Fixed trailing space
tomsotte Mar 8, 2019
7f430d9
Using test.serial for testing result.all with lower timeout for noop-…
tomsotte Mar 9, 2019
b500215
Added execa.all(), similar to execa.stdout() for result.all; added test
tomsotte Mar 9, 2019
a419b4a
Fixed link to Node.js docs about process I/O stream
tomsotte Mar 9, 2019
bc1450a
all available on errors, if available in result (ex. no execa.sync())
tomsotte Mar 9, 2019
29f3f12
Added all interleaved stream to readme in Why, example and API
tomsotte Mar 9, 2019
43a43bd
Removed unnecessary comments on noop-132 and test.js
tomsotte Mar 10, 2019
dc68492
Fixed and improved readme about all interleaved stream
tomsotte Mar 10, 2019
360bdb4
Removed execa.all()
tomsotte Mar 10, 2019
06fbfff
Update readme.md
sindresorhus Mar 10, 2019
e128464
Update readme.md
sindresorhus Mar 10, 2019
e842c72
Merge branch 'master' into iss1
sindresorhus Mar 10, 2019
b815a79
Update readme.md
sindresorhus Mar 10, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions fixtures/noop-132
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#!/usr/bin/env node
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not test interleaving because stderr is not in-between two stdout calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was my mistake, I should have left it in-between and let the test fail. As I said in the comments I was testing various way to implement the test to be correct but I couldn't find one for all (more on this on the next conversation).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I think it might be a good idea to do a more complex interleaving like stdout > stderr > stderr > stdout > stderr > stdout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it needs to be more complex, if it works just once we should expect to interleave the rest of the outputs. Otherwise we know it's just concatenating the output strings.
From what I understand by the tests I'm doing is that we cannot guarantee the order of the interleaving.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think @sindresorhus?

'use strict';

process.stdout.write('1');
process.stderr.write('3');

setTimeout(() => {
process.stdout.write('2');
}, 1000);
34 changes: 33 additions & 1 deletion index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const stripFinalNewline = require('strip-final-newline');
const npmRunPath = require('npm-run-path');
const isStream = require('is-stream');
const _getStream = require('get-stream');
const mergeStream = require('merge-stream');
const pFinally = require('p-finally');
const onExit = require('signal-exit');
const errname = require('./lib/errname');
Expand Down Expand Up @@ -91,6 +92,24 @@ function handleShell(fn, command, options) {
return fn(command, {...options, shell: true});
}

function makeAllStream(spawned) {
if (!spawned.stdout && !spawned.stderr) {
return null;
}

const mixed = mergeStream();

if (spawned.stdout) {
mixed.add(spawned.stdout);
}

if (spawned.stderr) {
mixed.add(spawned.stderr);
}

return mixed;
}

function getStream(process, stream, {encoding, buffer, maxBuffer}) {
if (!process[stream]) {
return null;
Expand Down Expand Up @@ -146,6 +165,10 @@ function makeError(result, options) {
error.cmd = joinedCommand;
error.timedOut = Boolean(timedOut);

if ('all' in result) {
sindresorhus marked this conversation as resolved.
Show resolved Hide resolved
error.all = result.all;
}

return error;
}

Expand Down Expand Up @@ -263,17 +286,23 @@ module.exports = (command, args, options) => {
if (spawned.stderr) {
spawned.stderr.destroy();
}

if (spawned.all) {
spawned.all.destroy();
}
}

// TODO: Use native "finally" syntax when targeting Node.js 10
const handlePromise = () => pFinally(Promise.all([
processDone,
getStream(spawned, 'stdout', {encoding, buffer, maxBuffer}),
getStream(spawned, 'stderr', {encoding, buffer, maxBuffer})
getStream(spawned, 'stderr', {encoding, buffer, maxBuffer}),
getStream(spawned, 'all', {encoding, buffer, maxBuffer: maxBuffer * 2})
]).then(results => { // eslint-disable-line promise/prefer-await-to-then
const result = results[0];
result.stdout = results[1];
result.stderr = results[2];
result.all = results[3];

if (result.error || result.code !== 0 || result.signal !== null) {
const error = makeError(result, {
Expand All @@ -297,6 +326,7 @@ module.exports = (command, args, options) => {
return {
stdout: handleOutput(parsed.options, result.stdout),
stderr: handleOutput(parsed.options, result.stderr),
all: handleOutput(parsed.options, result.all),
code: 0,
exitCode: 0,
exitCodeName: 'SUCCESS',
Expand All @@ -312,6 +342,8 @@ module.exports = (command, args, options) => {

handleInput(spawned, parsed.options.input);

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);
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"cross-spawn": "^6.0.0",
"get-stream": "^4.0.0",
"is-stream": "^1.1.0",
"merge-stream": "1.0.1",
"npm-run-path": "^2.0.0",
"p-finally": "^1.0.0",
"signal-exit": "^3.0.0",
Expand Down
10 changes: 9 additions & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- Higher max buffer. 10 MB instead of 200 KB.
- [Executes locally installed binaries by name.](#preferlocal)
- [Cleans up spawned processes when the parent process dies.](#cleanup)
- [Adds an `.all` property](#execafile-arguments-options) with interleaved output from `stdout` and `stderr`, similar to what the terminal sees. [*(Async only)*](#execasyncfile-arguments-options)


## Install
Expand Down Expand Up @@ -65,6 +66,7 @@ const execa = require('execa');
exitCodeName: 'ESRCH',
stdout: '',
stderr: '',
all: '',
failed: true,
signal: null,
cmd: 'exit 3',
Expand Down Expand Up @@ -106,7 +108,11 @@ Execute a file.

Think of this as a mix of `child_process.execFile` and `child_process.spawn`.

Returns a [`child_process` instance](https://nodejs.org/api/child_process.html#child_process_class_childprocess), which is enhanced to also be a `Promise` for a result `Object` with `stdout` and `stderr` properties.
Returns a [`child_process` instance](https://nodejs.org/api/child_process.html#child_process_class_childprocess) which is enhanced to be a `Promise`.

It exposes an additional `.all` stream, with `stdout` and `stderr` interleaved.

The promise result is an `Object` with `stdout`, `stderr` and `all` properties.

### execa.stdout(file, [arguments], [options])

Expand All @@ -130,6 +136,8 @@ Execute a file synchronously.

Returns the same result object as [`child_process.spawnSync`](https://nodejs.org/api/child_process.html#child_process_child_process_spawnsync_command_args_options).

It does not have the `.all` property that `execa()` has because the [underlying synchronous implementation](https://nodejs.org/api/child_process.html#child_process_child_process_execfilesync_file_args_options) only returns `stdout` and `stderr` at the end of the execution, so they cannot be interleaved.

This method throws an `Error` if the command fails.

### execa.shellSync(file, [options])
Expand Down
14 changes: 11 additions & 3 deletions test.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,16 @@ test('execa.stderr()', async t => {
t.is(stderr, 'foo');
});

test('stdout/stderr available on errors', async t => {
test.serial('result.all shows both `stdout` and `stderr` intermixed', async t => {
const result = await execa('noop-132');
t.is(result.all, '132');
});

test('stdout/stderr/all available on errors', async t => {
const err = await t.throwsAsync(execa('exit', ['2']), {message: getExitRegExp('2')});
t.is(typeof err.stdout, 'string');
t.is(typeof err.stderr, 'string');
t.is(typeof err.all, 'string');
});

test('include stdout and stderr in errors for improved debugging', async t => {
Expand Down Expand Up @@ -234,7 +240,8 @@ test('do not buffer stdout when `buffer` set to `false`', async t => {
const promise = execa('max-buffer', ['stdout', '10'], {buffer: false});
const [result, stdout] = await Promise.all([
promise,
getStream(promise.stdout)
getStream(promise.stdout),
getStream(promise.all)
]);

t.is(result.stdout, undefined);
Expand All @@ -245,7 +252,8 @@ test('do not buffer stderr when `buffer` set to `false`', async t => {
const promise = execa('max-buffer', ['stderr', '10'], {buffer: false});
const [result, stderr] = await Promise.all([
promise,
getStream(promise.stderr)
getStream(promise.stderr),
getStream(promise.all)
]);

t.is(result.stderr, undefined);
Expand Down