feat: proper support for scoped plugins (#821)

* Merge PluginSpec.scope into PluginSpec.id

* Store scoped plugins in same dir structure as in node_modules

* Update and extend E2E tests for adding scoped plugins

* Fix plugman.uninstall for scoped plugins

* Remove top_plugins override

* Pass pluginsDir to get_fetch_metadata to support scoped plugins

* Fix cordova/util.findPlugins for scoped plugins

* Update to cordova-common@4.0.0-nightly

integration-tests/plugin.spec.js

@@ -46,6 +46,8 @@ var org_test_defaultvariables = 'org.test.defaultvariables';
 var npmInfoTestPlugin = 'cordova-lib-test-plugin';
 var npmInfoTestPluginVersion = '1.1.2';
 
+var scopedTestPlugin = '@cordova/plugin-test-dummy';
+
 var testGitPluginRepository = 'https://github.com/apache/cordova-plugin-device.git';
 var testGitPluginId = 'cordova-plugin-device';
 
@@ -224,33 +226,47 @@ describe('plugin end-to-end', function () {
     }, 30000);
 
     it('Test 011 : should handle scoped npm packages', function () {
-        var scopedPackage = '@testscope/' + npmInfoTestPlugin;
-        mockPluginFetch(npmInfoTestPlugin, path.join(pluginsDir, npmInfoTestPlugin));
+        mockPluginFetch(scopedTestPlugin, path.join(pluginsDir, scopedTestPlugin));
 
         spyOn(plugin_util, 'info').and.returnValue(Promise.resolve({}));
-        return addPlugin(scopedPackage, npmInfoTestPlugin, {})
+        return addPlugin(scopedTestPlugin, scopedTestPlugin, {})
             .then(function () {
                 // Check to make sure that we are at least trying to get the correct package.
                 // This package is not published to npm, so we can't truly do end-to-end tests
 
-                expect(plugin_util.info).toHaveBeenCalledWith([scopedPackage]);
+                expect(plugin_util.info).toHaveBeenCalledWith([scopedTestPlugin]);
 
                 var fetchTarget = plugman.fetch.calls.mostRecent().args[0];
-                expect(fetchTarget).toEqual(scopedPackage);
+                expect(fetchTarget).toEqual(scopedTestPlugin);
             });
     }, 30000);
 
     it('Test 012 : should handle scoped npm packages with given version tags', function () {
-        var scopedPackage = '@testscope/' + npmInfoTestPlugin + '@latest';
-        mockPluginFetch(npmInfoTestPlugin, path.join(pluginsDir, npmInfoTestPlugin));
+        var scopedPackage = scopedTestPlugin + '@latest';
+        mockPluginFetch(scopedTestPlugin, path.join(pluginsDir, scopedTestPlugin));
 
         spyOn(plugin_util, 'info');
-        return addPlugin(scopedPackage, npmInfoTestPlugin, {})
+        return addPlugin(scopedPackage, scopedTestPlugin, {})
             .then(function () {
                 expect(plugin_util.info).not.toHaveBeenCalled();
 
                 var fetchTarget = plugman.fetch.calls.mostRecent().args[0];
                 expect(fetchTarget).toEqual(scopedPackage);
             });
     }, 30000);
+
+    it('Test 013 : should be able to add and remove scoped npm packages without screwing up everything', () => {
+        mockPluginFetch(scopedTestPlugin, path.join(pluginsDir, scopedTestPlugin));
+        spyOn(plugin_util, 'info').and.returnValue(Promise.resolve({}));
+
+        return addPlugin(scopedTestPlugin, scopedTestPlugin, {})
+            .then(() => {
+                expect(plugin_util.info).toHaveBeenCalledWith([scopedTestPlugin]);
+
+                var fetchTarget = plugman.fetch.calls.mostRecent().args[0];
+                expect(fetchTarget).toEqual(scopedTestPlugin);
+
+                return removePlugin(scopedTestPlugin);
+            });
+    }, 30000);
 });

package.json

