Repository: oclif/core
Pull request #204 (2021-07-15 14:39:36)

fix: make findCommand deterministic
@W-9514894@
when duplicate commands are found across all plugins always choose the command from the plugin that appears first in pjson.oclif.plugins


Patch:
diff --git a/src/config/config.ts b/src/config/config.ts
index aa2d3f0..ce8df66 100644
--- a/src/config/config.ts
+++ b/src/config/config.ts
@@ -7,12 +7,9 @@ import {format} from 'util'
 
 import {Options, Plugin as IPlugin} from '../interfaces/plugin'
 import {Config as IConfig, ArchTypes, PlatformTypes, LoadOptions} from '../interfaces/config'
-import {Command} from '../interfaces/command'
+import {Command, Hook, PJSON, Topic} from '../interfaces'
 import {Debug} from './util'
-import {Hook} from '../interfaces/hooks'
-import {PJSON} from '../interfaces/pjson'
 import * as Plugin from './plugin'
-import {Topic} from '../interfaces/topic'
 import {compact, flatMap, loadJSON, uniq} from './util'
 import {isProd} from '../util'
 import ModuleLoader from '../module-loader'
@@ -93,7 +90,8 @@ export class Config implements IConfig {
   private _topics?: Topic[]
 
   // eslint-disable-next-line no-useless-constructor
-  constructor(public options: Options) {}
+  constructor(public options: Options) {
+  }
 
   static async load(opts: LoadOptions = (module.parent && module.parent.parent && module.parent.parent.filename) || __dirname) {
     // Handle the case when a file URL string is passed in such as 'import.meta.url'; covert to file path.
@@ -224,7 +222,7 @@ export class Config implements IConfig {
         log(message?: any, ...args: any[]) {
           process.stdout.write(format(message, ...args) + '\n')
         },
-        error(message, options: {code?: string; exit?: number} = {}) {
+        error(message, options: { code?: string; exit?: number } = {}) {
           error(message, options)
         },
         warn(message: string) {
@@ -287,21 +285,65 @@ export class Config implements IConfig {
     .toUpperCase()
   }
 
-  findCommand(id: string, opts: {must: true}): Command.Plugin
-
-  findCommand(id: string, opts?: {must: boolean}): Command.Plugin | undefined
-
-  findCommand(id: string, opts: {must?: boolean} = {}): Command.Plugin | undefined {
-    const command = this.commands.find(c => c.id === id || c.aliases.includes(id))
-    if (command) return command
-    if (opts.must) error(`command ${id} not found`)
+  findCommand(id: string, opts: { must: true }): Command.Plugin
+
+  findCommand(id: string, opts?: { must: boolean }): Command.Plugin | undefined
+
+  /**
+   * This function is responsible for locating the correct plugin to use for a named command id
+   * It searches the {Config} registered commands to match either the raw command id or the command alias
+   * It is possible that more than one command will be found. This is due the ability of two distinct plugins to
+   * create the same command or command alias.
+   *
+   * In the case of more than one found command, the function will select the command based on the order in which
+   * the plugin is included in the package.json `oclif.plugins` list. The command that occurs first in the list
+   * is selected as the command to run.
+   *
+   * Commands can also be present from either an install or a link. When a command is one of these and a core plugin
+   * is present, this function defers to the core plugin.
+   *
+   * If there is not a core plugin command present, this function will return the first
+   * plugin as discovered (will not change the order)
+   * @param id raw command id or command alias
+   * @param opts options to control if the command must be found
+   * @returns command instance {Command.Plugin} or undefined
+   */
+  findCommand(id: string, opts: { must?: boolean } = {}): Command.Plugin | undefined {
+    const commands = this.commands.filter(c => c.id === id || c.aliases.includes(id))
+    if (opts.must && commands.length === 0) error(`command ${id} not found`)
+    if (commands.length === 1) return commands[0]
+
+    // more than one command found across available plugins
+    const oclifPlugins = this.pjson.oclif?.plugins ?? []
+    const commandPlugins = commands.sort((a, b) => {
+      const pluginAliasA = a.pluginAlias ?? 'A-Cannot-Find-This'
+      const pluginAliasB = b.pluginAlias ?? 'B-Cannot-Find-This'
+      const aIndex = oclifPlugins.indexOf(pluginAliasA)
+      const bIndex = oclifPlugins.indexOf(pluginAliasB)
+      // When both plugin types are 'core' plugins sort based on index
+      if (a.pluginType === 'core' && b.pluginType === 'core') {
+        // If b appears first in the pjson.plugins sort it first
+        return aIndex - bIndex
+      }
+      // if b is a core plugin and a is not sort b first
+      if (b.pluginType === 'core' && a.pluginType !== 'core') {
+        return 1
+      }
+      // if a is a core plugin and b is not sort a first
+      if (a.pluginType === 'core' && b.pluginType !== 'core') {
+        return -1
+      }
+      // neither plugin is core, so do not change the order
+      return 0
+    })
+    return commandPlugins[0]
   }
 
-  findTopic(id: string, opts: {must: true}): Topic
+  findTopic(id: string, opts: { must: true }): Topic
 
-  findTopic(id: string, opts?: {must: boolean}): Topic | undefined
+  findTopic(id: string, opts?: { must: boolean }): Topic | undefined
 
-  findTopic(name: string, opts: {must?: boolean} = {}) {
+  findTopic(name: string, opts: { must?: boolean } = {}) {
     const topic = this.topics.find(t => t.name === name)
     if (topic) return topic
     if (opts.must) throw new Error(`topic ${name} not found`)
@@ -403,11 +445,12 @@ export class Config implements IConfig {
     try {
       const {enabled} = require('debug')(this.bin)
       if (enabled) return 1
-    } catch {}
+    } catch {
+    }
     return 0
   }
 
-  protected async loadPlugins(root: string, type: string, plugins: (string | {root?: string; name?: string; tag?: string})[], parent?: Plugin.Plugin) {
+  protected async loadPlugins(root: string, type: string, plugins: (string | { root?: string; name?: string; tag?: string })[], parent?: Plugin.Plugin) {
     if (!plugins || plugins.length === 0) return
     debug('loading plugins', plugins)
     await Promise.all((plugins || []).map(async plugin => {
@@ -437,7 +480,7 @@ export class Config implements IConfig {
     }))
   }
 
-  protected warn(err: string | Error | {name: string; detail: string}, scope?: string) {
+  protected warn(err: string | Error | { name: string; detail: string }, scope?: string) {
     if (this.warned) return
 
     if (typeof err === 'string') {
@@ -539,6 +582,7 @@ export async function toCached(c: Command.Class, plugin?: IPlugin): Promise<Comm
     strict: c.strict,
     usage: c.usage,
     pluginName: plugin && plugin.name,
+    pluginAlias: plugin && plugin.alias,
     pluginType: plugin && plugin.type,
     hidden: c.hidden,
     aliases: c.aliases || [],
diff --git a/src/config/plugin.ts b/src/config/plugin.ts
index b4c38a4..c71bec4 100644
--- a/src/config/plugin.ts
+++ b/src/config/plugin.ts
@@ -79,6 +79,8 @@ export class Plugin implements IPlugin {
 
   root!: string
 
+  alias!: string
+
   tag?: string
 
   manifest!: Manifest
@@ -112,6 +114,7 @@ export class Plugin implements IPlugin {
     this._debug('reading %s plugin %s', this.type, root)
     this.pjson = await loadJSON(path.join(root, 'package.json')) as any
     this.name = this.pjson.name
+    this.alias = this.options.name ?? this.pjson.name
     const pjsonPath = path.join(root, 'package.json')
     if (!this.name) throw new Error(`no name in ${pjsonPath}`)
     if (!isProd() && !this.pjson.files) this.warn(`files attribute must be specified in ${pjsonPath}`)
@@ -128,7 +131,7 @@ export class Plugin implements IPlugin {
 
     this.manifest = await this._manifest(Boolean(this.options.ignoreManifest), Boolean(this.options.errorOnManifestCreate))
     this.commands = Object.entries(this.manifest.commands)
-    .map(([id, c]) => ({...c, load: async () => this.findCommand(id, {must: true})}))
+    .map(([id, c]) => ({...c, pluginAlias: this.alias, pluginType: this.type, load: async () => this.findCommand(id, {must: true})}))
     this.commands.sort((a, b) => {
       if (a.id < b.id) return -1
       if (a.id > b.id) return 1
diff --git a/src/interfaces/command.ts b/src/interfaces/command.ts
index e13e0cf..20068c4 100644
--- a/src/interfaces/command.ts
+++ b/src/interfaces/command.ts
@@ -55,6 +55,7 @@ export interface Command extends CommandProps {
   type?: string;
   pluginName?: string;
   pluginType?: string;
+  pluginAlias?: string;
   flags: {[name: string]: Command.Flag};
   args: Command.Arg[];
   strict: boolean;
diff --git a/src/interfaces/plugin.ts b/src/interfaces/plugin.ts
index 668ea02..2316218 100644
--- a/src/interfaces/plugin.ts
+++ b/src/interfaces/plugin.ts
@@ -30,6 +30,10 @@ export interface Plugin {
    * name from package.json
    */
   name: string;
+  /**
+   * aliases from package.json dependencies
+   */
+  alias: string;
   /**
    * version from package.json
    *


Test patch:
diff --git a/test/config/config.test.ts b/test/config/config.test.ts
index 2b70f72..104c194 100644
--- a/test/config/config.test.ts
+++ b/test/config/config.test.ts
@@ -1,16 +1,48 @@
 import * as os from 'os'
 import * as path from 'path'
 
-import {Config, Interfaces} from '../../src'
+import {Config} from '../../src/config/config'
+import {Plugin as IPlugin} from '../../src/interfaces'
 import * as util from '../../src/config/util'
+import {Command as ICommand} from '../../src/interfaces'
 
 import {expect, fancy} from './test'
+import {Interfaces} from '../../src'
 
 interface Options {
   pjson?: any;
   homedir?: string;
   platform?: string;
   env?: {[k: string]: string};
+  commandIds?: string[];
+  types?: string[];
+}
+
+const pjson = {
+  name: 'foo',
+  version: '1.0.0',
+  files: [],
+  commands: {},
+  oclif: {
+    topics: {
+      t1: {
+        description: 'desc for t1',
+        subtopics: {
+          't1-1': {
+            description: 'desc for t1-1',
+            subtopics: {
+              't1-1-1': {
+                description: 'desc for t1-1-1',
+              },
+              't1-1-2': {
+                description: 'desc for t1-1-2',
+              },
+            },
+          },
+        },
+      },
+    },
+  },
 }
 
 describe('Config', () => {
@@ -125,34 +157,169 @@ describe('Config', () => {
   })
 
   testConfig({
-    pjson: {
-      name: 'foo',
-      version: '1.0.0',
-      files: [],
-      commands: {},
-      oclif: {
-        topics: {
-          t1: {
-            description: 'desc for t1',
-            subtopics: {
-              't1-1': {
-                description: 'desc for t1-1',
-                subtopics: {
-                  't1-1-1': {
-                    description: 'desc for t1-1-1',
-                  },
-                  't1-1-2': {
-                    description: 'desc for t1-1-2',
-                  },
-                },
-              },
-            },
-          },
-        },
-      },
-    },
+    pjson,
   })
   .it('has subtopics', config => {
     expect(config.topics.map(t => t.name)).to.have.members(['t1', 't1:t1-1', 't1:t1-1:t1-1-1', 't1:t1-1:t1-1-2'])
   })
+
+  describe('findCommand', () => {
+    const findCommandTestConfig = ({pjson,
+      homedir = '/my/home',
+      platform = 'darwin',
+      env = {},
+      commandIds = ['foo:bar', 'foo:baz'],
+      types = [],
+    }: Options = {}) => {
+    // eslint-disable-next-line @typescript-eslint/ban-ts-ignore
+    // @ts-ignore
+      class MyComandClass implements ICommand.Class {
+      _base = ''
+
+      aliases: string[] = []
+
+      hidden = false
+
+      id = 'foo:bar'
+
+      new(): ICommand.Instance {
+        return {_run(): Promise<any> {
+          return Promise.resolve()
+        }}
+      }
+
+      run(): PromiseLike<any> {
+        return Promise.resolve(undefined)
+      }
+      }
+      const load = async (): Promise<void>  => {}
+      const findCommand = async (): Promise<ICommand.Class> => {
+      // eslint-disable-next-line @typescript-eslint/ban-ts-ignore
+      // @ts-ignore
+        return new MyComandClass()
+      }
+      const commandPluginA: ICommand.Plugin = {
+        strict: false,
+        aliases: [], args: [], flags: {}, hidden: false, id: commandIds[0], async load(): Promise<ICommand.Class> {
+          return new MyComandClass() as unknown as ICommand.Class
+        },
+        pluginType: types[0] ?? 'core',
+        pluginAlias: '@My/plugina',
+      }
+      const commandPluginB: ICommand.Plugin = {
+        strict: false,
+        aliases: [], args: [], flags: {}, hidden: false, id: commandIds[1], async load(): Promise<ICommand.Class> {
+          return new MyComandClass() as unknown as ICommand.Class
+        },
+        pluginType: types[1] ?? 'core',
+        pluginAlias: '@My/pluginb',
+      }
+      const hooks = {}
+      const pluginA: IPlugin = {load,
+        findCommand,
+        name: '@My/plugina',
+        alias: '@My/plugina',
+        commands: [commandPluginA],
+        _base: '',
+        pjson,
+        commandIDs: [commandIds[0]] as string[],
+        root: '',
+        version: '0.0.0',
+        type: types[0] ?? 'core',
+        hooks,
+        topics: [],
+        valid: true,
+        tag: 'tag',
+      }
+      commandPluginA
+      const pluginB: IPlugin = {
+        load,
+        findCommand,
+        name: '@My/pluginb',
+        alias: '@My/pluginb',
+        commands: [commandPluginB],
+        _base: '',
+        pjson,
+        commandIDs: [commandIds[1]] as string[],
+        root: '',
+        version: '0.0.0',
+        type: types[1] ?? 'core',
+        hooks,
+        topics: [],
+        valid: true,
+        tag: 'tag',
+      }
+      const plugins: IPlugin[] = [pluginA, pluginB]
+      let test = fancy
+      .resetConfig()
+      .env(env, {clear: true})
+      .stub(os, 'homedir', () => path.join(homedir))
+      .stub(os, 'platform', () => platform)
+
+      if (pjson) test = test.stub(util, 'loadJSON', () => Promise.resolve(pjson))
+      test = test.add('config', async () => {
+        const config = await Config.load()
+        config.plugins = plugins
+        config.pjson.oclif.plugins = ['@My/pluginb', '@My/plugina']
+        config.pjson.dependencies = {'@My/pluginb': '0.0.0', '@My/plugina': '0.0.0'}
+        return config
+      })
+      // eslint-disable-next-line @typescript-eslint/ban-ts-ignore
+      // @ts-ignore
+      return {
+        it(expectation: string, fn: (config: Interfaces.Config) => any) {
+          test
+          .do(({config}) => fn(config))
+          .it(expectation)
+          return this
+        },
+      }
+    }
+
+    findCommandTestConfig()
+    .it('find command with no duplicates', config => {
+      const command = config.findCommand('foo:bar', {must: true})
+      expect(command).to.have.property('pluginAlias', '@My/plugina')
+    })
+    findCommandTestConfig({commandIds: ['foo:bar', 'foo:bar']})
+    .it('find command with duplicates and choose the one that appears first in oclif.plugins', config => {
+      const command = config.findCommand('foo:bar', {must: true})
+      expect(command).to.have.property('pluginAlias', '@My/pluginb')
+    })
+    findCommandTestConfig({types: ['core', 'user']})
+    .it('find command with no duplicates core/user', config => {
+      const command = config.findCommand('foo:bar', {must: true})
+      expect(command).to.have.property('id', 'foo:bar')
+      expect(command).to.have.property('pluginType', 'core')
+      expect(command).to.have.property('pluginAlias', '@My/plugina')
+    })
+    findCommandTestConfig({types: ['user', 'core']})
+    .it('find command with no duplicates user/core', config => {
+      const command = config.findCommand('foo:bar', {must: true})
+      expect(command).to.have.property('id', 'foo:bar')
+      expect(command).to.have.property('pluginType', 'user')
+      expect(command).to.have.property('pluginAlias', '@My/plugina')
+    })
+    findCommandTestConfig({commandIds: ['foo:bar', 'foo:bar'], types: ['core', 'user']})
+    .it('find command with duplicates core/user', config => {
+      const command = config.findCommand('foo:bar', {must: true})
+      expect(command).to.have.property('id', 'foo:bar')
+      expect(command).to.have.property('pluginType', 'core')
+      expect(command).to.have.property('pluginAlias', '@My/plugina')
+    })
+    findCommandTestConfig({commandIds: ['foo:bar', 'foo:bar'], types: ['user', 'core']})
+    .it('find command with duplicates user/core', config => {
+      const command = config.findCommand('foo:bar', {must: true})
+      expect(command).to.have.property('id', 'foo:bar')
+      expect(command).to.have.property('pluginType', 'core')
+      expect(command).to.have.property('pluginAlias', '@My/pluginb')
+    })
+    findCommandTestConfig({commandIds: ['foo:bar', 'foo:bar'], types: ['user', 'user']})
+    .it('find command with duplicates user/user', config => {
+      const command = config.findCommand('foo:bar', {must: true})
+      expect(command).to.have.property('id', 'foo:bar')
+      expect(command).to.have.property('pluginType', 'user')
+      expect(command).to.have.property('pluginAlias', '@My/plugina')
+    })
+  })
 })
diff --git a/test/module-loader/fixtures/cjs/success.js b/test/module-loader/fixtures/cjs/success.js
index 6b5db4c..31a6702 100644
--- a/test/module-loader/fixtures/cjs/success.js
+++ b/test/module-loader/fixtures/cjs/success.js
@@ -1,2 +1,2 @@
-module.exports = ['SUCCESS'];
-module.exports.namedExport = 'SUCCESS_NAMED';
+module.exports = ['SUCCESS']
+module.exports.namedExport = 'SUCCESS_NAMED'