apache
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.
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
superspawninvocations withexecainvocations, 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-spawnas proposed in https://github.com/apache/cordova-common/pull/50 to solve the problem - then we migrate to
execaand get rid of ourcordova-common/superspawnas 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":
- https://github.com/apache/cordova-common/pull/50#issuecomment-422380691
- https://github.com/apache/cordova-common/pull/50#issuecomment-422386418
- https://github.com/apache/cordova-common/pull/50#issuecomment-422388901
@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.