@@ -17,7 +17,7 @@
     "node": ">=10.0.0"
   },
   "dependencies": {
-    "cordova-common": "^3.1.0",
+    "cordova-common": "4.0.0-nightly.2019.11.8.40cb6cee",
     "cordova-create": "^2.0.0",
     "cordova-fetch": "^2.0.0",
     "cordova-serve": "^3.0.0",

spec/cordova/fixtures/plugins/@cordova/plugin-test-dummy/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "@cordova/plugin-test-dummy",
+  "version": "0.0.1",
+  "author": "Apache Software Foundation",
+  "license": "Apache-2.0"
+}

spec/cordova/fixtures/plugins/@cordova/plugin-test-dummy/plugin.xml

@@ -0,0 +1,6 @@
+<?xml version='1.0' encoding='utf-8'?>
+<plugin
+    xmlns="http://apache.org/cordova/ns/plugins/1.0"
+    id="@cordova/plugin-test-dummy"
+    version="0.0.1"
+/>

spec/cordova/platform/addHelper.spec.js

@@ -347,6 +347,17 @@ describe('cordova/platform/addHelper', function () {
             });
         });
 
+        it('should invoke plugman.install with correct plugin ID for a scoped plugin', () => {
+            const scopedPluginId = '@cordova/cordova-plugin-scoped';
+            cordova_util.findPlugins.and.returnValue([scopedPluginId]);
+
+            return installPluginsForNewPlatformWithTestArgs().then(() => {
+                expect(plugman.install).toHaveBeenCalledTimes(1);
+                const pluginId = plugman.install.calls.argsFor(0)[2];
+                expect(pluginId).toBe(scopedPluginId);
+            });
+        });
+
         it('should include any plugin variables as options when invoking plugman install', function () {
             const variables = {};
             fetch_metadata.get_fetch_metadata.and.returnValue({ variables });

spec/cordova/plugin/add.spec.js

@@ -129,6 +129,15 @@ describe('cordova/plugin/add', function () {
                     expect(plugman.install).toHaveBeenCalledWith('android', jasmine.any(String), jasmine.any(String), jasmine.any(String), jasmine.any(Object));
                 });
             });
