Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove usage of cordova-common/superspawn #16

Open
janpio opened this issue Sep 18, 2018 · 12 comments
Open

Remove usage of cordova-common/superspawn #16

janpio opened this issue Sep 18, 2018 · 12 comments
Labels
enhancement New feature or request

Comments

@janpio
Copy link
Member

janpio commented Sep 18, 2018

After apache/cordova-common#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.

@raphinesse
Copy link
Contributor

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

@janpio
Copy link
Member Author

janpio commented Sep 18, 2018

Whatever works, I was getting this from apache/cordova-common#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.

@raphinesse
Copy link
Contributor

raphinesse commented Sep 18, 2018

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.

@janpio janpio added the enhancement New feature or request label Sep 18, 2018
@brodybits
Copy link

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

Why not use execa and skip cross-spawn?

@raphinesse
Copy link
Contributor

raphinesse commented Sep 18, 2018

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

@brodybits
Copy link

I am getting a bit confused here.

#16 (comment) and #16 (comment) indicate to me that we should skip cross-spawn and use execa right away, while apache/cordova-common#50 (comment) and apache/cordova-common#50 (comment) 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":

@raphinesse
Copy link
Contributor

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

@janpio
Copy link
Member Author

janpio commented Sep 18, 2018

apache/cordova-common#50 is one PR, doing one thing.
This, #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 apache/cordova-common#50 has been merged to remove the Windows specific handling.

@brodybits
Copy link

brodybits commented Sep 18, 2018 via email

@janpio
Copy link
Member Author

janpio commented Sep 18, 2018

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.

@brodybits
Copy link

Another rationale I gave in #7 (comment) 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).

@raphinesse
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants