search

stackvana / microcule

SDK and CLI for spawning streaming stateless HTTP microservices in multiple programming languages
Other
480 stars 29 forks source link

Use cross-spawn when spawning #11

Closed mikaelkaron closed 7 years ago

mikaelkaron commented 8 years ago

This uses cross-spawn instead of vanilla child_process for spawning new processes.

mikaelkaron commented 8 years ago

This solves cross platform spawning (in my case win 10.64) problems experienced in Stackvana/microservice-examples#1. There's still another problem in that ticket but it's more about the state of process.stdin in a bash-on-ubuntu shell than a pure spawn problem.

Marak commented 8 years ago

@ljharb -

We were having reports of microcule failing on Windows. I tried adding path.normalize, but still issues.

Do you have any quick suggestions for testing on windows? Cloud based? I'm away from my PC for a couple of days and don't prefer to setup a VM. Can't test anything on Windows at the moment.

Marak commented 8 years ago

@mikaelkaron -

Does this resolve your windows issues?

I'm not against adding a new dependency, but I'd like to understand exactly what the issue is ( and if we can possibly resolve it with a few lines of code instead of a new dependency )

mikaelkaron commented 8 years ago

It does. I think this also adds support for hashbang's on windows (not an out of the box solution) so we can avoid adding code for that (basically it's more than just fixing spawn, it's also helping you to stay portable)

ljharb commented 8 years ago

@Marak i've never set it up, but supposedly appveyor is like travis for windows

mikaelkaron commented 7 years ago

Would it be possible to merge this? If not what are the concerns?

Marak commented 7 years ago

@mikaelkaron Has been merged into master and is now pending release.

Sorry for the delay. microcule is used in a production and we need to be careful in making any cross-cutting changes. Going to push this out and see how it performs.

Thank you for your contribution.

mikaelkaron commented 7 years ago

I understand. If this affects performance I can try to take another stab at it but this was the simplest patch I could think of. Thank you for the merge.