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

Better stdio option handling - fixes #24 #80

Merged
merged 4 commits into from
Mar 24, 2017
Merged

Better stdio option handling - fixes #24 #80

merged 4 commits into from
Mar 24, 2017

Conversation

SamVerschueren
Copy link
Contributor

Cherry picked from #36 (Thanks @jamestalmage for the initial work!).

Also fixed some issues regarding #79 as it didn't support stdio to being an Array.

Feel free to provide feedback on anything, not sure about the docs either.

readme.md Outdated
Type: `string`<br>
Default: `pipe`

Configure the `stderr` pipe.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should link to the relevant Node docs and provide a reminder of the available options: https://nodejs.org/dist/latest-v6.x/docs/api/child_process.html#child_process_options_stdio

We should also allow positive integers, undefined, and null.

@SamVerschueren
Copy link
Contributor Author

SamVerschueren commented Mar 22, 2017

Made a little change to the docs, is this better? Or should we do something like

stdin

Same options as stdio. Shorter and more descriptive form for stdio: ['inherit', null, null].

@jamestalmage
Copy link
Contributor

That works. Do we have tests making sure we cover the number and stream scenarios?

@SamVerschueren
Copy link
Contributor Author

I will have a look tomorrow to cover that. Any ideas how we could test the streams?

@jamestalmage
Copy link
Contributor

Hmmm.

Using streams with child_process does not work how I thought. You can actually only pass native like streams (net / dgram / tty, etc).

I am working on a module to reliably detect those types of streams. (stealing from the code here).

If they pass a userland stream, I think we can still accommodate, but it's more of a PITA.

@jamestalmage
Copy link
Contributor

Just go with this for now. We can discuss providing more user friendly stream handling later

@SamVerschueren
Copy link
Contributor Author

Added 2 more types of tests

  1. Writing content to a file via a file descriptor fs.openSync('file.txt', 'w');
  2. Add tests to see if parsing numbers works. This wasn't working as expected so I fixed that

I was looking into the stream thing before I read your comment. From the docs

Share a readable or writable stream that refers to a tty, file, socket, or a pipe with the child process.

So I was expecting I could do something like this

const writeStream = fs.createWriteStream('file.txt');

await execa('foo', {stdout: writeStream});

But as you figured out, that is not possible.

test('pass `stderr` to a file descriptor', async t => {
const file = tempfile('.txt');
await m('fixtures/noop-err', ['foo bar'], {stderr: fs.openSync(file, 'w')});
t.is(fs.readFileSync(file, 'utf8'), 'foo bar\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Good test strategy. File streams are allowed. I opened an issue to handle userland streams better

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, I missed your comment. So this is just a descriptor, not a stream. The way child_process handles streams just gets weirder and weirder

Copy link
Contributor Author

@SamVerschueren SamVerschueren Mar 23, 2017

Choose a reason for hiding this comment

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

Yes exactly. Either the docs are wrong, or I did it wrong. But the docs link to Stream which states

screen shot 2017-03-23 at 19 32 53

Incorrect value for stdio stream: WriteStream

But but... Ugh, nevermind.

@jamestalmage
Copy link
Contributor

I like it. Let's merge

@jamestalmage jamestalmage merged commit 9cd5e2a into master Mar 24, 2017
@jamestalmage jamestalmage deleted the iss24 branch March 24, 2017 15:55
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.

2 participants