tapjs / foreground-child

Run a child as if it's the foreground process. Give it stdio. Exit when it exits.
http://tapjs.github.io/foreground-child/
ISC License
39 stars 14 forks source link

Replace cross-spawn-async with cross-spawn #9

Closed rmg closed 8 years ago

rmg commented 8 years ago

The logic for checking whether cross-spawn-async needed to be called is implemented directly in cross-spawn, so also remove our version of the logic and the corresponding direct dependency on 'which'.

Also includes a small fix to make the tests more reliable, especially when run with coverage.

Replaces #8

bcoe commented 8 years ago

@rmg this passed all my smoke tests on Windows, @isaacs any objects to landing and releasing?

isaacs commented 8 years ago

I'm fine with moving to use cross-spawn instead of cross-spawn-async, but I'm not clear on why this needs to be used on Linux. The commit message indicates that this is related to the travis failures, but I think that's just a race condition.

isaacs commented 8 years ago

@rmg Can you take a look at the cross-spawn-windows-only branch? Does that meet the same needs?

I'd like to avoid using any extra parsing code on unix where it's not really necessary.

rmg commented 8 years ago

The Travis fix is only in this PR so that we can see that the cross-spawn change doesn't break CI.

rmg commented 8 years ago

As for the extra parsing, that is still only done on Windows. The conditional just moves to inside the spawn wrapper. I only removed it once I looked at the cross-spawn code and saw it was nearly identical to the code in foreground-child. I suspect the code was either inspired by the code here or from a common source.

isaacs commented 8 years ago

As for the extra parsing, that is still only done on Windows.

Oh, I see, it calls into those parsing functions, but none of the functions do anything on not-windows. It just creates the "parsed" object out of the unmolested args.

But then... why even call into it on non-windows platforms then?

rmg commented 8 years ago

Put another way, why use a module that abstracts platform specifics if you're just going to check the platform yourself anyway?

I personally like this kind of shifting of responsibility where I can plug the leak and beat the abstraction back in to place. After all, cross-spawn practically has cross-platform in its name, so let it worry about that aspect.

isaacs commented 8 years ago

The reason to check the platform is that I want to guarantee that I get the "normal" child_process.spawn without any modifications on non-Windows platforms. Cross-spawn does that today, but I'm not sure if that's part of its contract per se. I'm a little hesitant because I've been burned by win-spawn and cross-spawn-async doing more than they should, and foreground-child is used cases where cp.spawn is already being wrapped and manipulated in creative ways.

rmg commented 8 years ago

Makes sense to me :+1:

rmg commented 8 years ago

@issacs thanks for the cleanup & merge :+1: