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

Change cordova-lib to execute plugin hooks without the need of Cordova project structure #236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
7 changes: 0 additions & 7 deletions cordova-lib/spec-cordova/HooksRunner.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,6 @@ describe('HooksRunner', function() {
process.chdir(path.join(__dirname, '..')); // Non e2e tests assume CWD is repo root.
});

it('should throw if provided directory is not a cordova project', function() {
expect(function() {
new HooksRunner(tmpDir);
}).toThrow();
});

it('should not throw if provided directory is a cordova project', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test case that deals with a NON-cordova project.

expect(function () {
new HooksRunner(project);
Expand Down Expand Up @@ -116,7 +110,6 @@ describe('HooksRunner', function() {
return Q();
});


// Add the testing platform.
cordova.raw.platform('add', [helpers.testPlatform]).fail(function (err) {
expect(err).toBeUndefined();
Expand Down
29 changes: 26 additions & 3 deletions cordova-lib/spec-plugman/install.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ var install = require('../src/plugman/install'),
events = require('../src/events'),
plugman = require('../src/plugman/plugman'),
platforms = require('../src/plugman/platforms/common'),
HooksRunner = require('../src/hooks/HooksRunner'),
common = require('./common'),
fs = require('fs'),
os = require('os'),
Expand All @@ -47,6 +48,7 @@ var install = require('../src/plugman/install'),
'org.test.plugins.childbrowser' : path.join(plugins_dir, 'org.test.plugins.childbrowser'),
'com.adobe.vars' : path.join(plugins_dir, 'com.adobe.vars'),
'org.test.defaultvariables' : path.join(plugins_dir, 'org.test.defaultvariables'),
'org.test.plugin-with-hooks' : path.join(plugins_dir, 'org.test.plugin-with-hooks'),
'A' : path.join(plugins_dir, 'dependencies', 'A'),
'B' : path.join(plugins_dir, 'dependencies', 'B'),
'C' : path.join(plugins_dir, 'dependencies', 'C'),
Expand Down Expand Up @@ -154,7 +156,7 @@ describe('start', function() {
});

describe('install', function() {
var chmod, exec, add_to_queue, prepare, cp, rm, fetchSpy;
var chmod, exec, add_to_queue, prepare, cp, rm, fetchSpy, fire;
var spawnSpy;

beforeEach(function() {
Expand All @@ -173,11 +175,32 @@ describe('install', function() {
spyOn(fs, 'writeFileSync').andReturn(true);
cp = spyOn(shell, 'cp').andReturn(true);
rm = spyOn(shell, 'rm').andReturn(true);
add_to_queue = spyOn(PlatformJson.prototype, 'addInstalledPluginToPrepareQueue');
done = false;
add_to_queue = spyOn(PlatformJson.prototype, 'addInstalledPluginToPrepareQueue');
fire = spyOn(HooksRunner.prototype, 'fire').andReturn(Q());
done = false;
});

describe('success', function() {
it('should fire hooks on plugin successful install', function() {
runs(function() {
installPromise( install('android', project, plugins['org.test.plugin-with-hooks']) );
});
waitsFor(function() { return done; }, 'install promise never resolved', 200);
runs(function() {
expect(fire).toHaveBeenCalled();
});
});

it('should not fire hooks on plugin successful install when run_hooks=false', function() {
runs(function() {
installPromise( install('android', project, plugins['org.test.plugin-with-hooks'], {run_hooks:false}) );
});
waitsFor(function() { return done; }, 'install promise never resolved', 200);
runs(function() {
expect(fire).not.toHaveBeenCalled();
});
});

it('should call prepare after a successful install', function() {
expect(results['prepareCount']).toBe(5);
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?xml version="1.0" encoding="UTF-8"?>
<plugin xmlns="http://apache.org/cordova/ns/plugins/1.0"
xmlns:android="http://schemas.android.com/apk/res/android"
id="org.test.plugin-with-hooks" version="0.0.1">

<name>Plugin With Hooks</name>
<license>Commercial</license>
<keywords>cordova,device</keywords>
<engines>
<engine name="cordova" version=">=3.0.0" />
</engines>

<hook type="before_plugin_install" src="scripts/beforeInstall.js" />
<hook type="after_plugin_install" src="scripts/afterInstall.js" />
<hook type="before_plugin_uninstall" src="scripts/beforeUninstall.js" />
</plugin>
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module.exports = function(context) {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module.exports = function(context) {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module.exports = function(context) {
}
29 changes: 24 additions & 5 deletions cordova-lib/spec-plugman/uninstall.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ var uninstall = require('../src/plugman/uninstall'),
actions = require('../src/plugman/util/action-stack'),
PlatformJson = require('../src/plugman/util/PlatformJson'),
events = require('../src/events'),
plugman = require('../src/plugman/plugman'),
plugman = require('../src/plugman/plugman'),
HooksRunner = require('../src/hooks/HooksRunner'),
common = require('./common'),
fs = require('fs'),
path = require('path'),
Expand All @@ -41,7 +42,8 @@ var uninstall = require('../src/plugman/uninstall'),
plugins_install_dir2 = path.join(project2, 'cordova', 'plugins'),

plugins = {
'org.test.plugins.dummyplugin' : path.join(plugins_dir, 'org.test.plugins.dummyplugin'),
'org.test.plugins.dummyplugin' : path.join(plugins_dir, 'org.test.plugins.dummyplugin'),
'org.test.plugin-with-hooks' : path.join(plugins_dir, 'org.test.plugin-with-hooks'),
'A' : path.join(plugins_dir, 'dependencies', 'A'),
'C' : path.join(plugins_dir, 'dependencies', 'C')
},
Expand All @@ -64,6 +66,8 @@ describe('start', function() {
promise = Q()
.then(function(){
return install('android', project, plugins['org.test.plugins.dummyplugin']);
}).then(function(){
return install('android', project, plugins['org.test.plugin-with-hooks']);
}).then(function(){
return install('android', project, plugins['A']);
}).then( function(){
Expand Down Expand Up @@ -97,7 +101,7 @@ describe('uninstallPlatform', function() {
add_to_queue = spyOn(PlatformJson.prototype, 'addUninstalledPluginToPrepareQueue');
done = false;
});
describe('success', function() {
describe('success', function() {
it('should call prepare after a successful uninstall', function() {
runs(function() {
uninstallPromise(uninstall.uninstallPlatform('android', project, dummy_id));
Expand Down Expand Up @@ -177,7 +181,7 @@ describe('uninstallPlugin', function() {
done = false;
});
describe('with dependencies', function() {

it('should delete all dependent plugins', function() {
runs(function() {
uninstallPromise( uninstall.uninstallPlugin('A', plugins_install_dir) );
Expand Down Expand Up @@ -234,15 +238,26 @@ describe('uninstallPlugin', function() {
});

describe('uninstall', function() {
var fsWrite, rm, add_to_queue;
var fsWrite, rm, add_to_queue, fire;

beforeEach(function() {
fsWrite = spyOn(fs, 'writeFileSync').andReturn(true);
rm = spyOn(shell, 'rm').andReturn(true);
add_to_queue = spyOn(PlatformJson.prototype, 'addUninstalledPluginToPrepareQueue');
fire = spyOn(HooksRunner.prototype, 'fire').andReturn(Q());
done = false;
});
describe('success', function() {
it('should fire hooks on plugin successful uninstalled', function() {
runs(function() {
uninstallPromise( uninstall('android', project, plugins['org.test.plugin-with-hooks']) );
});
waitsFor(function() { return done; }, 'promise never resolved', 200);
runs(function() {
expect(fire).toHaveBeenCalled();
});
});

it('should call the config-changes module\'s add_uninstalled_plugin_to_prepare_queue method after processing an install', function() {
runs(function() {
uninstallPromise( uninstall('android', project, plugins['org.test.plugins.dummyplugin']) );
Expand Down Expand Up @@ -285,6 +300,10 @@ describe('end', function() {
function(){
return uninstall('android', project, plugins['org.test.plugins.dummyplugin']);
}
).then(
function(){
return uninstall('android', project, plugins['org.test.plugin-with-hooks']);
}
).then(
function(){
// Fails... A depends on
Expand Down
41 changes: 33 additions & 8 deletions cordova-lib/src/hooks/HooksRunner.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,21 @@ var isWindows = os.platform().slice(0, 3) === 'win';
* Tries to create a HooksRunner for passed project root.
* @constructor
*/
function HooksRunner(projectRoot) {
var root = cordovaUtil.isCordova(projectRoot);
if (!root) throw new CordovaError('Not a Cordova project ("' + projectRoot + '"), can\'t use hooks.');
else this.projectRoot = root;
function DefaultHooksRunner(projectRoot) {
this.projectRoot = projectRoot;
}

DefaultHooksRunner.prototype.prepareOptions = function(opts) {
opts = opts || {};
opts.projectRoot = this.projectRoot;
return opts;
};

/**
* Fires all event handlers and scripts for a passed hook type.
* Returns a promise.
*/
HooksRunner.prototype.fire = function fire(hook, opts) {
DefaultHooksRunner.prototype.fire = function fire(hook, opts) {
// args check
if (!hook) {
throw new Error('hook type is not specified');
Expand All @@ -59,12 +63,24 @@ HooksRunner.prototype.fire = function fire(hook, opts) {
});
};

/**
* Tries to create a CordovaHooksRunner for passed project root.
* @constructor
*/
function CordovaHooksRunner(projectRoot) {
DefaultHooksRunner.call(this, projectRoot);
var root = cordovaUtil.isCordova(projectRoot);
if (!root) throw new CordovaError('Not a Cordova project ("' + projectRoot + '"), can\'t use hooks.');
Copy link
Contributor

Choose a reason for hiding this comment

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

:nit please use if (...) { } else { } (with curly braces).

else this.projectRoot = root;
}

require('util').inherits(CordovaHooksRunner, DefaultHooksRunner);

/**
* Refines passed options so that all required parameters are set.
*/
HooksRunner.prototype.prepareOptions = function(opts) {
opts = opts || {};
opts.projectRoot = this.projectRoot;
CordovaHooksRunner.prototype.prepareOptions = function(opts) {
opts = DefaultHooksRunner.prototype.prepareOptions.call(this, opts);
opts.cordova = opts.cordova || {};
opts.cordova.platforms = opts.cordova.platforms || opts.platforms || cordovaUtil.listPlatforms(opts.projectRoot);
opts.cordova.platforms = opts.cordova.platforms.map(function(platform) { return platform.split('@')[0]; } );
Expand All @@ -80,6 +96,15 @@ HooksRunner.prototype.prepareOptions = function(opts) {
return opts;
};

function HooksRunner(projectRoot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please provide the 'why' we are moving to have both CordovaHooksRunner and DefaultHooksRunner as a comment ? So that other maintainers are not confused as to the reason behind this shift.

var root = cordovaUtil.isCordova(projectRoot);
this.hooksRunner = root ? new CordovaHooksRunner(root) : new DefaultHooksRunner(projectRoot);
}

HooksRunner.prototype.fire = function fire(hook, opts) {
return this.hooksRunner.fire(hook, opts);
};

module.exports = HooksRunner;

/**
Expand Down
5 changes: 5 additions & 0 deletions cordova-lib/src/hooks/scriptsFinder.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ function getApplicationHookScriptsFromDir(dir) {
*/
function getScriptsFromConfigXml(hook, opts) {
var configPath = cordovaUtil.projectConfig(opts.projectRoot);

if (!(fs.existsSync(configPath))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a comment here that lets code readers know that there are cases when the cordova project structure is not available would be helpful.

return [];
}

var configXml = new ConfigParser(configPath);

return configXml.getHookScripts(hook, opts.cordova.platforms).map(function(scriptElement) {
Expand Down
9 changes: 4 additions & 5 deletions cordova-lib/src/plugman/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -336,16 +336,15 @@ function runInstall(actions, platform, project_dir, plugin_dir, plugins_dir, opt
}
).then(
function(){
var install_plugin_dir = path.join(plugins_dir, pluginInfo.id);
var install_plugin_dir = path.join(plugins_dir, pluginInfo.id),
run_hooks = options.hasOwnProperty('run_hooks') ? options.run_hooks : true;

// may need to copy to destination...
if ( !fs.existsSync(install_plugin_dir) ) {
copyPlugin(plugin_dir, plugins_dir, options.link, pluginInfoProvider);
}

var projectRoot = cordovaUtil.isCordova();

if(projectRoot) {
if(run_hooks) {
// using unified hooksRunner
var hookOptions = {
cordova: { platforms: [ platform ] },
Expand All @@ -357,7 +356,7 @@ function runInstall(actions, platform, project_dir, plugin_dir, plugins_dir, opt
}
};

var hooksRunner = new HooksRunner(projectRoot);
var hooksRunner = new HooksRunner(cordovaUtil.isCordova() || project_dir);

Copy link
Contributor

Choose a reason for hiding this comment

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

why cordovaUtil.isCordova() || project_dir ?
wouldn't just using project_dir achieve the same result ?

return hooksRunner.fire('before_plugin_install', hookOptions).then(function() {
return handleInstall(actions, pluginInfo, platform, project_dir, plugins_dir, install_plugin_dir, filtered_variables, options);
Expand Down
5 changes: 3 additions & 2 deletions cordova-lib/src/plugman/plugman.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ plugman.commands = {
cli_variables: cli_variables,
www_dir: cli_opts.www,
searchpath: cli_opts.searchpath,
link: cli_opts.link
link: cli_opts.link,
run_hooks: !cli_opts.nohooks
};

var p = Q();
Expand All @@ -130,7 +131,7 @@ plugman.commands = {
var p = Q();
cli_opts.plugin.forEach(function (pluginSrc) {
p = p.then(function () {
return plugman.raw.uninstall(cli_opts.platform, cli_opts.project, pluginSrc, cli_opts.plugins_dir, { www_dir: cli_opts.www });
return plugman.raw.uninstall(cli_opts.platform, cli_opts.project, pluginSrc, cli_opts.plugins_dir, { www_dir: cli_opts.www, run_hooks: !cli_opts.nohooks });
});
});

Expand Down
10 changes: 5 additions & 5 deletions cordova-lib/src/plugman/uninstall.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,9 @@ module.exports.uninstallPlugin = function(id, plugins_dir, options) {
// possible options: cli_variables, www_dir, is_top_level
// Returns a promise
function runUninstallPlatform(actions, platform, project_dir, plugin_dir, plugins_dir, options) {
var pluginInfoProvider = options.pluginInfoProvider;
var pluginInfoProvider = options.pluginInfoProvider,
run_hooks = options.hasOwnProperty('run_hooks') ? options.run_hooks : true;

// If this plugin is not really installed, return (CB-7004).
if (!fs.existsSync(plugin_dir)) {
return Q();
Expand Down Expand Up @@ -268,9 +270,7 @@ function runUninstallPlatform(actions, platform, project_dir, plugin_dir, plugin
promise = Q();
}

var projectRoot = cordovaUtil.isCordova();

if(projectRoot) {
if(run_hooks) {

// using unified hooksRunner
var hooksRunnerOptions = {
Expand All @@ -283,7 +283,7 @@ function runUninstallPlatform(actions, platform, project_dir, plugin_dir, plugin
}
};

var hooksRunner = new HooksRunner(projectRoot);
var hooksRunner = new HooksRunner(cordovaUtil.isCordova() || project_dir);

Copy link
Contributor

Choose a reason for hiding this comment

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

why cordovaUtil.isCordova() || project_dir ?
wouldn't just using project_dir achieve the same result ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sending project_dir should be enough

return promise.then(function() {
return hooksRunner.fire('before_plugin_uninstall', hooksRunnerOptions);
Expand Down