cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gorkem Ercan <gorkem.er...@gmail.com>
Subject Re: [GitHub] cordova-cli pull request: CB-6469 - Restore plugins from config.xm...
Date Fri, 25 Apr 2014 20:37:44 GMT
FYI. Just updated the PR with separated save & restore commands.
--
Gorkem


On Thu, Apr 24, 2014 at 7:21 PM, Gorkem Ercan <gorkem.ercan@gmail.com>wrote:

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

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