cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michal Mocny <mmo...@chromium.org>
Subject Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Date Thu, 24 Apr 2014 03:39:19 GMT
Gorkem, as an orthogonal issue to the syntax we use, I think there is
another small problem with this patch as-is.

"cordova plugin add org.apache.cordova.file-transfer && cordova plugin
save" would have my application explicitly depend on
org.apache.cordova.file.  If in the future the dependency is
removed/moved/renamed, my app would explicitly try to install it when
running "cordova plugin restore".

As a first version I think this is acceptable, but I think we may want a
better solution eventually.


On Tue, Apr 22, 2014 at 1:18 PM, Michal Mocny <mmocny@chromium.org> wrote:

>
>
>
> On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <gorkem.ercan@gmail.com>wrote:
>
>> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <mmocny@chromium.org>
>> wrote:
>>
>> > On Fri, Apr 18, 2014 at 4:11 PM, Gorkem Ercan <gorkem.ercan@gmail.com
>> > >wrote:
>> >
>> > > On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny <mmocny@chromium.org>
>> > wrote:
>> > >
>> > > > Took a quick glance.  General questions:
>> > > > - why the need for save?  Why not just alter the list on each
>> cordova
>> > > > plugin add/rm?
>> > > >
>> > >
>> > > I do not want to force this workflow. Calling save does not bring much
>> > > overhead, considering plugin list does not probably change constantly.
>> > >
>> >
>> > If you are going to make this choice, then I would not like to
>> > automatically install on prepare.  There should be an explicit "load"
>> > command.  (those names are wrong, but you get the point).
>> >
>> > Either we automatically manage plugin installs, or explicitly manage
>> them.
>> >  I'm happy to start with explicit and support opt-in to automatic
>> handling
>> > later once we have confidence in the feature.
>> >
>>
>> Makes sense... adding a 'restore' command and removing from prepare. We
>> can
>> add an auto-restore config in the next iteration.
>>
>
> Excellent, thanks.
>
>
>>
>>
>> >
>> >
>> > >
>> > >
>> > > > - wouldn't "cordova plugin rm foo && cordova prepare" re-install
>> that
>> > > > plugin right now?
>> > > >
>> > >
>> > > This workflow would require an explicit update to config.xml if a
>> plugin
>> > is
>> > > persisted in there. This is a good point, I shall update plugins rm to
>> > > print a warning about it.
>> >
>> >
>> > >
>> > > > - why the name <feature> and not <dependency> ?  I think
this
>> > > functionality
>> > > > should overlap with the plugin.xml spec.
>> > > >
>> > > >
>> > > feature tag lives in the w3c widget spec which has advantages for
>> those
>> > > (like myself)  who does provide tools for editing config,xml.
>> > >
>> >
>> > We are no longer using that spec, and I as we move more functionality
>> from
>> > plugins.xml into config.xml we should strive to keep them in line.  It
>> also
>> > makes our docs easier.
>> >
>> >
>> Tools developers are people too :) I think plugin.xml and config.xml has
>> two different audiences, I think there is a comparatively small
>> intersection of developers who are interested on both. At the moment, we
>> are pretty much within the w3c spec with the top-level config.xml and I do
>> not see the benefit of breaking it.
>>
>
> Actually, I am thinking about tools devs.  Namely, our tools devs who
> should be using the same config parsing and handlers for dependency
> management, etc.
>
> Anyway.  You are putting in the sweat and blood on this feature, so you
> get double the voting power on this as far as I'm concerned.  Still, I
> think we should bring this to the attention of the list to see how everyone
> feels.  Sound fair?  I'll start a quick thread.
>
>
>>
>>
>> >
>> > >
>> > >
>> > >
>> > > > I haven't put the PR through testing yet.
>> > > >
>> > > >
>> > > > On Thu, Apr 17, 2014 at 5:54 PM, Jesse <purplecabbage@gmail.com>
>> > wrote:
>> > > >
>> > > > > Yeah, that looks weird.
>> > > > >
>> > > > > @purplecabbage
>> > > > > risingj.com
>> > > > >
>> > > > >
>> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <git@git.apache.org>
>> wrote:
>> > > > >
>> > > > > > Github user kamrik commented on a diff in the pull request:
>> > > > > >
>> > > > > >
>> > > > https://github.com/apache/cordova-cli/pull/165#discussion_r11755812
>> > > > > >
>> > > > > >     --- Diff: src/save.js ---
>> > > > > >     @@ -0,0 +1,71 @@
>> > > > > >     +/**
>> > > > > >     +    Licensed to the Apache Software Foundation (ASF)
under
>> one
>> > > > > >     +    or more contributor license agreements.  See the
NOTICE
>> > file
>> > > > > >     +    distributed with this work for additional information
>> > > > > >     +    regarding copyright ownership.  The ASF licenses
this
>> file
>> > > > > >     +    to you under the Apache License, Version 2.0 (the
>> > > > > >     +    "License"); you may not use this file except in
>> compliance
>> > > > > >     +    with the License.  You may obtain a copy of the
>> License at
>> > > > > >     +
>> > > > > >     +    http://www.apache.org/licenses/LICENSE-2.0
>> > > > > >     +
>> > > > > >     +    Unless required by applicable law or agreed to
in
>> writing,
>> > > > > >     +    software distributed under the License is distributed
>> on
>> > an
>> > > > > >     +    "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS
OF ANY
>> > > > > >     +    KIND, either express or implied.  See the License
for
>> the
>> > > > > >     +    specific language governing permissions and limitations
>> > > > > >     +    under the License.
>> > > > > >     +*/
>> > > > > >     +
>> > > > > >     +var cordova_util     = require('./util'),
>> > > > > >     +    ConfigParser     = require('./ConfigParser'),
>> > > > > >     +    path             = require('path'),
>> > > > > >     +    xml              =require('./xml-helpers')
>> > > > > >     +    Q                = require('q'),
>> > > > > >     +    events           = require('./events');
>> > > > > >     +
>> > > > > >     +;
>> > > > > >     +
>> > > > > >     +
>> > > > > >     +module.exports = function save(target){
>> > > > > >     +  var projectHome = cordova_util.cdProjectRoot();
>> > > > > >     +  var configPath = cordova_util.projectConfig(projectHome);
>> > > > > >     +  var configXml = new ConfigParser(configPath);
>> > > > > >     +  var pluginsPath = path.join(projectHome, 'plugins');
>> > > > > >     +  var plugins = cordova_util.findPlugins(pluginsPath);
>> > > > > >     +
>> > > > > >     +  return Q.all(plugins.map(function(plugin){
>> > > > > >     +    var currentPluginPath = path.join(pluginsPath,plugin);
>> > > > > >     +    var name = readPluginName(currentPluginPath);
>> > > > > >     +    var id = plugin;
>> > > > > >     +    var version = readPluginVersion(currentPluginPath);
>> > > > > >     +    var features = configXml.doc.findall('feature');
>> > > > > >     +      for(var i=0; i<features.length; i++){
>> > > > > >     +        if(features[i].attrib.name === name){
>> > > > > >     +          events.emit('results', 'An entry for "'+
plugin+
>> '"
>> > > > > already
>> > > > > > exists');
>> > > > > >     +          return Q();
>> > > > > >     +        }
>> > > > > >     +      }
>> > > > > >     +    configXml.addFeature(name, JSON.parse('[{"name":"id",
>> > > > > > "value":"'+id+'"},{"name":"version", "value":"'+version+'"}]'));
>> > > > > >     --- End diff --
>> > > > > >
>> > > > > >     I might be missing something, but why JSON.parse() rather
>> than
>> > > just
>> > > > > > literal array of objects?
>> > > > > >
>> > > > > >
>> > > > > > ---
>> > > > > > 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.
>> > > > > > ---
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message