search

ArtskydJ / node-sox-stream

:mega: A stream-friendly wrapper around SoX
53 stars 14 forks source link

Emit finished event when writable is finished #6

Closed rijnhard closed 7 years ago

rijnhard commented 7 years ago

I carry the finish event from sox to the duplex stream, it should do this according to to the streams spec. I assume there may be other events as well but this is the most useful for determining when it's complete and successful.

I haven't investigated tests yet, but have tested this in an actual project

ArtskydJ commented 7 years ago

Thanks for the PR! A few things I'd like before merging:

  1. Fix lines 30 and 46 to be indented with tabs.
  2. Add a test that fails with the old code but works with the new.

If you don't get around to these, then I'll fix them when I can. (Hopefully within a week.)

rijnhard commented 7 years ago

Hi I've added test coverage. Will push soon. But I have some questions. Firstly why write to a temp file and not just pipe to sox directly? Secondly I did a test now and both soxOutput and sox.stdout will output a finish event, the former coming first. I'm not sure which to go with. the former would be fine I think for the event itself, but not for the cleanup.

ArtskydJ commented 7 years ago

Why write to a temp file and not just pipe to sox directly?

Good question. I originally just piped out of SoX, but it started throwing weird errors with certain output types. I wish it didn't need the temp file.

soxOutput emits finish before sox.stdout emits finish

Maybe it's because of this: https://github.com/ArtskydJ/create-temp-file/blob/master/index.js#L12-L13

If you believe it's a problem, then you or I can open an issue on that repo.

I've added test coverage

Thanks! The build passed, so I'm merging it now.

ArtskydJ commented 7 years ago

Published as v2.0.1.

rijnhard commented 7 years ago

Good question. I originally just piped out of SoX, but it started throwing weird errors with certain output types. I wish it didn't need the temp file.

I tried digging around and playing with the code for most of today, and granted I'm not exactly a master of streams, I couldn't get it working, I know you can pipe to sox in terminal, and I've seen python code where they have successfully done that, but node's spawn was giving me a lot of crap, because it wants a file descriptor for stdin so I gave up, my experience with this is just too limited at the moment. I was hoping memory-streams or something could be used, but the problem is with spawn and sox.

Thank you for merging. One thing I will say, I think there may be more events that may need to be carried, if I look at a lib like process-streams it gets hairy fast. But it doesnt stop my use case from working.

ArtskydJ commented 7 years ago

Hmm, it looks like finish is for writable stream. The stream that is returned from sox is a readable stream.

Ideally you would be watching for the close event when you're waiting for a readable stream to end.