You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by galexandrov <gi...@git.apache.org> on 2015/06/05 09:44:45 UTC
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
GitHub user galexandrov opened a pull request:
https://github.com/apache/cordova-lib/pull/236
Change cordova-lib to execute plugin hooks without the need of Cordova project structure
Change cordova-lib to execute plugin hooks defined in plugin.xml without the need of Cordova project structure and extend Plugman with extra command argument (--nohooks) for preventing plugin hooks execution. By default the hooks will be executed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Icenium/cordova-lib galexandrov/plugman-hooks
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/236.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #236
----
commit bfea31ae60debb2869cddd1231278a3c7163e21a
Author: Georgi Alexandrov <ge...@telerik.com>
Date: 2015-06-04T14:30:35Z
Enable Plugman to run hooks defined in plugin.xml
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/236#discussion_r37145990
--- Diff: cordova-lib/src/hooks/scriptsFinder.js ---
@@ -105,6 +105,11 @@ function getApplicationHookScriptsFromDir(dir) {
*/
function getScriptsFromConfigXml(hook, opts) {
var configPath = cordovaUtil.projectConfig(opts.projectRoot);
+
+ if (!(fs.existsSync(configPath))) {
--- End diff --
a comment here that lets code readers know that there are cases when the cordova project structure is not available would be helpful.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/236#discussion_r37145607
--- Diff: cordova-lib/src/hooks/HooksRunner.js ---
@@ -80,6 +96,15 @@ HooksRunner.prototype.prepareOptions = function(opts) {
return opts;
};
+function HooksRunner(projectRoot) {
--- End diff --
Could you please provide the 'why' we are moving to have both CordovaHooksRunner and DefaultHooksRunner ? So that other maintainers are not confused as to the reason behind this shift.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/236#issuecomment-131553995
I feel like the plugman changes ( `run_hooks` ) should be part of a separate pull request.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/236#discussion_r37146073
--- Diff: cordova-lib/src/plugman/install.js ---
@@ -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);
--- End diff --
why `cordovaUtil.isCordova() || project_dir` ?
wouldn't just using `project_dir` achieve the same result ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/236#issuecomment-122213253
What's the reason behind this proposed change ? What use case does this fulfill ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by galexandrov <gi...@git.apache.org>.
Github user galexandrov commented on the pull request:
https://github.com/apache/cordova-lib/pull/236#issuecomment-125530595
In our case we don't use the Cordova CLI we use cordova-{platform} scripts for project creation and that's why we don't have the Cordova CLI Project structure. But it seems that the hooks execution is tightly bound with the Cordova Project structure and more specific with the existence of cordova folder.
The condition for hook execution in plugmna was cordovaUtil.isCordova(), which means that when we use cordova-{platform} scripts for project creation and cordova-plugman for plugin management the hooks will not be executed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/236#issuecomment-130831453
@galexandrov , will take a look when possible...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on the pull request:
https://github.com/apache/cordova-lib/pull/236#issuecomment-218633214
I'd like to revive this PR. Hooks should run when using create scripts + plugman.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by stevengill <gi...@git.apache.org>.
Github user stevengill commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/236#discussion_r62952050
--- Diff: cordova-lib/src/plugman/uninstall.js ---
@@ -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);
--- End diff --
Sending project_dir should be enough
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/236#discussion_r37146091
--- Diff: cordova-lib/spec-cordova/HooksRunner.spec.js ---
@@ -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() {
--- End diff --
Please add a test case that deals with a NON-cordova project.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/236#discussion_r37145507
--- Diff: cordova-lib/src/hooks/HooksRunner.js ---
@@ -60,11 +64,23 @@ 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.');
--- End diff --
:nit please use `if (...) { } else { }` (with curly braces).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by galexandrov <gi...@git.apache.org>.
Github user galexandrov commented on the pull request:
https://github.com/apache/cordova-lib/pull/236#issuecomment-130315580
ping @omefire
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-lib pull request: Change cordova-lib to execute plugin hoo...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/236#discussion_r37146081
--- Diff: cordova-lib/src/plugman/uninstall.js ---
@@ -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);
--- End diff --
why cordovaUtil.isCordova() || project_dir ?
wouldn't just using project_dir achieve the same result ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org