cordova-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CB-8627) 'cordova plugin add git_url' erroneously updates fetch.json even when the 'add operation fails'
Date Fri, 05 Jun 2015 23:53:00 GMT

    [ 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: issues-unsubscribe@cordova.apache.org
For additional commands, e-mail: issues-help@cordova.apache.org


Mime
View raw message