You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by vladimir-kotikov <gi...@git.apache.org> on 2015/09/09 17:35:56 UTC
[GitHub] cordova-lib pull request: CB-9617 Do not restore plugins immediate...
GitHub user vladimir-kotikov opened a pull request:
https://github.com/apache/cordova-lib/pull/304
CB-9617 Do not restore plugins immediately after plugin removal
https://github.com/apache/cordova-lib/commit/14675051c400a6811a6c6171fbf92f3475244630 introduced `cordova.prepare` call after each plugin uninstallation (see [CB-9617](https://issues.apache.org/jira/browse/CB-9617)). This results in a weird behaviour, when removed plugin immediately restored if it was saved in config.xml.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-lib CB-9617
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/304.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 #304
----
commit 28f50131ef14615157c56fae748557cda30f9e00
Author: Vladimir Kotikov <v-...@microsoft.com>
Date: 2015-09-09T15:25:10Z
CB-9617 Do not restore plugins after plugin removal
----
---
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: CB-9617 Do not restore plugins immediate...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-lib/pull/304#issuecomment-140217724
:+1:
---
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: CB-9617 Do not restore plugins immediate...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/304#issuecomment-138958045
@omefire, @TimBarham, please review 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: CB-9617 Do not restore plugins immediate...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/304#issuecomment-139129776
Will review tomorrow
---
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: CB-9617 Do not restore plugins immediate...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/304#discussion_r39400734
--- Diff: cordova-lib/src/cordova/plugin.js ---
@@ -283,6 +283,31 @@ module.exports = function plugin(command, targets, opts) {
}
};
+/**
+ * Calls `platformApi.prepare` for each platform in project
+ *
+ * @param {string[]} platformList List of platforms, added to current project
+ * @param {string} projectRoot Project root directory
+ *
+ * @return {Promise}
+ */
+function preparePlatforms (platformList, projectRoot) {
+ return Q.all(platformList.map(function(platform) {
+ // TODO: this need to be replaced by real projectInfo
+ // instance for current project.
+ var project = {
+ root: projectRoot,
+ projectConfig: new ConfigParser(cordova_util.projectConfig(projectRoot)),
+ locations: {
+ plugins: path.join(projectRoot, 'plugins'),
+ www: cordova_util.projectWww(projectRoot)
+ }
+ };
+
+ return platforms.getPlatformApi(platform).prepare(project);
+ }));
+}
+
--- End diff --
Yup, factored this to separate function. Another thing that i've missed, is that we still need to do a `browserify` if corresponding option is specified.
---
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: CB-9617 Do not restore plugins immediate...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-lib/pull/304#issuecomment-139157987
It seems like an ok fix for the immediate problem, but it does raise a concern in my mind - before the Platform API changes, we didn't call `prepare` when adding/removing plugins. This meant we didn't call `installPluginsFromConfigXML()` even when *adding* a plugin.
So, I have a question: is it perhaps overkill to call `prepare()` when adding/removing plugins? We end up doing a bunch of stuff we didn't used to do - such as calling `installPluginsFromConfigXML()`, calling `installPlatformsFromConfigXML()`, firing `before_prepare` and `after_prepare` hooks. Maybe other stuff? Is this desirable, or should we only be calling a subset of prepare() functionality when adding/removing plugins?
---
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: CB-9617 Do not restore plugins immediate...
Posted by vladimir-kotikov <gi...@git.apache.org>.
Github user vladimir-kotikov commented on the pull request:
https://github.com/apache/cordova-lib/pull/304#issuecomment-139542072
@TimBarham, agree with you, calling a `cordova.prepare` on each plugin add/rm might be excess. Reworked.
---
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: CB-9617 Do not restore plugins immediate...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cordova-lib/pull/304
---
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: CB-9617 Do not restore plugins immediate...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/304#discussion_r39388053
--- Diff: cordova-lib/src/cordova/plugin.js ---
@@ -283,6 +283,31 @@ module.exports = function plugin(command, targets, opts) {
}
};
+/**
+ * Calls `platformApi.prepare` for each platform in project
+ *
+ * @param {string[]} platformList List of platforms, added to current project
+ * @param {string} projectRoot Project root directory
+ *
+ * @return {Promise}
+ */
+function preparePlatforms (platformList, projectRoot) {
+ return Q.all(platformList.map(function(platform) {
+ // TODO: this need to be replaced by real projectInfo
+ // instance for current project.
+ var project = {
+ root: projectRoot,
+ projectConfig: new ConfigParser(cordova_util.projectConfig(projectRoot)),
+ locations: {
+ plugins: path.join(projectRoot, 'plugins'),
+ www: cordova_util.projectWww(projectRoot)
+ }
+ };
+
+ return platforms.getPlatformApi(platform).prepare(project);
+ }));
+}
+
--- End diff --
Logic looks good to me. Just one suggestion - the guts of `preparePlatforms()` is an exact duplicate of code within `prepare.js`, correct? Why not add a `preparePlatform()` method in `prepare.js`, and call that from within the loop here and within the loop in `prepare`?
I guess one argument against that would be that this logic will become pretty simplistic once there is a real `projectInfo` instance for the project, but on the other hand, sharing this code means you don't have to update that in two places. So... I guess up to you :smile:.
---
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