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 Thu, 01 May 2014 12:28:09 GMT
I would appreciate it if someone can have a final look and get this in.. I
have doc updates to follow.
Thanks
--
Gorkem


On Fri, Apr 25, 2014 at 4:37 PM, Gorkem Ercan <gorkem.ercan@gmail.com>wrote:

> 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