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 execa.stdout() shortcut #14

Merged
merged 1 commit into from
Apr 25, 2016
Merged

add execa.stdout() shortcut #14

merged 1 commit into from
Apr 25, 2016

Conversation

sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Apr 22, 2016

Not sure about this one. The thinking is that stdout is the most common use-case so might be nice to have a shortcut for it. This is not about the amount of characters, as that's mostly the same, but rather clarity.

Example:

test(async t => {
    t.is((await execa('./cli.js')).stdout, 'foo');
});
test(async t => {
    t.is(await execa.stdout('./cli.js'), 'foo');
});

You can see how the second one is more readable because of less parens.

@kevva @SamVerschueren @jamestalmage @novemberborn Could use your opion 🙌

@jamestalmage
Copy link
Contributor

The one advantage to the first, is that power-assert will show you the output of stderr in the graph. If you do this, you might as well create the equivalent stderr shortcut. Just so it's easy to flip a test to print stderr instead of stdout without reworking your call structure.

@sindresorhus
Copy link
Owner Author

Sure, I guess I could add that, but the important question is; Do you think this is a valid addition?

@kevva
Copy link
Contributor

kevva commented Apr 22, 2016

Regardless of the parens, I actually don't think one way is more clearer than the other. The first example is how I would expect it to work, the latter one would just be a nice shortcut. I could live without it.

@jamestalmage
Copy link
Contributor

I don't think it's that helpful, but I haven't written a lot of tests using it.

@novemberborn
Copy link

Alternatively change the execa return value to be a decorated thenable, e.g. add a stdout getter that returns a promise:

test(async t => {
    t.is(await execa('./cli.js').stdout, 'foo');
});

@jamestalmage
Copy link
Contributor

I think Mark's idea is the best so far.

@jamestalmage
Copy link
Contributor

Note: it needs to be implemented as a getter that defers creation of additional promises so you don't end up with unhandled rejections.

@sindresorhus
Copy link
Owner Author

sindresorhus commented Apr 23, 2016

I think Mark's idea is the best so far.

Why is it better?

Alternatively change the execa return value to be a decorated thenable, e.g. add a stdout getter that returns a promise

What if you want both stdout and stderr at the same time? Not easily doable, unless I'm missing how this will work.

@jamestalmage
Copy link
Contributor

Why is it better?

Subjective, I like the trailing .stdout - it reads like synchronous code.

What if you want both stdout and stderr at the same time?

both:

const {stdout, stderr} = await execa('./cli');

Just one:

const stdout = await execa('./cli').stdout;

Or maybe just leave it as is:

const {stdout} = await execa('./cli');

@sindresorhus
Copy link
Owner Author

How can const stdout = await execa('./cli').stdout; both be a string and promise?

@jamestalmage
Copy link
Contributor

It can't - it would always be a promise.

@sindresorhus
Copy link
Owner Author

@jamestalmage But, then your example wouldn't work:

const {stdout} = await execa('./cli');

stdout is a string here.

@jamestalmage
Copy link
Contributor

Both would work - the await is evaluated before the assignment:

const {stdout} = await execa('./cli');
// =>
const {stdout} = await Promise.resolve({stdout:string, stderr: string});
// =>
const {stdout} = {stdout: string, stderr: string};
// => 
const stdout = string

and

const stdout = await exec('./cli').stdout;
// =>
const stdout = await Promise.resolve(string);
// =>
const stdout = string;

jamestalmage added a commit to jamestalmage/execa that referenced this pull request Apr 23, 2016
@jamestalmage
Copy link
Contributor

See jamestalmage@70629c6#diff-1dd241c4cd3fd1dd89c570cee98b79ddR115

It can definitely work both ways. Still, not sure it's the best idea

@jamestalmage
Copy link
Contributor

My new preference is to merge this, and #18.

#17 was a fun idea, but it conflicts with #18, which I think is more important.

@sindresorhus
Copy link
Owner Author

So if we're going with this it would be execa.stdout() and execa.stderr(). I'm not so sure anymore it's worth the extra API bloat and potential confusion. We now have two very similar ways to get stdout/stderr.

@jamestalmage
Copy link
Contributor

jamestalmage commented Apr 23, 2016

One potential advantage: #18 (comment)

Ignoring the stream you don't care about improves memory efficiency.

@sindresorhus
Copy link
Owner Author

Another argument for adding this. When returning the result of execa and you only want stdout you currently have to do:

return execa('bin').then(result => result.stdout);

This would be nicer:

return execa.stdout('bin');

@sindresorhus
Copy link
Owner Author

@jamestalmage Ready for review.

@@ -123,6 +123,20 @@ module.exports = function (cmd, args, opts) {
return spawned;
};

module.exports.stdout = function () {
// TODO: set `stderr: 'ignore'` when that option is implemented
Copy link
Owner Author

Choose a reason for hiding this comment

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

Deferring this until #24 is done.

@sindresorhus sindresorhus merged commit 868f505 into master Apr 25, 2016
@sindresorhus sindresorhus deleted the stdout-alias branch April 25, 2016 07:47
@sindresorhus
Copy link
Owner Author

Thanks for the great feedback :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants