cordova
cordova copied to clipboard
Remove usage of cordova-common/superspawn
After https://github.com/apache/cordova-common/pull/50 is merged, superspawn
is just a small wrapper around cross-spawn
, that makes sure the cmd
is actually executable by chmod-ing the file. As this should never be necessary for e.g. npm
and other commands we run, this is probably only appropriate in places like hooks or similar. superspawn
can thus be replaced with direct invocations of cross-spawn
, maybe removed alltogether.
I think we want to replace superspawn
invocations with execa
invocations, not cross-spawn
.
Whatever works, I was getting this from https://github.com/apache/cordova-common/pull/50 but exaca
looks great as well. On the other hand, that might show that we may not want to get rid of the wrapper after all - preferences seems to change here.
To provide more context: cross-spawn
only improves cross-platform compatibility of child_process.spawn
while replicating it's interface. execa
builds on top of that and adds various other features, like a nicer Promise-based interface (similar to superspawn
) cross-platform shebang support (would be useful for HooksRunner
) and a lot more. And lastly we do not have to maintain execa
. That's a big plus.
So yes, we want something a bit more high-level than cross-spawn
, but I'd prefer execa
over superspawn
.
I think we want to replace
superspawn
invocations withexeca
invocations, notcross-spawn
.
Why not use execa
and skip cross-spawn
?
@brodybits That is what I'm suggesting. Or I don't understand the question.
I am getting a bit confused here.
https://github.com/apache/cordova/issues/16#issuecomment-422374967 and https://github.com/apache/cordova/issues/16#issuecomment-422375635 indicate to me that we should skip cross-spawn
and use execa
right away, while https://github.com/apache/cordova-common/pull/50#issuecomment-422380691 and https://github.com/apache/cordova-common/pull/50#issuecomment-422386418 indicate to me that we take a two-step process (use cross-spawn
first then use execa
).
I would personally favor the two-step process:
- first use
cross-spawn
as proposed in https://github.com/apache/cordova-common/pull/50 to solve the problem - then we migrate to
execa
and get rid of ourcordova-common/superspawn
as discussed here
@raphinesse can you please straighten me out here?
I would also like to motion that we hide or remove the following comments as "off-topic":
@brodybits Now I understand. Yes, I would favor a two step process too since migration to execa
could take quite some time.
I hid the comments you referred to as "resolved"
https://github.com/apache/cordova-common/pull/50 is one PR, doing one thing.
This, https://github.com/apache/cordova/issues/16, is another issue, suggesting the doing of another thing. (At first it was "replace superspawn
with cross-spawn
", after comments from @raphinesse it is "replace superspawn
with execa
").
These are not related only in that this issue only makes sense after https://github.com/apache/cordova-common/pull/50 has been merged to remove the Windows specific handling.
Thanks Raphael for the perfect disposition. Thanks Janpio for the clarification. I would like to make another motion to put execa into the title and description of either this issue or a new issue.
This goal of this issue is to "Remove usage of cordova-common/superspawn". execa
is an implementation detail. When someone gets around to implement this, it might very well be that it makes sense to use something else or newer.
Another rationale I gave in https://github.com/apache/cordova/issues/7#issuecomment-501917531 is that superspawn
seems to return a non-standard Promise-like object that can also notify the listener of stdout
and stderr
data. I think getting rid of superspawn
would really help finish getting rid of q
(#7).
I'd like to finally resolve this issue and #7, so I'm going to try and finish the transformation we started here.