Add all property to returned object

[sotojuan]

This fixes #1 by returning an additional all property that is just the stderr and stdout together, updating the readme, and adding two tests that test this feature.

[sindresorhus]

Ok, I should have been more clear in the original issue. It's not that simple. The user could already do this themselves. What I'm after is the combination of stdout and stderr as it's outputted, just like what you would see in your terminal.

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

Would result in 132 your way. I want 123.

Will probably have to spawn and gather it manually.

[sotojuan]

I was wondering why you said you didn't have enough time to do this! I'll look into it. Thanks for clearing it up.

[sotojuan]

All right, so I use spawn and then listen on data events in stdout and stderr and add each to a string for stdout, stderr, and all respectively. I got it, but I am trying to think of something I can use to test it. For some reason if I do echo good; ls x; echo good for example (assuming x does not exist), the two echos get put together, with the error from ls put last.

However, if I do echo good; ls x; ls . then it works, with the error from ls x in the middle and the result from ls . put last.

I am not sure if this is an error in my implementation, but let me know!

[sindresorhus]

but I am trying to think of something I can use to test it

Just create a fixture.js file that contains my above code example.

[sotojuan]

All right, so this passes all the tests and seems to work, but (IMO) probably could be refactored/cleaned up.

spawn does not seem to have an option for encoding, so all the outputs are saved as strings and if the user set an encoding (anything not undefined) the string is converted into a buffer with that encoding. I did this in the handle function. I tried using buffers instead of strings but it got messy/wasn't working.

A lot of stuff I did was new to me (I learned stuff at least!), so let me know what you think.

[sindresorhus]

spawn does not seem to have an option for encoding

You use e.g. stdout.setEncoding() on the streams instead.

[sindresorhus]

Using .toString() is a common mistake in Node.js, but this will totally break on unicode where one character might be split between different data chunks. Instead use .setEncoding() upfront.

[jamestalmage]

Aren't you supposed to use string-decoder for that? Or is that what setEncoding uses under the hood?

[sindresorhus]

@jamestalmage You could. I think it uses it under the hood, but .setEncoding() is the correct way regardless.

[jamestalmage]

http://stackoverflow.com/a/9046164

[sindresorhus]

Yup. I blame the Node.js docs. They could be a lot clearer. Especially since it's easy to do it wrong and would cause subtle bugs.

[sotojuan]

Hey Sindre, so my old all branch of my fork somehow disappeared/got messed up, so uh I just started with a fresh one based on latest master and forced push. Probably a bad thing to do but the reason I am commenting is because apparently for some reason the all output works. It works on the tests, tried it with external files and the fixture, and even shell commands. Not sure why since it's the same code.

[sindresorhus]

Force push is fine, and often useful, on non-master branches. Your changes does not yet address all the previous inline feedback.

[sotojuan]

Sure, was just explaining the force push! I'll get to work on the rest of the feedback. Thanks!

[sotojuan]

All right, I took care of some of the smaller things. The all works when running a file but not when doing a shell command (echo at least, passes with others). Still investigating that.

Also, can you clarify on what I should do about this?

[jamestalmage]

https://www.npmjs.com/package/merge-stream

[sotojuan]

Ha, I had forgotten about this PR. Yeah there's probably a better way to do this now. Might be worth to just close this one and I (or someone else?) can work with the new proposed solutions.