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 Sat, 19 Apr 2014 02:21:49 GMT
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.


>
>
> > - 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.


>
>
>
> > 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