[
https://issues.apache.org/jira/browse/CB-8627?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14575421#comment-14575421
]
ASF GitHub Bot commented on CB-8627:
------------------------------------
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).
> 'cordova plugin add git_url' erroneously updates fetch.json even when the
> 'add operation fails'
> -----------------------------------------------------------------------------------------------
>
> Key: CB-8627
> URL: https://issues.apache.org/jira/browse/CB-8627
> Project: Apache Cordova
> Issue Type: Bug
> Components: CLI
> Reporter: Omar Mefire
> Assignee: Omar Mefire
>
> - cordova plugin add https://github.com/Wizcorp/phonegap-facebook-plugin.git
> This results in fetch.json being updated with the plugin info even though
> the plugin installation failed. It should not be the case.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
