You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by omefire <gi...@git.apache.org> on 2015/05/27 20:28:25 UTC
[GitHub] cordova-lib pull request: CB-8627: Only update fetch.json when plu...
GitHub user omefire opened a pull request:
https://github.com/apache/cordova-lib/pull/228
CB-8627: Only update fetch.json when plugins are successfully installed.
CB-8627: Only update fetch.json when plugins are successfully installed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/MSOpenTech/cordova-lib CB-8627
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cordova-lib/pull/228.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 #228
----
commit cfbf601e578aab54a2d601065922f42ba56592d5
Author: Omar Mefire <om...@microsoft.com>
Date: 2015-05-27T18:25:50Z
CB-8627: Only update fetch.json when plugins are successfully installed.
----
---
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-8627: Only update fetch.json when plu...
Posted by robpaveza <gi...@git.apache.org>.
Github user robpaveza commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/228#discussion_r36766838
--- Diff: cordova-lib/src/cordova/plugin.js ---
@@ -206,10 +178,41 @@ module.exports = function plugin(command, targets, opts) {
}
}
- events.emit('verbose', 'Calling plugman.install on plugin "' + dir + '" for platform "' + platform + '" with options "' + JSON.stringify(options) + '"');
- return plugman.raw.install(platform, platformRoot, path.basename(dir), pluginsDir, options);
+ events.emit('verbose', 'Calling plugman.install on plugin "' + result.dest + '" for platform "' + platform + '" with options "' + JSON.stringify(options) + '"');
+ return plugman.raw.install(platform, platformRoot, path.basename(result.dest), pluginsDir, options);
});
- }, Q());
+ }, Q()).then(function(){
+ return Q(result);
+ });
+ })
+ .then(function(result){
+ metadata.save_fetch_metadata(pluginsDir, result.pinfo.id, { source: result.fetchJsonSource, variables: opts.cli_variables, is_top_level: true });
+ return Q(result.dest);
+ })
+ .then(function(dir){
+ // save to config.xml
+ if(saveToConfigXmlOn(config_json,opts)){
+ var pluginInfo = pluginInfoProvider.get(dir);
+
+ var attributes = {};
+ attributes.name = pluginInfo.id;
+
+ var src = parseSource(target, opts);
+ attributes.spec = src ? src : '^' + pluginInfo.version;
+
+ var variables = [];
+ if (opts.cli_variables) {
+ for (var varname in opts.cli_variables) {
--- End diff --
`Object.keys(opts.cli_variables)` rather than `for...in`.
---
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-8627: Only update fetch.json when plu...
Posted by robpaveza <gi...@git.apache.org>.
Github user robpaveza commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/228#discussion_r36766359
--- Diff: cordova-lib/src/cordova/plugin.js ---
@@ -206,10 +178,41 @@ module.exports = function plugin(command, targets, opts) {
}
}
- events.emit('verbose', 'Calling plugman.install on plugin "' + dir + '" for platform "' + platform + '" with options "' + JSON.stringify(options) + '"');
- return plugman.raw.install(platform, platformRoot, path.basename(dir), pluginsDir, options);
+ events.emit('verbose', 'Calling plugman.install on plugin "' + result.dest + '" for platform "' + platform + '" with options "' + JSON.stringify(options) + '"');
+ return plugman.raw.install(platform, platformRoot, path.basename(result.dest), pluginsDir, options);
});
- }, Q());
+ }, Q()).then(function(){
+ return Q(result);
+ });
+ })
+ .then(function(result){
+ metadata.save_fetch_metadata(pluginsDir, result.pinfo.id, { source: result.fetchJsonSource, variables: opts.cli_variables, is_top_level: true });
--- End diff --
Why not camelCased instead of separated_by_underscore?
---
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-8627: Only update fetch.json when plu...
Posted by robpaveza <gi...@git.apache.org>.
Github user robpaveza commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/228#discussion_r36766275
--- Diff: cordova-lib/src/cordova/plugin.js ---
@@ -206,10 +178,41 @@ module.exports = function plugin(command, targets, opts) {
}
}
- events.emit('verbose', 'Calling plugman.install on plugin "' + dir + '" for platform "' + platform + '" with options "' + JSON.stringify(options) + '"');
- return plugman.raw.install(platform, platformRoot, path.basename(dir), pluginsDir, options);
+ events.emit('verbose', 'Calling plugman.install on plugin "' + result.dest + '" for platform "' + platform + '" with options "' + JSON.stringify(options) + '"');
+ return plugman.raw.install(platform, platformRoot, path.basename(result.dest), pluginsDir, options);
});
- }, Q());
+ }, Q()).then(function(){
+ return Q(result);
+ });
+ })
+ .then(function(result){
+ metadata.save_fetch_metadata(pluginsDir, result.pinfo.id, { source: result.fetchJsonSource, variables: opts.cli_variables, is_top_level: true });
+ return Q(result.dest);
+ })
+ .then(function(dir){
+ // save to config.xml
+ if(saveToConfigXmlOn(config_json,opts)){
--- End diff --
Usage: Function name `saveToConfigXmlOn` implies that it performs the saving, but your branch seems to do so. Perhaps rename to `shouldSaveToConfigXml`.
---
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-8627: Only update fetch.json when plu...
Posted by TimBarham <gi...@git.apache.org>.
Github user TimBarham commented on the pull request:
https://github.com/apache/cordova-lib/pull/228#issuecomment-109485385
As I general design point, I don't like the idea of breaking the `save_fetch_metadata()` call out so it needs to be called separately, for two reasons:
1. First and most importantly, `plugman.fetch()` is a public API, and this changes its behavior (it no longer writes to `fetch.json`, and it returns a different value).
2. You've tightened the coupling between `cordova.plugin.add` and `plugman` - `cordova.plugin.add` now needs to know stuff about that result object, and needs to use it to call `save_fetch_metadata()`.
I think a better approach would be for `fetch()` to support an (*optional*) method that it calls before it writes `fetch.json` - this method would return a promise (or reject if something fails). Let's say this parameter was called `validate`, the end of `fetch()` might look something like this (though you'd need to handle the fact that this method would need to be optional):
``` js
}).then(function(result) {
options.plugin_src_dir = result.pinfo.dir;
return Q.when(copyPlugin(result.pinfo, plugins_dir, options.link && result.fetchJsonSource.type == 'local'))
.then(function(dir) {
result.dest = dir;
return result;
});
});
}).then(function(result) {
checkID(options.expected_id, result.pinfo);
return result;
}).then(validate).then(function (result) {
var data = { source: result.fetchJsonSource };
data.is_top_level = options.is_top_level;
data.variables = options.variables || {};
metadata.save_fetch_metadata(plugins_dir, result.pinfo.id, data);
return result.dest;
});
}
```
Some additional points:
* In the actual implementation, you should find a cleaner way to pass `result` through to the final piece of code, rather than requiring the `validate()` method return it like my example would. Maybe `validate()` doesn't need to be asynchronous (that is, it could just return a Boolean rather than a promise), but it is certainly more flexible if it supports it being async.
* Since `plugman.fetch()` is responsible for creating the plugin's folder and its contents, it should also be responsible for removing it if the process fails (rather than `plugin.add()` doing it as happens now).
---
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-8627: Only update fetch.json when plu...
Posted by robpaveza <gi...@git.apache.org>.
Github user robpaveza commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/228#discussion_r36766569
--- Diff: cordova-lib/src/plugman/fetch.js ---
@@ -146,11 +145,7 @@ function fetchPlugin(plugin_src, plugins_dir, options) {
});
}).then(function(result){
checkID(options.expected_id, result.pinfo);
- var data = { source: result.fetchJsonSource };
- data.is_top_level = options.is_top_level;
- data.variables = options.variables || {};
- metadata.save_fetch_metadata(plugins_dir, result.pinfo.id, data);
- return result.dest;
+ return result;
--- End diff --
Function name `checkID` seems to do something and return a result, but you don't capture that result, nor do you return anything with that result. Because you pass the value of `result.pinfo` by value, `checkID` has no opportunity to mutate or modify anything.
What happens if `checkID` "fails", if it can? If it can't, why is it called `check`?
---
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-8627: Only update fetch.json when plu...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/228#issuecomment-106026407
This change makes sure that the fetch.json file only gets updated with plugin information once the plugin has been successfully installed. fetch.json is considered the source of truth with regards to what plugins are installed.
One particular case this change addresses is that a case of missing variables during `cordova plugin add` (e.g: for plugins like facebook-plugin) will not end up with fetch.json modified even though the plugin addition failed.
---
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-8627: Only update fetch.json when plu...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-lib/pull/228#issuecomment-126621793
Ping!
---
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-8627: Only update fetch.json when plu...
Posted by dblotsky <gi...@git.apache.org>.
Github user dblotsky commented on the pull request:
https://github.com/apache/cordova-lib/pull/228#issuecomment-121700137
What's the status on this PR?
---
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-8627: Only update fetch.json when plu...
Posted by robpaveza <gi...@git.apache.org>.
Github user robpaveza commented on a diff in the pull request:
https://github.com/apache/cordova-lib/pull/228#discussion_r36766958
--- Diff: cordova-lib/src/cordova/plugin.js ---
@@ -206,10 +178,41 @@ module.exports = function plugin(command, targets, opts) {
}
}
- events.emit('verbose', 'Calling plugman.install on plugin "' + dir + '" for platform "' + platform + '" with options "' + JSON.stringify(options) + '"');
- return plugman.raw.install(platform, platformRoot, path.basename(dir), pluginsDir, options);
+ events.emit('verbose', 'Calling plugman.install on plugin "' + result.dest + '" for platform "' + platform + '" with options "' + JSON.stringify(options) + '"');
+ return plugman.raw.install(platform, platformRoot, path.basename(result.dest), pluginsDir, options);
});
- }, Q());
+ }, Q()).then(function(){
+ return Q(result);
+ });
+ })
+ .then(function(result){
+ metadata.save_fetch_metadata(pluginsDir, result.pinfo.id, { source: result.fetchJsonSource, variables: opts.cli_variables, is_top_level: true });
+ return Q(result.dest);
+ })
+ .then(function(dir){
+ // save to config.xml
+ if(saveToConfigXmlOn(config_json,opts)){
+ var pluginInfo = pluginInfoProvider.get(dir);
+
+ var attributes = {};
+ attributes.name = pluginInfo.id;
--- End diff --
I'm confused by the dichotomy of `name` and `id`.
---
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-8627: Only update fetch.json when plu...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/228#issuecomment-206015614
Closing this as not relevant anymore.
---
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-8627: Only update fetch.json when plu...
Posted by omefire <gi...@git.apache.org>.
Github user omefire closed the pull request at:
https://github.com/apache/cordova-lib/pull/228
---
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-8627: Only update fetch.json when plu...
Posted by omefire <gi...@git.apache.org>.
Github user omefire commented on the pull request:
https://github.com/apache/cordova-lib/pull/228#issuecomment-106671208
ping ... @TimBarham
---
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