+            it('should invoke plugman.install with the full ID of a scoped plugin', () => {
+                const scopedPluginId = '@cordova/plugin-thinger';
+                plugin_info.id = scopedPluginId;
+
+                return add(projectRoot, hook_mock, { plugins: [scopedPluginId] }).then(() => {
+                    const actualPluginId = plugman.install.calls.argsFor(0)[2];
+                    expect(actualPluginId).toBe(scopedPluginId);
+                });
+            });
             it('should save plugin variable information to package.json file (if exists)', function () {
                 var cli_plugin_variables = { some: 'variable' };
 

spec/cordova/plugin/plugin_spec_parser.spec.js

@@ -20,39 +20,37 @@
 var pluginSpec = require('../../../src/cordova/plugin/plugin_spec_parser');
 
 describe('methods for parsing npm plugin packages', function () {
-    function checkPluginSpecParsing (testString, scope, id, version) {
+    function checkPluginSpecParsing (testString, id, version) {
         var parsedSpec = pluginSpec.parse(testString);
-        expect(parsedSpec.scope).toEqual(scope);
         expect(parsedSpec.id).toEqual(id || testString);
         expect(parsedSpec.version).toEqual(version);
-        expect(parsedSpec.package).toEqual(scope ? scope + id : id);
     }
 
     it('Test 001 : should handle package names with no scope or version', function () {
-        checkPluginSpecParsing('test-plugin', null, 'test-plugin', null);
+        checkPluginSpecParsing('test-plugin', 'test-plugin', null);
     });
     it('Test 002 : should handle package names with a version', function () {
-        checkPluginSpecParsing('test-plugin@1.0.0', null, 'test-plugin', '1.0.0');
-        checkPluginSpecParsing('test-plugin@latest', null, 'test-plugin', 'latest');
+        checkPluginSpecParsing('test-plugin@1.0.0', 'test-plugin', '1.0.0');
+        checkPluginSpecParsing('test-plugin@latest', 'test-plugin', 'latest');
     });
     it('Test 003 : should handle package names with a scope', function () {
-        checkPluginSpecParsing('@test/test-plugin', '@test/', 'test-plugin', null);
+        checkPluginSpecParsing('@test/test-plugin', '@test/test-plugin', null);
     });
     it('Test 004 : should handle package names with a scope and a version', function () {
-        checkPluginSpecParsing('@test/test-plugin@1.0.0', '@test/', 'test-plugin', '1.0.0');
-        checkPluginSpecParsing('@test/test-plugin@latest', '@test/', 'test-plugin', 'latest');
+        checkPluginSpecParsing('@test/test-plugin@1.0.0', '@test/test-plugin', '1.0.0');
+        checkPluginSpecParsing('@test/test-plugin@latest', '@test/test-plugin', 'latest');
     });
     it('Test 005 : should handle invalid package specs', function () {
-        checkPluginSpecParsing('@nonsense', null, null, null);
-        checkPluginSpecParsing('@/nonsense', null, null, null);
-        checkPluginSpecParsing('@', null, null, null);
-        checkPluginSpecParsing('@nonsense@latest', null, null, null);
-        checkPluginSpecParsing('@/@', null, null, null);
-        checkPluginSpecParsing('/', null, null, null);
-        checkPluginSpecParsing('../../@directory', null, null, null);
-        checkPluginSpecParsing('@directory/../@directory', null, null, null);
-        checkPluginSpecParsing('./directory', null, null, null);
-        checkPluginSpecParsing('directory/directory', null, null, null);
-        checkPluginSpecParsing('http://cordova.apache.org', null, null, null);
+        checkPluginSpecParsing('@nonsense', null, null);
+        checkPluginSpecParsing('@/nonsense', null, null);
+        checkPluginSpecParsing('@', null, null);
+        checkPluginSpecParsing('@nonsense@latest', null, null);
+        checkPluginSpecParsing('@/@', null, null);
+        checkPluginSpecParsing('/', null, null);
+        checkPluginSpecParsing('../../@directory', null, null);
+        checkPluginSpecParsing('@directory/../@directory', null, null);
+        checkPluginSpecParsing('./directory', null, null);
+        checkPluginSpecParsing('directory/directory', null, null);
+        checkPluginSpecParsing('http://cordova.apache.org', null, null);
     });
 });

spec/cordova/util.spec.js

@@ -145,6 +145,11 @@ describe('util module', function () {
             fs.ensureSymlinkSync(linkedPluginPath, pluginLinkPath);
             expectFindPluginsToReturn(plugins.concat('plugin-link'));
         });
+
+        it('Test 032 : should work with scoped plugins', () => {
+            fs.ensureDirSync(path.join(pluginsDir, '@baz/foo'));
+            expectFindPluginsToReturn(plugins.concat('@baz/foo'));
+        });
     });
 
     describe('preprocessOptions method', function () {

spec/plugman/util/metadata.spec.js

@@ -53,7 +53,7 @@ describe('plugman.metadata', () => {
 
     describe('get_fetch_metadata', () => {
         const get_fetch_metadata = pluginId =>
-            metadata.get_fetch_metadata(path.join(pluginsDir, pluginId));
+            metadata.get_fetch_metadata(pluginsDir, pluginId);
 
         it('should return an empty object if there is no record', () => {
             fetchJson = null;
@@ -64,6 +64,13 @@ describe('plugman.metadata', () => {
             expect(get_fetch_metadata(TEST_PLUGIN)).toEqual({ metadata: 'matches' });
         });
 
+        it('should return the fetch metadata in plugins_dir/fetch.json for a scoped plugin', () => {
+            const meta = { metadata: 'matches' };
+            fetchJson = JSON.stringify({ '@cordova/plugin-thinger': meta });
+
+            expect(get_fetch_metadata('@cordova/plugin-thinger')).toEqual(meta);
+        });
+
         describe('cache behaviour', () => {
             beforeEach(() => {
                 spyOn(fsMock, 'readFileSync').and.callThrough();

src/cordova/platform/addHelper.js

@@ -313,10 +313,9 @@ function installPluginsForNewPlatform (platform, projectRoot, opts) {
     return plugins.reduce(function (soFar, plugin) {
         return soFar.then(function () {
             events.emit('verbose', 'Installing plugin "' + plugin + '" following successful platform add of ' + platform);
-            plugin = path.basename(plugin);
 
             // Get plugin variables from fetch.json if have any and pass them as cli_variables to plugman
-            var pluginMetadata = fetchMetadata.get_fetch_metadata(path.join(plugins_dir, plugin));
+            var pluginMetadata = fetchMetadata.get_fetch_metadata(plugins_dir, plugin);
 
             var options = {
                 searchpath: opts.searchpath,

src/cordova/plugin/add.js

@@ -123,7 +123,7 @@ function add (projectRoot, hooksRunner, opts) {
                         };
 
                         events.emit('verbose', 'Calling plugman.install on plugin "' + pluginInfo.dir + '" for platform "' + platform);
-                        return plugman.install(platform, platformRoot, path.basename(pluginInfo.dir), pluginPath, options)
+                        return plugman.install(platform, platformRoot, pluginInfo.id, pluginPath, options)
                             .then(function (didPrepare) {
                                 // If platform does not returned anything we'll need
                                 // to trigger a prepare after all plugins installed
@@ -166,18 +166,12 @@ function add (projectRoot, hooksRunner, opts) {
                             attributes.spec = src;
                         } else {
                             var ver = '~' + pluginInfo.version;
-                            // Scoped packages need to have the package-spec along with the version
-                            var parsedSpec = pluginSpec.parse(target);
                             if (pkgJson && pkgJson.dependencies && pkgJson.dependencies[pluginInfo.id]) {
                                 attributes.spec = pkgJson.dependencies[pluginInfo.id];
                             } else if (pkgJson && pkgJson.devDependencies && pkgJson.devDependencies[pluginInfo.id]) {
                                 attributes.spec = pkgJson.devDependencies[pluginInfo.id];
                             } else {
-                                if (parsedSpec.scope) {
-                                    attributes.spec = parsedSpec.package + '@' + ver;
-                                } else {
-                                    attributes.spec = ver;
-                                }
+                                attributes.spec = ver;
                             }
                         }
                     }
@@ -256,7 +250,7 @@ function determinePluginTarget (projectRoot, cfg, target, fetchOptions) {
         }
     } */
 
-    if (cordova_util.isUrl(parsedSpec.version) || cordova_util.isDirectory(parsedSpec.version) || pluginSpec.parse(parsedSpec.version).scope) {
+    if (cordova_util.isUrl(parsedSpec.version) || cordova_util.isDirectory(parsedSpec.version)) {
         return Promise.resolve(parsedSpec.version);
     }
 

src/cordova/plugin/plugin_spec_parser.js

@@ -26,22 +26,18 @@ module.exports.parse = parse;
  * Represents a parsed specification for a plugin
  * @class
  * @param {String} raw      The raw specification (i.e. provided by the user)
- * @param {String} scope    The scope of the package if this is an npm package
  * @param {String} id       The id of the package if this is an npm package
  * @param {String} version  The version specified for the package if this is an npm package
  */
-function PluginSpec (raw, scope, id, version) {
-    /** @member {String|null} The npm scope of the plugin spec or null if it does not have one */
-    this.scope = scope || null;
-
+function PluginSpec (raw, id, version) {
     /** @member {String|null} The id of the plugin or the raw plugin spec if it is not an npm package */
     this.id = id || raw;
 
     /** @member {String|null} The specified version of the plugin or null if no version was specified */
     this.version = version || null;
 
     /** @member {String|null} The npm package of the plugin (with scope) or null if this is not a spec for an npm package */
-    this.package = (scope ? scope + id : id) || null;
+    this.package = id;
 }
 
 /**
@@ -54,7 +50,7 @@ function PluginSpec (raw, scope, id, version) {
 function parse (raw) {
     var split = NPM_SPEC_REGEX.exec(raw);
     if (split) {
-        return new PluginSpec(raw, split[1], split[2], split[3]);
+        return new PluginSpec(raw, (split[1] || '') + split[2], split[3]);
     }
 
     return new PluginSpec(raw);

src/cordova/util.js

@@ -22,6 +22,7 @@ var path = require('path');
 var events = require('cordova-common').events;
 var CordovaError = require('cordova-common').CordovaError;
 var url = require('url');
+const globby = require('globby');
 
 var origCwd = null;
 
@@ -213,17 +214,12 @@ function getPlatformVersionOrNull (platformPath) {
 
 // list the directories in the path, ignoring any files
 function findPlugins (pluginDir) {
-    var plugins = [];
+    if (!fs.existsSync(pluginDir)) return [];
 
-    if (fs.existsSync(pluginDir)) {
-        plugins = fs.readdirSync(pluginDir).filter(function (fileName) {
-            var pluginPath = path.join(pluginDir, fileName);
-            var isPlugin = isDirectory(pluginPath) || isSymbolicLink(pluginPath);
-            return fileName !== '.svn' && fileName !== 'CVS' && isPlugin;
-        });
-    }
-
-    return plugins;
+    return globby.sync([ '*', '!@*', '@*/*', '!CVS' ], {
+        cwd: pluginDir,
+        onlyDirectories: true
+    });
 }
 
 function projectWww (projectDir) {
@@ -293,20 +289,6 @@ function isDirectory (dir) {
     }
 }
 
-/**
- * Checks to see if the argument is a symbolic link
- *
- * @param {string} dir - string representing path of directory
- * @return {boolean}
- */
-function isSymbolicLink (dir) {
-    try {
-        return fs.lstatSync(dir).isSymbolicLink();
-    } catch (e) {
-        return false;
-    }
-}
-
 // Returns the API of the platform contained in `dir`.
 // Potential errors : module isn't found, can't load or doesn't implement the expected interface.
 function getPlatformApiFunction (dir) {

src/plugman/install.js

@@ -366,11 +366,7 @@ function runInstall (actions, platform, project_dir, plugin_dir, plugins_dir, op
 function installDependencies (install, dependencies, options) {
     events.emit('verbose', 'Dependencies detected, iterating through them...');
 
-    var top_plugins = path.join(options.plugin_src_dir || install.top_plugin_dir, '..');
-
-    // Add directory of top-level plugin to search path
     options.searchpath = options.searchpath || [];
-    if (top_plugins !== install.plugins_dir && options.searchpath.indexOf(top_plugins) === -1) { options.searchpath.push(top_plugins); }
 
     // Search for dependency by Id is:
     // a) Look for {$top_plugins}/{$depId} directory
@@ -410,7 +406,7 @@ function tryFetchDependency (dep, install, options) {
     var relativePath;
     if (dep.url === '.') {
         // Look up the parent plugin's fetch metadata and determine the correct URL.
-        var fetchdata = require('./util/metadata').get_fetch_metadata(install.top_plugin_dir);
+        var fetchdata = require('./util/metadata').get_fetch_metadata(install.plugins_dir, install.top_plugin_id);
         if (!fetchdata || !(fetchdata.source && fetchdata.source.type)) {
             relativePath = dep.subdir || dep.id;
 

src/plugman/uninstall.js

@@ -138,7 +138,7 @@ module.exports.uninstallPlugin = function (id, plugins_dir, options) {
     // Recursively remove plugins which were installed as dependents (that are not top-level)
     var toDelete = [];
     function findDependencies (pluginId) {
-        var depPluginDir = path.join(plugin_dir, '..', pluginId);
+        var depPluginDir = path.join(plugins_dir, pluginId);
         // Skip plugin check for dependencies if it does not exist (CB-7846).
         if (!fs.existsSync(depPluginDir)) {
             events.emit('verbose', 'Plugin "' + pluginId + '" does not exist (' + depPluginDir + ')');

src/plugman/util/metadata.js

@@ -34,10 +34,7 @@ function getJson (pluginsDir) {
     return cachedJson;
 }
 
-exports.get_fetch_metadata = function (plugin_dir) {
-    var pluginsDir = path.dirname(plugin_dir);
-    var pluginId = path.basename(plugin_dir);
-
+exports.get_fetch_metadata = function (pluginsDir, pluginId) {
     var metadataJson = getJson(pluginsDir);
     return metadataJson[pluginId] || {};
 };