fix(appium): allow multiple drivers to be installed

Closes #16674

the problem here was the existence of the empty `package.json`.  it should exist, because if it doesn't, then `npm` may end up using a `package.json` in a parent directory.  but it can't be _empty_, because subsequent `npm install`s will essentially remove any package that it's currently not installing!  `npm` likes to "fix" `node_modules`, which is undesirable here.

to avoid this, the behavior has changed to persisting the dependency version to `package.json`.  behavior differs if we're using a local `APPIUM_HOME` or the default one.  with the default, we have more control, and choose to use `--save-dev --save-exact --no-package-lock --global-style` which gives us what we need.  if `APPIUM_HOME` is a local package, all we can do is `--save-dev` (since it's _likely_ that an extension will be a dev dependency).  in the future, we may choose to actually determine _what kind_ of dependency `appium` is and tell `npm` to use the same one.  not sure if that's needed, though.

further, in the case of a local appium dep (local `APPIUM_HOME`), updating `package.json` should cause the package hash (used by the thing that syncs `extensions.yaml` with installed packages) to update as well, so we're doing that.

modified the logic in `ExtensionCommand` to treat `--source=local` to pass the `installSpec` thru verbatim.

also fixed `BuildInfo` type to reflect reality and use `@types/teen_process`.  fixed some other broken types in `test` (files in `test` are not checked by typescript yet; we'll need a separate `tsconfig.json` for this, since we do not want to emit declarations)

packages/appium/lib/cli/extension-command.js

@@ -1,11 +1,13 @@
 /* eslint-disable no-console */
 
 import _ from 'lodash';
-import { npm, fs, util } from '@appium/support';
+import path from 'path';
+import { npm, fs, util, env } from '@appium/support';
 import { log, spinWith, RingBuffer } from './utils';
 import { SubProcess } from 'teen_process';
 import { INSTALL_TYPE_NPM, INSTALL_TYPE_GIT, INSTALL_TYPE_GITHUB,
          INSTALL_TYPE_LOCAL } from '../extension/extension-config';
+import { packageDidChange } from '../extension/package-changed';
 
 const UPDATE_ALL = 'installed';
 
@@ -184,14 +186,7 @@ class ExtensionCommand {
       throw new Error(`When using --source=${installType}, must also use --package`);
     }
 
-    if (installType === INSTALL_TYPE_LOCAL) {
-      const msg = `Linking ${this.type} from local path into ${this.config.appiumHome}`;
-      const pkgJsonData = await spinWith(this.isJsonOutput, msg, async () => (
-        await npm.linkPackage(this.config.appiumHome, installSpec))
-      );
-      extData = this.getExtensionFields(pkgJsonData);
-      log(this.isJsonOutput, `Successfully linked ${extData.pkgName} into ${this.config.appiumHome}`);
-    } else if (installType === INSTALL_TYPE_GITHUB) {
+    if (installType === INSTALL_TYPE_GITHUB) {
       if (installSpec.split('/').length !== 2) {
         throw new Error(`Github ${this.type} spec ${installSpec} appeared to be invalid; ` +
                         'it should be of the form <org>/<repo>');
@@ -203,40 +198,44 @@ class ExtensionCommand {
       installSpec = installSpec.replace(/\.git$/, '');
       extData = await this.installViaNpm({ext: installSpec, pkgName: /** @type {string} */(packageName)});
     } else {
-      // at this point we have either an npm package or an appium verified extension
-      // name. both of which will be installed via npm.
-      // extensions installed via npm can include versions or tags after the '@'
-      // sign, so check for that. We also need to be careful that package names themselves can
-      // contain the '@' symbol, as in `npm install @appium/fake-driver@1.2.0`
-      let name, pkgVer;
-      const splits = installSpec.split('@');
-      if (installSpec[0] === '@') {
-        // this is the case where we have an npm org included in the package name
-        [name, pkgVer] = [`@${splits[1]}`, splits[2]];
+      let pkgName, pkgVer;
+      if (installType === INSTALL_TYPE_LOCAL) {
+        pkgName = path.isAbsolute(installSpec) ? installSpec : path.resolve(installSpec);
       } else {
-        // this is the case without an npm org
-        [name, pkgVer] = splits;
-      }
-      let pkgName;
+        // at this point we have either an npm package or an appium verified extension
+        // name or a local path. both of which will be installed via npm.
+        // extensions installed via npm can include versions or tags after the '@'
+        // sign, so check for that. We also need to be careful that package names themselves can
+        // contain the '@' symbol, as in `npm install @appium/fake-driver@1.2.0`
+        let name;
+        const splits = installSpec.split('@');
+        if (installSpec[0] === '@') {
+          // this is the case where we have an npm org included in the package name
+          [name, pkgVer] = [`@${splits[1]}`, splits[2]];
+        } else {
+          // this is the case without an npm org
+          [name, pkgVer] = splits;
+        }
 
-      if (installType === INSTALL_TYPE_NPM) {
-        // if we're installing a named package from npm, we don't need to check
-        // against the appium extension list; just use the installSpec as is
-        pkgName = name;
-      } else {
-        // if we're installing a named appium driver (like 'xcuitest') we need to
-        // dereference the actual npm package ('appiupm-xcuitest-driver'), so
-        // check it exists and get the correct package
-        const knownNames = Object.keys(this.knownExtensions);
-        if (!_.includes(knownNames, name)) {
-          const msg = `Could not resolve ${this.type}; are you sure it's in the list ` +
-                      `of supported ${this.type}s? ${JSON.stringify(knownNames)}`;
-          throw new Error(msg);
+        if (installType === INSTALL_TYPE_NPM) {
+          // if we're installing a named package from npm, we don't need to check
+          // against the appium extension list; just use the installSpec as is
+          pkgName = name;
+        } else {
+          // if we're installing a named appium driver (like 'xcuitest') we need to
+          // dereference the actual npm package ('appiupm-xcuitest-driver'), so
+          // check it exists and get the correct package
+          const knownNames = Object.keys(this.knownExtensions);
+          if (!_.includes(knownNames, name)) {
+            const msg = `Could not resolve ${this.type}; are you sure it's in the list ` +
+                        `of supported ${this.type}s? ${JSON.stringify(knownNames)}`;
+            throw new Error(msg);
+          }
+          pkgName = this.knownExtensions[name];
+          // given that we'll use the install type in the driver json, store it as
+          // 'npm' now
+          installType = INSTALL_TYPE_NPM;
         }
-        pkgName = this.knownExtensions[name];
-        // given that we'll use the install type in the driver json, store it as
-        // 'npm' now
-        installType = INSTALL_TYPE_NPM;
       }
 
       extData = await this.installViaNpm({ext, pkgName, pkgVer});
@@ -254,6 +253,11 @@ class ExtensionCommand {
     const extManifest = {...extData, installType, installSpec};
     await this.config.addExtension(extName, extManifest);
 
+    // update the if we've changed the local `package.json`
+    if (await env.hasAppiumDependency(this.config.appiumHome)) {
+      await packageDidChange(this.config.appiumHome);
+    }
+
     // log info for the user
     log(this.isJsonOutput, this.getPostInstallText({extName, extData}));
 

packages/appium/lib/config.js

@@ -86,7 +86,7 @@ async function getGitRev (useGithubApiFallback = false) {
 /**
  * @param {string} commitSha
  * @param {boolean} [useGithubApiFallback]
- * @returns {Promise<number?>}
+ * @returns {Promise<string?>}
  */
 async function getGitTimestamp (commitSha, useGithubApiFallback = false) {
   const gitRoot = await findGitRoot();

packages/appium/test/e2e/cli.e2e.spec.js

@@ -27,6 +27,8 @@ describe('CLI behavior', function () {
    */
   let appiumHome;
 
+  const testDriverPath = path.dirname(resolveFixture('test-driver/package.json'));
+
   describe('when appium is a dependency', function () {
     /** @type {string} */
     let hashPath;
@@ -142,28 +144,24 @@ describe('CLI behavior', function () {
         });
       });
 
-      describe('when a different driver is installed via appium', function () {
-        const testDriverPath = path.dirname(resolveFixture('test-driver/package.json'));
+      describe('when a different driver is installed via "appium driver install"', function () {
         /** @type {string} */
         let oldHash;
-        /** @type {string} */
-        let oldPkg;
 
         before(async function () {
           await runJson([DRIVER_TYPE, LIST]);
           oldHash = await readHash();
-          oldPkg = await fs.readFile(appiumHomePkgPath, 'utf8');
           await installLocalExtension(appiumHome, DRIVER_TYPE, testDriverPath);
         });
 
-        it('should not update package.json', async function () {
-          const newPkg = await fs.readFile(appiumHomePkgPath, 'utf8');
-          newPkg.should.equal(oldPkg);
+        it('should update package.json', async function () {
+          const newPkg = JSON.parse(await fs.readFile(appiumHomePkgPath, 'utf8'));
+          expect(newPkg).to.have.nested.property('devDependencies.test-driver');
         });
 
-        it('should not update the hash', async function () {
-          const newHash = await fs.readFile(path.join(hashPath), 'utf8');
-          newHash.should.equal(oldHash);
+        it('should update the hash', async function () {
+          const newHash = await readHash();
+          newHash.should.not.equal(oldHash);
         });
 
         it('should update the manifest with the new driver', async function () {
@@ -173,6 +171,11 @@ describe('CLI behavior', function () {
           manifestParsed.should.have.nested.property('drivers.test');
           manifestParsed.should.have.nested.property('drivers.fake');
         });
+
+        it('should actually install both drivers', function () {
+          expect(() => resolveFrom(appiumHome, '@appium/fake-driver')).not.to.throw;
+          expect(() => resolveFrom(appiumHome, 'test-driver')).not.to.throw;
+        });
       });
     });
   });
@@ -283,7 +286,6 @@ describe('CLI behavior', function () {
           ret.uiautomator2.installType.should.eql('npm');
           ret.uiautomator2.installSpec.should.eql('uiautomator2');
           const list = await runList(['--installed']);
-          // @ts-expect-error
           delete list.uiautomator2.installed;
           list.should.eql(ret);
         });
@@ -298,10 +300,40 @@ describe('CLI behavior', function () {
           ret.fake.installType.should.eql('npm');
           ret.fake.installSpec.should.eql('@appium/fake-driver');
           const list = await runList(['--installed']);
-          // @ts-expect-error
           delete list.fake.installed;
           list.should.eql(ret);
         });
+
+        it('should install a driver from npm and a local driver', async function () {
+          await clear();
+          await runInstall([
+            '@appium/fake-driver',
+            '--source',
+            'npm',
+          ]);
+          await installLocalExtension(appiumHome, 'driver', testDriverPath);
+          const list = await runList(['--installed']);
+          expect(list.fake).to.exist;
+          expect(list.test).to.exist;
+          expect(() => resolveFrom(appiumHome, '@appium/fake-driver')).not.to.throw;
+          expect(() => resolveFrom(appiumHome, 'test-driver')).not.to.throw;
+        });
+
+        it('should install _two_ drivers from npm', async function () {
+          await clear();
+          await runInstall([
+            '@appium/fake-driver',
+            '--source',
+            'npm',
+          ]);
+          await runInstall(['appium-uiautomator2-driver', '--source', 'npm']);
+          const list = await runList(['--installed']);
+          expect(list.fake).to.exist;
+          expect(list.uiautomator2).to.exist;
+          expect(() => resolveFrom(appiumHome, '@appium/fake-driver')).not.to.throw;
+          expect(() => resolveFrom(appiumHome, 'appium-uiautomator2-driver')).not.to.throw;
+        });
+
         it('should install a driver from npm with a specific version/tag', async function () {
           const currentFakeDriverVersionAsOfRightNow = '3.0.5';
           await clear();
@@ -311,7 +343,6 @@ describe('CLI behavior', function () {
           ret.fake.installType.should.eql('npm');
           ret.fake.installSpec.should.eql(installSpec);
           const list = await runList(['--installed']);
-          // @ts-expect-error
           delete list.fake.installed;
           list.should.eql(ret);
         });
@@ -328,7 +359,6 @@ describe('CLI behavior', function () {
           ret.fake.installType.should.eql('github');
           ret.fake.installSpec.should.eql('appium/appium-fake-driver');
           const list = await runList(['--installed']);
-          // @ts-expect-error
           delete list.fake.installed;
           list.should.eql(ret);
         });
@@ -345,7 +375,6 @@ describe('CLI behavior', function () {
           ret.fake.installType.should.eql('git');
           ret.fake.installSpec.should.eql(FAKE_DRIVER_DIR);
           const list = await runList(['--installed', '--json']);
-          // @ts-expect-error
           delete list.fake.installed;
           list.should.eql(ret);
         });
@@ -364,7 +393,6 @@ describe('CLI behavior', function () {
             'git+https://github.com/appium/appium-fake-driver',
           );
           const list = await runList(['--installed']);
-          // @ts-expect-error
           delete list.fake.installed;
           list.should.eql(ret);
         });
@@ -377,9 +405,13 @@ describe('CLI behavior', function () {
           ret.fake.installType.should.eql('local');
           ret.fake.installSpec.should.eql(FAKE_DRIVER_DIR);
           const list = await runList(['--installed']);
-          // @ts-expect-error
           delete list.fake.installed;
           list.should.eql(ret);
+
+          // it should be a link!  this may be npm-version dependent, but it's worked
+          // this way for quite awhile
+          const stat = await fs.lstat(path.join(appiumHome, 'node_modules', '@appium', 'fake-driver'));
+          expect(stat.isSymbolicLink()).to.be.true;
         });
       });
 
@@ -528,7 +560,7 @@ describe('CLI behavior', function () {
  * @typedef {import('../../lib/extension/manifest').PluginType} PluginType
  * @typedef {import('../../lib/cli/extension-command').ExtensionListData} ExtensionListData
  * @typedef {import('./e2e-helpers').CliArgs} CliArgs
- * @typedef {import('../../types/types').CliExtensionSubcommand} CliExtensionSubcommand
+ * @typedef {import('../../types/cli').CliExtensionSubcommand} CliExtensionSubcommand
  */
 
 /**

packages/appium/test/e2e/driver.e2e.spec.js

@@ -44,7 +44,7 @@ describe('FakeDriver - via HTTP', function () {
   let appiumHome;
   // since we update the FakeDriver.prototype below, make sure we update the FakeDriver which is
   // actually going to be required by Appium
-  /** @type {import('../../lib/extension/manifest').DriverClass} */
+  /** @type {import('../../types/extension').DriverClass} */
   let FakeDriver;
   /** @type {string} */
   let testServerBaseSessionUrl;
@@ -85,7 +85,7 @@ describe('FakeDriver - via HTTP', function () {
   /**
    *
    * @param {number} port
-   * @param {Partial<import('../../types/types').ParsedArgs>} [args]
+   * @param {Partial<import('../../types/cli').ParsedArgs>} [args]
    */
   async function serverStart (port, args = {}) {
     args = {...args, port, address: TEST_HOST};