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 Tue, 22 Apr 2014 17:18:18 GMT
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