fix(fund): correctly parse and use which config

Previously the `which` param in `npm fund` was validated incorrectly
leading to `EFUNDNUMBER` errors when used. This fixes that and does a
better job detecting when `which` is pointing to an incorrect array
bounds in the returned funding array.

Author: lukekarrys

lib/commands/fund.js

@@ -16,12 +16,27 @@ const getPrintableName = ({ name, version }) => {
   return `${name}${printableVersion}`
 }
 
+const errCode = (msg, code) => Object.assign(new Error(msg), { code })
+
 class Fund extends ArboristWorkspaceCmd {
   static description = 'Retrieve funding information'
   static name = 'fund'
   static params = ['json', 'browser', 'unicode', 'workspace', 'which']
   static usage = ['[<package-spec>]']
 
+  // XXX: maybe worth making this generic for all commands?
+  usageMessage (paramsObj = {}) {
+    let msg = `\`npm ${this.constructor.name}`
+    const params = Object.entries(paramsObj)
+    if (params.length) {
+      msg += ` ${this.constructor.usage}`
+    }
+    for (const [key, value] of params) {
+      msg += ` --${key}=${value}`
+    }
+    return `${msg}\``
+  }
+
   // TODO
   /* istanbul ignore next */
   async completion (opts) {
@@ -30,25 +45,23 @@ class Fund extends ArboristWorkspaceCmd {
 
   async exec (args) {
     const spec = args[0]
-    const numberArg = this.npm.config.get('which')
 
-    const fundingSourceNumber = numberArg && parseInt(numberArg, 10)
-
-    const badFundingSourceNumber =
-      numberArg !== null && (String(fundingSourceNumber) !== numberArg || fundingSourceNumber < 1)
-
-    if (badFundingSourceNumber) {
-      const err = new Error(
-        '`npm fund [<@scope>/]<pkg> [--which=fundingSourceNumber]` must be given a positive integer'
-      )
-      err.code = 'EFUNDNUMBER'
-      throw err
+    let fundingSourceNumber = this.npm.config.get('which')
+    if (fundingSourceNumber != null) {
+      fundingSourceNumber = parseInt(fundingSourceNumber, 10)
+      if (isNaN(fundingSourceNumber) || fundingSourceNumber < 1) {
+        throw errCode(
+          `${this.usageMessage({ which: 'fundingSourceNumber' })} must be given a positive integer`,
+          'EFUNDNUMBER'
+        )
+      }
     }
 
     if (this.npm.global) {
-      const err = new Error('`npm fund` does not support global packages')
-      err.code = 'EFUNDGLOBAL'
-      throw err
+      throw errCode(
+        `${this.usageMessage()} does not support global packages`,
+        'EFUNDGLOBAL'
+      )
     }
 
     const where = this.npm.prefix
@@ -146,6 +159,7 @@ class Fund extends ArboristWorkspaceCmd {
 
   async openFundingUrl ({ path, tree, spec, fundingSourceNumber }) {
     const arg = npa(spec, path)
+
     const retrievePackageMetadata = () => {
       if (arg.type === 'directory') {
         if (tree.path === arg.fetchSpec) {
@@ -178,32 +192,35 @@ class Fund extends ArboristWorkspaceCmd {
 
     const validSources = [].concat(normalizeFunding(funding)).filter(isValidFunding)
 
-    const matchesValidSource =
-      validSources.length === 1 ||
-      (fundingSourceNumber > 0 && fundingSourceNumber <= validSources.length)
-
-    if (matchesValidSource) {
-      const index = fundingSourceNumber ? fundingSourceNumber - 1 : 0
-      const { type, url } = validSources[index]
-      const typePrefix = type ? `${type} funding` : 'Funding'
-      const msg = `${typePrefix} available at the following URL`
-      return openUrl(this.npm, url, msg)
-    } else if (validSources.length && !(fundingSourceNumber >= 1)) {
-      validSources.forEach(({ type, url }, i) => {
-        const typePrefix = type ? `${type} funding` : 'Funding'
-        const msg = `${typePrefix} available at the following URL`
-        this.npm.output(`${i + 1}: ${msg}: ${url}`)
-      })
-      this.npm.output(
-        /* eslint-disable-next-line max-len */
-        'Run `npm fund [<@scope>/]<pkg> --which=1`, for example, to open the first funding URL listed in that package'
-      )
-    } else {
-      const noFundingError = new Error(`No valid funding method available for: ${spec}`)
-      noFundingError.code = 'ENOFUND'
+    if (!validSources.length) {
+      throw errCode(`No valid funding method available for: ${spec}`, 'ENOFUND')
+    }
 
-      throw noFundingError
+    const fundSource = fundingSourceNumber
+      ? validSources[fundingSourceNumber - 1]
+      : validSources.length === 1 ? validSources[0]
+      : null
+
+    if (fundSource) {
+      return openUrl(this.npm, ...this.urlMessage(fundSource))
+    }
+
+    const ambiguousUrlMsg = [
+      ...validSources.map((s, i) => `${i + 1}: ${this.urlMessage(s).reverse().join(': ')}`),
+      `Run ${this.usageMessage({ which: '1' })}` +
+      ', for example, to open the first funding URL listed in that package',
+    ]
+    if (fundingSourceNumber) {
+      ambiguousUrlMsg.unshift(`--which=${fundingSourceNumber} is not a valid index`)
     }
+    this.npm.output(ambiguousUrlMsg.join('\n'))
+  }
+
+  urlMessage (source) {
+    const { type, url } = source
+    const typePrefix = type ? `${type} funding` : 'Funding'
+    const message = `${typePrefix} available at the following URL`
+    return [url, message]
   }
 }
 module.exports = Fund

tap-snapshots/test/lib/commands/fund.js.test.cjs

@@ -8,8 +8,7 @@
 exports[`test/lib/commands/fund.js TAP fund a package with type and multiple sources > should print prompt select message 1`] = `
 1: Foo funding available at the following URL: http://example.com/foo
 2: Lorem funding available at the following URL: http://example.com/foo-lorem
-Run \`npm fund [<@scope>/]<pkg> --which=1\`, for example, to open the first funding URL listed in that package
-
+Run \`npm fund [<package-spec>] --which=1\`, for example, to open the first funding URL listed in that package
 `
 
 exports[`test/lib/commands/fund.js TAP fund colors > should print output with color info 1`] = `
@@ -23,7 +22,6 @@ exports[`test/lib/commands/fund.js TAP fund colors > should print output with co
   \`-- http://example.com/e
       \`-- [email protected]
 
-
 `
 
 exports[`test/lib/commands/fund.js TAP fund containing multi-level nested deps with no funding > should omit dependencies with no funding declared 1`] = `
@@ -33,54 +31,37 @@ [email protected]
 \`-- http://example.com/donate
     \`-- [email protected]
 
-
 `
 
 exports[`test/lib/commands/fund.js TAP fund in which same maintainer owns all its deps > should print stack packages together 1`] = `
 http://example.com/donate
   \`-- [email protected], [email protected], [email protected], [email protected]
 
-
 `
 
 exports[`test/lib/commands/fund.js TAP fund pkg missing version number > should print name only 1`] = `
 http://example.com/foo
   \`-- foo
 
+`
 
+exports[`test/lib/commands/fund.js TAP fund using bad which value: index too high > should print message about invalid which 1`] = `
+--which=100 is not a valid index
+1: Funding available at the following URL: http://example.com
+2: Funding available at the following URL: http://sponsors.example.com/me
+3: Funding available at the following URL: http://collective.example.com
+Run \`npm fund [<package-spec>] --which=1\`, for example, to open the first funding URL listed in that package
 `
 
 exports[`test/lib/commands/fund.js TAP fund using nested packages with multiple sources > should prompt with all available URLs 1`] = `
 1: Funding available at the following URL: https://one.example.com
 2: Funding available at the following URL: https://two.example.com
-Run \`npm fund [<@scope>/]<pkg> --which=1\`, for example, to open the first funding URL listed in that package
-
-`
-
-exports[`test/lib/commands/fund.js TAP fund using nested packages with multiple sources, with a source number > should open the numbered URL 1`] = `
-Funding available at the following URL:
-  https://one.example.com
-`
-
-exports[`test/lib/commands/fund.js TAP fund using package argument > should open funding url 1`] = `
-individual funding available at the following URL:
-  http://example.com/donate
-`
-
-exports[`test/lib/commands/fund.js TAP fund using pkg name while having conflicting versions > should open greatest version 1`] = `
-Funding available at the following URL:
-  http://example.com/2
-`
-
-exports[`test/lib/commands/fund.js TAP fund using string shorthand > should open string-only url 1`] = `
-Funding available at the following URL:
-  https://example.com/sponsor
+Run \`npm fund [<package-spec>] --which=1\`, for example, to open the first funding URL listed in that package
 `
 
 exports[`test/lib/commands/fund.js TAP fund with no package containing funding > should print empty funding info 1`] = `
 [email protected]
 
-
 `
 
 exports[`test/lib/commands/fund.js TAP sub dep with fund info and a parent with no funding info > should nest sub dep as child of root 1`] = `
@@ -90,25 +71,22 @@ [email protected]
 \`-- http://example.com/c
     \`-- [email protected]
 
-
 `
 
-exports[`test/lib/commands/fund.js TAP workspaces filter funding info by a specific workspace > should display only filtered workspace name and its deps 1`] = `
+exports[`test/lib/commands/fund.js TAP workspaces filter funding info by a specific workspace name > should display only filtered workspace name and its deps 1`] = `
 [email protected]
 \`-- https://example.com/a
   | \`-- [email protected]
   \`-- http://example.com/c
       \`-- [email protected]
 
-
 `
 
-exports[`test/lib/commands/fund.js TAP workspaces filter funding info by a specific workspace > should display only filtered workspace path and its deps 1`] = `
+exports[`test/lib/commands/fund.js TAP workspaces filter funding info by a specific workspace path > should display only filtered workspace name and its deps 1`] = `
 [email protected]
 \`-- https://example.com/a
   | \`-- [email protected]
   \`-- http://example.com/c
       \`-- [email protected]
 
-
 `