cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From TimBarham <...@git.apache.org>
Subject [GitHub] cordova-lib pull request: CB-8627: Only update fetch.json when plu...
Date Fri, 05 Jun 2015 23:52:17 GMT
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


Mime
View raw message