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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org
For additional commands, e-mail: dev-h...@cordova.apache.org
