My plan is to add a warning to "cordova plugin rm" that will remind to explicitly call "cordova save plugins" to refresh the plugin dependencies. In a future iteration we can add a "keep my dependencies always restorable" flag so that cordova plugin rm/add also do the save. (and prepare does the auto restore) -- Gorkem
On Wed, Apr 23, 2014 at 11:39 PM, Michal Mocny <[email protected]> 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 <[email protected]> wrote: > > > > > > > > > On Tue, Apr 22, 2014 at 11:37 AM, Gorkem Ercan <[email protected] > >wrote: > > > >> On Fri, Apr 18, 2014 at 7:21 PM, Michal Mocny <[email protected]> > >> wrote: > >> > >> > On Fri, Apr 18, 2014 at 4:11 PM, Gorkem Ercan <[email protected] > >> > >wrote: > >> > > >> > > On Thu, Apr 17, 2014 at 3:46 PM, Michal Mocny <[email protected]> > >> > 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 <[email protected]> > >> > wrote: > >> > > > > >> > > > > Yeah, that looks weird. > >> > > > > > >> > > > > @purplecabbage > >> > > > > risingj.com > >> > > > > > >> > > > > > >> > > > > On Thu, Apr 17, 2014 at 2:46 PM, kamrik <[email protected]> > >> 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 [email protected] or file a > >> JIRA > >> > > > > ticket > >> > > > > > with INFRA. > >> > > > > > --- > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > > > >
