sindresorhus
Duplex stream
No need to implement writev, it's optional method that is here for optimization purposes if you can handle multiple chunks at once, which we can't since we just pass them to process stdin.
Do you have an idea on what other tests I could make ?
Unless someone can lend me a hand, it will take some times for me to track the test failures on windows ; I don't have access to a windows machine right now.
Okay, found why some tests where failing on Windows and not Linux, but that raises more questions @sindresorhus
It’s on unit tests testing this code path : https://github.com/sloonz/execa/blob/28ac048159f6ea1fa2c27499451c014a74e80265/index.js#L268-L287
I tried to trigger an exception on line 266 by passing null to file. This works as intended on Linux ; however, on Windows, the exception gets triggered earlier (at line 261, handleArgs), and the exception directly unwind to the caller instead of being caught and handled.
Two questions :
- How can I consistently trigger an exception on
childProcess.spawn? - I have not tested, but my problem most likely is also here for the main
execa()function, whereexeca(null)returns an error (which doubles as a rejected promise) on Linux whereas it just throws on Windows. Is this the intended behavoir ? Should I fill a bug for this ?
I think the feature is in a good spot for review/possible integration in the current iteration ; should I squash the commits ?
I would like to see even more tests for this. It's a complicated feature (streams have a lot of edge-cases).
Sorry for the hiatus.
New iteration of the branch is there.
No new test because my dumb brain can't generate any idea of what to test that isn't currently covered. Do you have any suggestion ?
In general for PRs:
- Look over your own diff.
- Try to improve the implementation.
- Try to refactor and simplify.
- Improve docs.
I would like your opinion on something.
I planned to refactor in two parts, but now I wonder if the second part is needed.
In the previous implementation the result of duplexStream was both a duplex stream AND and execa object (spawned process object + promise). Now I wonder if this is a good idea, or if the result of duplexStream should not be just a stream and don't return the spawned process or a promise at all
Pros: way simpler code and interface Cons: can't wait for the process to be killed after a failure in the pipeline (a kill signal is sent if the pipeline fails, but with just the duplex stream you have no way of waiting for the process to actually have ended) ; can't get the exit code (or really any information) of the process after a failure in the pipeline. I think the biggest con is that I see no way of unit testing "pipeline failure should kill the process".
I tend to prefer the simpler solution, but at the end it's your call :)
Attempt n˚ 2
I simplified both the API (discarded the idea that the returned value is both a stream and a promise ; it’s a pure duplex now) and the implementation.
As a side-node, I’m running it in production for my backup system for a few months now (fortunately for testing, unfortunately for me including two backup recoveries).
It appears that this PR might be stalled.
Also, it seems like using a TransformStream might now be more proper than a Duplex?
In terms of implementation, I am wondering whether instead of duplicating the code from the main execa() function, a smaller function could first call execa() then create a TransformStream wrapping the return value's stdin, stdout (or all if the all option is true). This would be easier to maintain. This is the approach that both execa.node() and execa.$ are using.
Based on this, I am wondering whether this PR should be closed? cc @sindresorhus
Also, it seems like using a TransformStream might now be more proper than a Duplex?
How so?
For similar reasons as Buffer vs Uint8Array: web streams can be used in both browsers and Node.js.
However, Duplex is fine too, considering it is easy to convert using Stream.Duplex.toWeb(duplex).
Regardless, this PR is quite behind the current codebase. Also, it relies on re-implementing the main logic of execa() instead of wrapping it like execa.node and execa.$, which might come with some maintenance cost. What do you think?
However, Duplex is fine too, considering it is easy to convert using Stream.Duplex.toWeb(duplex).
I do I agree it would be nicer to use modern APIs whenever we add new features. So yes, TransformStream (web streams) would be best.
instead of wrapping it like execa.node and execa.$, which might come with some maintenance cost. What do you think?
Agreed. It would be better to wrap it like you said.