Mailing-List: contact dev-help@cordova.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cordova.apache.org
Received-SPF: pass (nike.apache.org: domain of gorkem.ercan@gmail.com
 designates 209.85.160.175 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAEeF2TexkAG933WBNBKhFG9cO1ROFuJ9df95wx8H9aqxZ6dmOg@mail.gmail.com>
References: <git-pr-165-cordova-cli@git.apache.org>
	<20140417214643.E0C2B9923CF@tyr.zones.apache.org>
	<CAKP6k2vfWUSoQngM5LrhzS1udT36VcaZnofL8+w6245DhmOWWA@mail.gmail.com>
	<CAEeF2Tc=mc5cPGJ_bTYzhuUW7RvWLEQOE2hTH_p7_7bDwW9aNQ@mail.gmail.com>
	<CA+X37cmLxTninngSUM7CLtfHYHpTqhteUaC4v7QL+03=8ib65w@mail.gmail.com>
	<CAEeF2Te-B6iP1Z36GtvVHWjRw+9N2qyYHZ9MEtg6opCpLgJmVQ@mail.gmail.com>
	<CA+X37cnbdpLjqecO73xQV3MzDV5e4ozZZ9kYm4Ob_5CQ4-z8OA@mail.gmail.com>
	<CAEeF2TdzsQrAgUUV0P=h5XSNuXNm3g=sOVfR+PCdEjmOrUd8DA@mail.gmail.com>
	<CAEeF2TdNS6iTpOwSj0SSA2Yb-jxVpVCfvBJc4JYb9AJN4SR36Q@mail.gmail.com>
	<CAEeF2TexkAG933WBNBKhFG9cO1ROFuJ9df95wx8H9aqxZ6dmOg@mail.gmail.com>
Date: Thu, 24 Apr 2014 19:21:51 -0400
Message-ID: 
 <CA+X37cm57FS5L_jvcVHYpoZwXBuB4CMY1rj=iRga3O5f-5Cgow@mail.gmail.com>
Subject: Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from
 config.xm...
From: Gorkem Ercan <gorkem.ercan@gmail.com>
To: "dev@cordova.apache.org" <dev@cordova.apache.org>
Content-Type: multipart/alternative; boundary=20cf303a36dd26c75b04f7d21ddc

--20cf303a36dd26c75b04f7d21ddc
Content-Type: text/plain; charset=UTF-8

I guess this is essentially moving save command as a flag to cordova plugin
* commands and restore becomes harder to discover. We need to consider the
platforms too since next stop is implementing "cordova save/restore
platforms"
--
Gorkem


On Thu, Apr 24, 2014 at 5:05 PM, Michal Mocny <mmocny@chromium.org> wrote:

> Gorkem,
>
> I also found an old JIRA duplicate issue for this which had listed an old
> suggestion I think is interesting:
> https://issues.apache.org/jira/browse/CB-4624
>
> Specifically, instead of introducing save/restore commands, we could mirror
> "npm install" and its use of "--save":
> - npm install -> cordova plugin add
>   - restores deps
> - npm install foo -> cordova plugin add foo
>   - install foo, but don't add it to deps
> - npm install foo --save -> cordova plugin add foo --save
>   - install foo, and do add it to deps
> - npm install --save -> cordova plugin add --save
>   - don't install anything, just save the current installed plugins into
> deps
>
> Just something to consider.
>
> (note, just tried it again and npm install --save doesn't actually do what
> I want.. wonder if its supported..)
>
> -Michal
>
>
> On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <mmocny@chromium.org>
> wrote:
>
> > 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.
> >>> > > > > > ---
> >>> > > > > >
> >>> > > > >
> >>> > > >
> >>> > >
> >>> >
> >>>
> >>
> >>
> >
>

--20cf303a36dd26c75b04f7d21ddc--