Skip to content

Use cross-spawn & shelljs instead of child-process #478

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.

Sign up for GitHub

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

Merged
brody2consult merged 2 commits into from Dec 18, 2018
+23 −24
Merged

Use cross-spawn & shelljs instead of child-process #478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f0190c
Use superspawn consistently
oliversalzburg Dec 18, 2018
22b7841
Use shelljs consistently
oliversalzburg Dec 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions bin/templates/scripts/cordova/lib/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
var Q = require('q');
var path = require('path');
var shell = require('shelljs');
var spawn = require('./spawn');
var superspawn = require('cordova-common').superspawn;
var fs = require('fs');
var plist = require('plist');
var util = require('util');
Expand Down Expand Up @@ -168,11 +168,10 @@ module.exports.run = function (buildOpts) {
var buildOutputDir = path.join(projectPath, 'build', (buildOpts.device ? 'device' : 'emulator'));

// remove the build/device folder before building
return spawn('rm', [ '-rf', buildOutputDir ], projectPath)
.then(function () {
var xcodebuildArgs = getXcodeBuildArgs(projectName, projectPath, configuration, buildOpts.device, buildOpts.buildFlag, emulatorTarget, buildOpts.automaticProvisioning);
return spawn('xcodebuild', xcodebuildArgs, projectPath);
});
shell.rm('-rf', buildOutputDir);

var xcodebuildArgs = getXcodeBuildArgs(projectName, projectPath, configuration, buildOpts.device, buildOpts.buildFlag, emulatorTarget, buildOpts.automaticProvisioning);
return superspawn.spawn('xcodebuild', xcodebuildArgs, { cwd: projectPath });

}).then(function () {
if (!buildOpts.device || buildOpts.noSign) {
Expand Down Expand Up @@ -225,7 +224,7 @@ module.exports.run = function (buildOpts) {

function packageArchive () {
var xcodearchiveArgs = getXcodeArchiveArgs(projectName, projectPath, buildOutputDir, exportOptionsPath, buildOpts.automaticProvisioning);
return spawn('xcodebuild', xcodearchiveArgs, projectPath);
return superspawn.spawn('xcodebuild', xcodearchiveArgs, { cwd: projectPath });
}

return Q.nfcall(fs.writeFile, exportOptionsPath, exportOptionsPlist, 'utf-8')
Expand Down
6 changes: 3 additions & 3 deletions bin/templates/scripts/cordova/lib/clean.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
var Q = require('q');
var path = require('path');
var shell = require('shelljs');
var spawn = require('./spawn');
var superspawn = require('cordova-common').superspawn;

var projectPath = path.join(__dirname, '..', '..');

Expand All @@ -33,9 +33,9 @@ module.exports.run = function () {
return Q.reject('No Xcode project found in ' + projectPath);
}

return spawn('xcodebuild', ['-project', projectName, '-configuration', 'Debug', '-alltargets', 'clean'], projectPath)
return superspawn.spawn('xcodebuild', ['-project', projectName, '-configuration', 'Debug', '-alltargets', 'clean'], { cwd: projectPath })
.then(function () {
return spawn('xcodebuild', ['-project', projectName, '-configuration', 'Release', '-alltargets', 'clean'], projectPath);
return superspawn.spawn('xcodebuild', ['-project', projectName, '-configuration', 'Release', '-alltargets', 'clean'], { cwd: projectPath });
}).then(function () {
return shell.rm('-rf', path.join(projectPath, 'build'));
});
Expand Down
27 changes: 13 additions & 14 deletions bin/templates/scripts/cordova/lib/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ var Q = require('q');
var path = require('path');
var cp = require('child_process');
var build = require('./build');
var spawn = require('./spawn');
var shell = require('shelljs');
var superspawn = require('cordova-common').superspawn;
var check_reqs = require('./check_reqs');

var events = require('cordova-common').events;
Expand Down Expand Up @@ -78,7 +79,7 @@ module.exports.run = function (runOptions) {
var ipafile = path.join(buildOutputDir, projectName + '.ipa');

// unpack the existing platform/ios/build/device/appname.ipa (zipfile), will create a Payload folder
return spawn('unzip', [ '-o', '-qq', ipafile ], buildOutputDir);
return superspawn.spawn('unzip', [ '-o', '-qq', ipafile ], { cwd: buildOutputDir });
})
.then(function () {
// Uncompress IPA (zip file)
Expand All @@ -87,15 +88,13 @@ module.exports.run = function (runOptions) {
var payloadFolder = path.join(buildOutputDir, 'Payload');

// delete the existing platform/ios/build/device/appname.app
return spawn('rm', [ '-rf', appFile ], buildOutputDir)
.then(function () {
// move the platform/ios/build/device/Payload/appname.app to parent
return spawn('mv', [ '-f', appFileInflated, buildOutputDir ], buildOutputDir);
})
.then(function () {
// delete the platform/ios/build/device/Payload folder
return spawn('rm', [ '-rf', payloadFolder ], buildOutputDir);
});
shell.rm('-rf', appFile);
// move the platform/ios/build/device/Payload/appname.app to parent
shell.mv('-f', appFileInflated, buildOutputDir);
// delete the platform/ios/build/device/Payload folder
shell.rm('-rf', payloadFolder);

return null;
})
.then(function () {
appPath = path.join(projectPath, 'build', 'device', projectName + '.app');
Expand Down Expand Up @@ -149,7 +148,7 @@ function filterSupportedArgs (args) {
* @return {Promise} Fullfilled when any device is connected, rejected otherwise
*/
function checkDeviceConnected () {
return spawn('ios-deploy', ['-c', '-t', '1']);
return superspawn.spawn('ios-deploy', ['-c', '-t', '1']);
}

/**
Expand All @@ -161,9 +160,9 @@ function checkDeviceConnected () {
function deployToDevice (appPath, target, extraArgs) {
// Deploying to device...
if (target) {
return spawn('ios-deploy', ['--justlaunch', '-d', '-b', appPath, '-i', target].concat(extraArgs));
return superspawn.spawn('ios-deploy', ['--justlaunch', '-d', '-b', appPath, '-i', target].concat(extraArgs));
} else {
return spawn('ios-deploy', ['--justlaunch', '--no-wifi', '-d', '-b', appPath].concat(extraArgs));
return superspawn.spawn('ios-deploy', ['--justlaunch', '--no-wifi', '-d', '-b', appPath].concat(extraArgs));
}
}

Expand Down
1 change: 1 addition & 0 deletions bin/templates/scripts/cordova/lib/spawn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Copy Markdown
Member

@dpogue dpogue Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙁

Copy link
Copy Markdown

@brody2consult brody2consult Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also would favor updating spawn.js to output a warning message that it is deprecated and will be removed in the future.

Copy link
Copy Markdown
Contributor Author

@oliversalzburg oliversalzburg Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@brody2consult brody2consult Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
module.exports = function (cmd, args, opt_cwd) {
var d = Q.defer();
Expand Down