cordova icon indicating copy to clipboard operation
cordova copied to clipboard

Remove usage of cordova-common/superspawn

Open janpio opened this issue 6 years ago • 12 comments

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.

janpio avatar Sep 18 '18 11:09 janpio

I think we want to replace superspawn invocations with execa invocations, not cross-spawn.

raphinesse avatar Sep 18 '18 12:09 raphinesse

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.

janpio avatar Sep 18 '18 12:09 janpio

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.

raphinesse avatar Sep 18 '18 12:09 raphinesse

I think we want to replace superspawn invocations with execa invocations, not cross-spawn.

Why not use execa and skip cross-spawn?

brody4hire avatar Sep 18 '18 12:09 brody4hire

@brodybits That is what I'm suggesting. Or I don't understand the question.

raphinesse avatar Sep 18 '18 12:09 raphinesse

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:

@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":

brody4hire avatar Sep 18 '18 13:09 brody4hire

@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"

raphinesse avatar Sep 18 '18 13:09 raphinesse

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.

janpio avatar Sep 18 '18 13:09 janpio

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.

brody4hire avatar Sep 18 '18 14:09 brody4hire

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.

janpio avatar Sep 18 '18 14:09 janpio

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).

brody4hire avatar Jun 14 '19 00:06 brody4hire

I'd like to finally resolve this issue and #7, so I'm going to try and finish the transformation we started here.

raphinesse avatar Nov 01 '21 15:11 raphinesse