Use cross-spawn & shelljs instead of child-process #478
Conversation
oliversalzburg
changed the title
| @@ -27,6 +27,7 @@ var proc = require('child_process'); | |||
| * @param {String} opt_cwd Working directory for command | |||
| * @param {String} opt_verbosity Verbosity level for command stdout output, "verbose" by default | |||
| * @return {Promise} Promise either fullfilled or rejected with error code | |||
| * @deprecated Use `require('cordova-common').superspawn` instead. | |||
There was a problem hiding this comment.
I'd be inclined to just remove this outright in the next major, but I guess technically we don't know if anything else used it so we should go through one major cycle with it marked deprecated 🙁
There was a problem hiding this comment.
I also would favor updating spawn.js to output a warning message that it is deprecated and will be removed in the future.
There was a problem hiding this comment.
I'm somewhat resistant to just console.log in this context. If we're assuming that someone is just going to require() this file, we can't assume other logging constructs to be in place. And logging directly to console may be more disturbing than helpful because it might break code that parses console output (which wouldn't be unreasonable with this code).
So I'd rather leave it as is, unless someone has a very specific suggestion on how to approach the issue.
There was a problem hiding this comment.
Got it. I would like to raise to finish raising another PR to add the deprecation message, for separate discussion, before we resolving this conversation.
Codecov Report
@@ Coverage Diff @@
## master #478 +/- ##
=========================================
+ Coverage 75.4% 75.71% +0.3%
=========================================
Files 12 11 -1
Lines 1805 1795 -10
=========================================
- Hits 1361 1359 -2
+ Misses 444 436 -8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks @oliversalzburg, issues seem to be resolved now. Are you ready for us to merge this one?
brodycj
changed the title
oliversalzburg
changed the title
|
@brodybits Should be good now. I also adjusted the title per your suggestion. Thanks! |
|
Oh, you just did that also. Sorry. Feel free to change back. I won't touch it again :D |
|
Wonders of interactive editing:) I made the terser title to get a "squash merge" commit message within 50 characters. I will probably use the terser title in the commit message and leave it up to you what to do with the title. I just edited the description to add a more descriptive list of the actual changes, which I would like to use in the squash merge commit message. Please feel free to edit as needed. I will probably merge it in the next 20-30 minutes or so. Thanks again! |
brodycj
changed the title
|
I just updated the title and my comments in the description yet again. I was under the mistaken impression that this PR did actually fix the xcodebuild command log, which is in #479. I will raise another followup PR to add the warning that the Another comment is that I think we should do the same thing on cordova-osx, just raised apache/cordova-osx#81 to track this. I will probably merge this in 10-20 minutes. Thanks again for the contributions! |
Platforms affected
iOS
What does this PR do?
Applies
superspawnandshelljsconsistently. Both modules are used in other places in this module. The existingspawn.jsin this project should be obsolete now. I preserved it just in case.Details by @brodybits:
Change details:
cross-spawn("superspawn") instead ofchild-processspawnshelljsinstead instead ofchild-processspawn in other placesspawnfunction provided byspawn.jsis deprecatedMotivation: this change will allow us to spawn with
printCommand: trueoption, to print thexcodebuildand other shell commands as proposed in #479.What testing has been done on this change?
Nothing at all.