You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cordova.apache.org by ka...@apache.org on 2015/03/27 16:16:34 UTC
[1/2] cordova-lib git commit: CB-8754 Auto-restoring a plugin fails
when adding a platform.
Repository: cordova-lib
Updated Branches:
refs/heads/master e6c940d2a -> 979f499b1
CB-8754 Auto-restoring a plugin fails when adding a platform.
Start with a blank Cordova app, then enter the following:
cordova plugin add org.apache.cordova.camera --save
cordova plugin remove org.apache.cordova.camera
cordova platform add browser
The final step should restore the camera plugin, but it fails with the following exception:
TypeError: Cannot read property 'latest' of undefined
at next (D:\GIT\Cordova\cordova-lib\cordova-lib\node_modules\npm\lib\cache.js:694:35)
at D:\GIT\Cordova\cordova-lib\cordova-lib\node_modules\npm\lib\cache.js:682:5
at RegClient.get_ (D:\GIT\Cordova\cordova-lib\cordova-lib\node_modules\npm\node_modules\npm-registry-client\lib\get.js:105:14)
at RegClient.<anonymous> (D:\GIT\Cordova\cordova-lib\cordova-lib\node_modules\npm\node_modules\npm-registry-client\lib\get.js:41:12)
at fs.js:336:14
at D:\GIT\Cordova\cordova-lib\cordova-lib\node_modules\npm\node_modules\graceful-fs\graceful-fs.js:103:5
at FSReqWrap.oncomplete (fs.js:99:15)
The core problem here is an issue with how we work with npm. Every time we are going to use npm (when dealing with plugins), we call npm.load() passing it our settings. But npm.load() can only be called once per session - subsequent calls are ignored. The correct approach is to call npm.load() without any settings (to make sure npm is loaded), then call npm.config.set() for each setting.
This change had been made for platforms, but not plugins. I also wanted to make sure each time we worked with npm we had a clean config (for example, if we get an npm package from the Cordova plugin respository, then later from the npm repository, it would try to get the second package from the CPR because that setting would still be around and not get overridden). So now any code that wants to load and init npm I pass through a central "load and restore" method. You pass this method your npm settings and the chain of promises you want to execute with those settings applied. It loads and initializes npm, executes the promises, then restores npm's configuration to what it was before we started.
Finally, once that problem was fixed there was an additional problem - we were adding plugins too early and the platform wasn't fully initialized, so we'd get an error about not being able to find the platform's config.xml file.
Project: http://git-wip-us.apache.org/repos/asf/cordova-lib/repo
Commit: http://git-wip-us.apache.org/repos/asf/cordova-lib/commit/4ad1b3d8
Tree: http://git-wip-us.apache.org/repos/asf/cordova-lib/tree/4ad1b3d8
Diff: http://git-wip-us.apache.org/repos/asf/cordova-lib/diff/4ad1b3d8
Branch: refs/heads/master
Commit: 4ad1b3d8226df68db8ee26451554996b77399314
Parents: e6c940d
Author: Tim Barham <ti...@microsoft.com>
Authored: Thu Mar 26 21:54:37 2015 +1000
Committer: Mark Koudritsky <ka...@gmail.com>
Committed: Fri Mar 27 11:09:45 2015 -0400
----------------------------------------------------------------------
.../spec-plugman/registry/registry.spec.js | 14 ++-
cordova-lib/src/cordova/lazy_load.js | 23 ++--
cordova-lib/src/cordova/prepare.js | 5 +-
cordova-lib/src/plugman/registry/registry.js | 110 +++++++++----------
cordova-lib/src/util/npm-helper.js | 76 +++++++++++++
5 files changed, 149 insertions(+), 79 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cordova-lib/blob/4ad1b3d8/cordova-lib/spec-plugman/registry/registry.spec.js
----------------------------------------------------------------------
diff --git a/cordova-lib/spec-plugman/registry/registry.spec.js b/cordova-lib/spec-plugman/registry/registry.spec.js
index ea4cd51..abc2048 100644
--- a/cordova-lib/spec-plugman/registry/registry.spec.js
+++ b/cordova-lib/spec-plugman/registry/registry.spec.js
@@ -100,7 +100,7 @@ describe('registry', function() {
};
registry.settings = fakeSettings;
- fakeLoad = spyOn(npm, 'load').andCallFake(function(settings, cb) { cb(null, true); });
+ fakeLoad = spyOn(npm, 'load').andCallFake(function () { arguments[arguments.length - 1](null, true); });
fakeNPMCommands = {};
['config', 'adduser', 'cache', 'publish', 'unpublish', 'search'].forEach(function(cmd) {
@@ -109,17 +109,19 @@ describe('registry', function() {
npm.commands = fakeNPMCommands;
npm.config.set = function(){};
+ npm.config.get = function(){};
+ npm.config.del = function(){};
});
it('should run config', function() {
var params = ['set', 'registry', 'http://registry.cordova.io'];
registryPromise(true, registry.config(params).then(function() {
- expect(fakeLoad).toHaveBeenCalledWith(registry.settings, jasmine.any(Function));
+ expect(fakeLoad).toHaveBeenCalledWith(jasmine.any(Function));
expect(fakeNPMCommands.config).toHaveBeenCalledWith(params, jasmine.any(Function));
}));
});
it('should run adduser', function() {
registryPromise(true, registry.adduser(null).then(function() {
- expect(fakeLoad).toHaveBeenCalledWith(registry.settings, jasmine.any(Function));
+ expect(fakeLoad).toHaveBeenCalledWith(jasmine.any(Function));
expect(fakeNPMCommands.adduser).toHaveBeenCalledWith(null, jasmine.any(Function));
}));
});
@@ -128,7 +130,7 @@ describe('registry', function() {
var spyGenerate = spyOn(manifest, 'generatePackageJsonFromPluginXml').andReturn(Q());
var spyUnlink = spyOn(fs, 'unlink');
registryPromise(true, registry.publish(params).then(function() {
- expect(fakeLoad).toHaveBeenCalledWith(registry.settings, jasmine.any(Function));
+ expect(fakeLoad).toHaveBeenCalledWith(jasmine.any(Function));
expect(spyGenerate).toHaveBeenCalledWith(params[0]);
expect(fakeNPMCommands.publish).toHaveBeenCalledWith(params, jasmine.any(Function));
expect(spyUnlink).toHaveBeenCalledWith(path.resolve(params[0], 'package.json'));
@@ -137,7 +139,7 @@ describe('registry', function() {
it('should run unpublish', function() {
var params = ['dummyplugin@0.6.0'];
registryPromise(true, registry.unpublish(params).then(function() {
- expect(fakeLoad).toHaveBeenCalledWith(registry.settings, jasmine.any(Function));
+ expect(fakeLoad).toHaveBeenCalledWith(jasmine.any(Function));
expect(fakeNPMCommands.unpublish).toHaveBeenCalledWith(params, jasmine.any(Function));
expect(fakeNPMCommands.cache).toHaveBeenCalledWith(['clean'], jasmine.any(Function));
}));
@@ -145,7 +147,7 @@ describe('registry', function() {
it('should run search', function() {
var params = ['dummyplugin', 'plugin'];
registryPromise(true, registry.search(params).then(function() {
- expect(fakeLoad).toHaveBeenCalledWith(registry.settings, jasmine.any(Function));
+ expect(fakeLoad).toHaveBeenCalledWith(jasmine.any(Function));
expect(fakeNPMCommands.search).toHaveBeenCalledWith(params, true, jasmine.any(Function));
}));
});
http://git-wip-us.apache.org/repos/asf/cordova-lib/blob/4ad1b3d8/cordova-lib/src/cordova/lazy_load.js
----------------------------------------------------------------------
diff --git a/cordova-lib/src/cordova/lazy_load.js b/cordova-lib/src/cordova/lazy_load.js
index 4b1b270..71ab5dd 100644
--- a/cordova-lib/src/cordova/lazy_load.js
+++ b/cordova-lib/src/cordova/lazy_load.js
@@ -35,6 +35,7 @@ var path = require('path'),
URL = require('url'),
Q = require('q'),
npm = require('npm'),
+ npmhelper = require('../util/npm-helper'),
unpack = require('../util/unpack'),
util = require('./util'),
gitclone = require('../gitclone'),
@@ -148,20 +149,14 @@ function npm_cache_add(pkg) {
registry: 'https://registry.npmjs.org'
};
- return Q.nfcall(npm.load)
- .then(function () {
- // configure npm here instead of passing parameters to npm.load due to CB-7670
- for (var prop in platformNpmConfig) {
- npm.config.set(prop, platformNpmConfig[prop]);
- }
- })
- .then(function() {
- return Q.ninvoke(npm.commands, 'cache', ['add', pkg]);
- }).then(function(info) {
- var pkgDir = path.resolve(npm.cache, info.name, info.version, 'package');
- // Unpack the package that was added to the cache (CB-8154)
- var package_tgz = path.resolve(npm.cache, info.name, info.version, 'package.tgz');
- return unpack.unpackTgz(package_tgz, pkgDir);
+ return npmhelper.loadWithSettingsThenRestore(platformNpmConfig, function () {
+ return Q.ninvoke(npm.commands, 'cache', ['add', pkg])
+ .then(function (info) {
+ var pkgDir = path.resolve(npm.cache, info.name, info.version, 'package');
+ // Unpack the package that was added to the cache (CB-8154)
+ var package_tgz = path.resolve(npm.cache, info.name, info.version, 'package.tgz');
+ return unpack.unpackTgz(package_tgz, pkgDir);
+ });
});
}
http://git-wip-us.apache.org/repos/asf/cordova-lib/blob/4ad1b3d8/cordova-lib/src/cordova/prepare.js
----------------------------------------------------------------------
diff --git a/cordova-lib/src/cordova/prepare.js b/cordova-lib/src/cordova/prepare.js
index cb3f5f7..c7e46ed 100644
--- a/cordova-lib/src/cordova/prepare.js
+++ b/cordova-lib/src/cordova/prepare.js
@@ -63,9 +63,6 @@ function prepare(options) {
});
options.paths = paths;
})
- .then(function(){
- return restore.installPluginsFromConfigXML(options);
- })
.then(function() {
var pluginInfoProvider = new PluginInfoProvider();
@@ -130,6 +127,8 @@ function prepare(options) {
})).then(function() {
return hooksRunner.fire('after_prepare', options);
});
+ }).then(function () {
+ return restore.installPluginsFromConfigXML(options);
});
}
http://git-wip-us.apache.org/repos/asf/cordova-lib/blob/4ad1b3d8/cordova-lib/src/plugman/registry/registry.js
----------------------------------------------------------------------
diff --git a/cordova-lib/src/plugman/registry/registry.js b/cordova-lib/src/plugman/registry/registry.js
index 9ed6cc3..1bd8c3e 100644
--- a/cordova-lib/src/plugman/registry/registry.js
+++ b/cordova-lib/src/plugman/registry/registry.js
@@ -29,6 +29,7 @@ var semver = require('semver'),
Q = require('q'),
request = require('request'),
pluginMapper = require('cordova-registry-mapper').oldToNew,
+ npmhelper = require('../../util/npm-helper'),
home = process.env.HOME || process.env.HOMEPATH || process.env.USERPROFILE,
events = require('../../events'),
unpack = require('../../util/unpack'),
@@ -52,11 +53,8 @@ module.exports = {
* @param {Array} args Command argument
* @return {Promise.<Object>} Promised configuration object.
*/
- config: function(args) {
- return initSettings().then(function(settings) {
- return Q.ninvoke(npm, 'load', settings);
- })
- .then(function() {
+ config: function (args) {
+ return initThenLoadSettingsWithRestore(function () {
return Q.ninvoke(npm.commands, 'config', args);
});
},
@@ -67,9 +65,7 @@ module.exports = {
* @return {Promise.<void>} Promise for completion.
*/
owner: function(args) {
- return initSettings().then(function(settings) {
- return Q.ninvoke(npm, 'load', settings);
- }).then(function() {
+ return initThenLoadSettingsWithRestore(function () {
return Q.ninvoke(npm.commands, 'owner', args);
});
},
@@ -79,10 +75,7 @@ module.exports = {
* @return {Promise.<void>} Promise for completion.
*/
adduser: function(args) {
- return initSettings().then(function(settings) {
- return Q.ninvoke(npm, 'load', settings);
- })
- .then(function() {
+ return initThenLoadSettingsWithRestore(function () {
return Q.ninvoke(npm.commands, 'adduser', args);
});
},
@@ -108,14 +101,14 @@ module.exports = {
}
}
return manifest.generatePackageJsonFromPluginXml(dir)
- .then(function() {
- return Q.ninvoke(npm, 'load', settings);
- }).then(function() {
- // With no --force we'll get a 409 (conflict) when trying to
- // overwrite an existing package@version.
- //npm.config.set('force', true);
- events.emit('log', 'attempting to publish plugin to registry');
- return Q.ninvoke(npm.commands, 'publish', args);
+ .then(function () {
+ return npmhelper.loadWithSettingsThenRestore(settings, function () {
+ // With no --force we'll get a 409 (conflict) when trying to
+ // overwrite an existing package@version.
+ //npm.config.set('force', true);
+ events.emit('log', 'attempting to publish plugin to registry');
+ return Q.ninvoke(npm.commands, 'publish', args);
+ });
}).then(function() {
fs.unlink(path.resolve(dir, 'package.json'));
//rename package.json1 to package.json if it exists
@@ -135,10 +128,7 @@ module.exports = {
* @return {Promise.<Object>} Promised search results.
*/
search: function(args) {
- return initSettings()
- .then(function(settings) {
- return Q.ninvoke(npm, 'load', settings);
- }).then(function() {
+ return initThenLoadSettingsWithRestore(function () {
return Q.ninvoke(npm.commands, 'search', args, true);
});
},
@@ -148,11 +138,8 @@ module.exports = {
* @param {Array} args Command argument
* @return {Promise.<Object>} Promised results.
*/
- unpublish: function(args) {
- return initSettings()
- .then(function(settings) {
- return Q.ninvoke(npm, 'load', settings);
- }).then(function() {
+ unpublish: function (args) {
+ return initThenLoadSettingsWithRestore(function () {
// --force is required to delete an entire plugin with all versions.
// Without --force npm can only unpublish a specific version.
//npm.config.set('force', true);
@@ -160,11 +147,12 @@ module.exports = {
// e.g.: `unpublish non.existent.plugin`
// will complete with no errors.
events.emit('log', 'attempting to unpublish plugin from registry');
- return Q.ninvoke(npm.commands, 'unpublish', args);
- }).then(function() {
- // npm.unpublish removes the cache for the unpublished package
- // cleaning the entire cache might not be necessary.
- return Q.ninvoke(npm.commands, 'cache', ['clean']);
+ return Q.ninvoke(npm.commands, 'unpublish', args)
+ .then(function () {
+ // npm.unpublish removes the cache for the unpublished package
+ // cleaning the entire cache might not be necessary.
+ return Q.ninvoke(npm.commands, 'cache', ['clean']);
+ });
});
},
@@ -223,11 +211,13 @@ module.exports = {
* @param {Boolean} determines if we are using the npm registry
* @return {Promise.<Object>} Promised settings.
*/
-function initSettings() {
+function initSettings(returnEmptySettings) {
var settings = module.exports.settings;
// check if settings already set
- if(settings !== null) return Q(settings);
+ if(settings !== null) {
+ return Q(settings);
+ }
// setting up settings
// obviously if settings dir does not exist settings is going to be empty
@@ -246,7 +236,23 @@ function initSettings() {
'cache-min': oneDay
});
- return Q(settings);
+ return Q(returnEmptySettings ? {} : settings);
+}
+
+/**
+ * @description Calls initSettings(), then npmhelper.loadWithSettingsThenRestore, which initializes npm.config with
+ * settings, executes the promises, then restores npm.config. Use this rather than passing settings to npm.load, since
+ * that only works the first time you try to load npm.
+ */
+function initThenLoadSettingsWithRestore(useEmptySettings, promises) {
+ if (typeof useEmptySettings === 'function') {
+ promises = useEmptySettings;
+ useEmptySettings = false;
+ }
+
+ return initSettings(useEmptySettings).then(function (settings) {
+ return npmhelper.loadWithSettingsThenRestore(settings, promises);
+ });
}
// Send a message to the registry to update download counts.
@@ -341,26 +347,18 @@ function fetchPlugin(plugin, client, useNpmRegistry) {
registryName = 'cordova plugins registry';
}
- return initSettings().then(function(settings) {
- // Don't use any option overrides for npm repo.
- if (useNpmRegistry) {
- settings = {};
- }
- return Q.ninvoke(npm, 'load', settings);
- })
- .then(function() {
+ return initThenLoadSettingsWithRestore(useNpmRegistry, function () {
events.emit('log', 'Fetching plugin "' + plugin + '" via ' + registryName);
- return Q.ninvoke(npm.commands, 'cache', ['add', plugin]);
- })
- .then(function(info) {
- var cl = (client === 'plugman' ? 'plugman' : 'cordova-cli');
- bumpCounter(info, cl);
- var pluginDir = path.resolve(npm.cache, info.name, info.version, 'package');
- // Unpack the plugin that was added to the cache (CB-8154)
- var package_tgz = path.resolve(npm.cache, info.name, info.version, 'package.tgz');
- return unpack.unpackTgz(package_tgz, pluginDir);
- })
- .fail(function(error) {
+ return Q.ninvoke(npm.commands, 'cache', ['add', plugin])
+ .then(function (info) {
+ var cl = (client === 'plugman' ? 'plugman' : 'cordova-cli');
+ bumpCounter(info, cl);
+ var pluginDir = path.resolve(npm.cache, info.name, info.version, 'package');
+ // Unpack the plugin that was added to the cache (CB-8154)
+ var package_tgz = path.resolve(npm.cache, info.name, info.version, 'package.tgz');
+ return unpack.unpackTgz(package_tgz, pluginDir);
+ });
+ }).fail(function(error) {
events.emit('log', 'Fetching from ' + registryName + ' failed: ' + error.message);
return Q.reject(error);
});
http://git-wip-us.apache.org/repos/asf/cordova-lib/blob/4ad1b3d8/cordova-lib/src/util/npm-helper.js
----------------------------------------------------------------------
diff --git a/cordova-lib/src/util/npm-helper.js b/cordova-lib/src/util/npm-helper.js
new file mode 100644
index 0000000..01b370b
--- /dev/null
+++ b/cordova-lib/src/util/npm-helper.js
@@ -0,0 +1,76 @@
+/**
+ 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.
+ */
+
+// Helper methods to help keep npm operations separated.
+
+var npm = require('npm'),
+ Q = require('q'),
+ cachedSettings = null,
+ cachedSettingsValues = null;
+
+/**
+ * @description Calls npm.load, then initializes npm.config with the specified settings. Then executes a chain of
+ * promises that rely on those npm settings, then restores npm settings back to their previous value. Use this rather
+ * than passing settings to npm.load, since that only works the first time you try to load npm.
+ * @param {Object} settings
+ * @param {Function} promiseChain
+ */
+function loadWithSettingsThenRestore(settings, promiseChain) {
+ return loadWithSettings(settings).then(promiseChain).then(restoreSettings);
+}
+
+function loadWithSettings(settings) {
+ if (cachedSettings) {
+ throw new Error('Trying to initialize npm when settings have not been restored from a previous initialization.');
+ }
+
+ return Q.nfcall(npm.load).then(function () {
+ for (var prop in settings) {
+ var currentValue = npm.config.get(prop);
+ var newValue = settings[prop];
+
+ if (currentValue !== newValue) {
+ cachedSettingsValues = cachedSettingsValues || {};
+ cachedSettings = cachedSettings || [];
+ cachedSettings.push(prop);
+ if (typeof currentValue !== 'undefined') {
+ cachedSettingsValues[prop] = currentValue;
+ }
+ npm.config.set(prop, newValue);
+ }
+ }
+ });
+}
+
+function restoreSettings(passthrough) {
+ if (cachedSettings) {
+ cachedSettings.forEach(function (prop) {
+ if (prop in cachedSettingsValues) {
+ npm.config.set(prop, cachedSettingsValues[prop]);
+ } else {
+ npm.config.del(prop);
+ }
+ });
+ cachedSettings = null;
+ cachedSettingsValues = null;
+ }
+ return passthrough;
+}
+
+module.exports.loadWithSettingsThenRestore = loadWithSettingsThenRestore;
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org
[2/2] cordova-lib git commit: CB-8754 Addressed comments:
Posted by ka...@apache.org.
CB-8754 Addressed comments:
* npm-helper.restoreSettings() should be called in a finally() rather than a
then().
* Ensure immediate return from registry.initSettings() honors
returnEmptySettings arg.
GitHub: close #194
Project: http://git-wip-us.apache.org/repos/asf/cordova-lib/repo
Commit: http://git-wip-us.apache.org/repos/asf/cordova-lib/commit/979f499b
Tree: http://git-wip-us.apache.org/repos/asf/cordova-lib/tree/979f499b
Diff: http://git-wip-us.apache.org/repos/asf/cordova-lib/diff/979f499b
Branch: refs/heads/master
Commit: 979f499b1bd8eb9cc51d3dd23cd128b14a9f9d08
Parents: 4ad1b3d
Author: Tim Barham <ti...@microsoft.com>
Authored: Sat Mar 28 00:42:56 2015 +1000
Committer: Mark Koudritsky <ka...@gmail.com>
Committed: Fri Mar 27 11:10:58 2015 -0400
----------------------------------------------------------------------
cordova-lib/src/plugman/registry/registry.js | 2 +-
cordova-lib/src/util/npm-helper.js | 5 ++---
2 files changed, 3 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cordova-lib/blob/979f499b/cordova-lib/src/plugman/registry/registry.js
----------------------------------------------------------------------
diff --git a/cordova-lib/src/plugman/registry/registry.js b/cordova-lib/src/plugman/registry/registry.js
index 1bd8c3e..71810a3 100644
--- a/cordova-lib/src/plugman/registry/registry.js
+++ b/cordova-lib/src/plugman/registry/registry.js
@@ -216,7 +216,7 @@ function initSettings(returnEmptySettings) {
// check if settings already set
if(settings !== null) {
- return Q(settings);
+ return Q(returnEmptySettings ? {} : settings);
}
// setting up settings
http://git-wip-us.apache.org/repos/asf/cordova-lib/blob/979f499b/cordova-lib/src/util/npm-helper.js
----------------------------------------------------------------------
diff --git a/cordova-lib/src/util/npm-helper.js b/cordova-lib/src/util/npm-helper.js
index 01b370b..c4a2c56 100644
--- a/cordova-lib/src/util/npm-helper.js
+++ b/cordova-lib/src/util/npm-helper.js
@@ -32,7 +32,7 @@ var npm = require('npm'),
* @param {Function} promiseChain
*/
function loadWithSettingsThenRestore(settings, promiseChain) {
- return loadWithSettings(settings).then(promiseChain).then(restoreSettings);
+ return loadWithSettings(settings).then(promiseChain).finally(restoreSettings);
}
function loadWithSettings(settings) {
@@ -58,7 +58,7 @@ function loadWithSettings(settings) {
});
}
-function restoreSettings(passthrough) {
+function restoreSettings() {
if (cachedSettings) {
cachedSettings.forEach(function (prop) {
if (prop in cachedSettingsValues) {
@@ -70,7 +70,6 @@ function restoreSettings(passthrough) {
cachedSettings = null;
cachedSettingsValues = null;
}
- return passthrough;
}
module.exports.loadWithSettingsThenRestore = loadWithSettingsThenRestore;
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cordova.apache.org
For additional commands, e-mail: commits-help@cordova.apache.org