fix(findBin): Resolve package script with args (#295)

Author: sudo-suhas

src/findBin.js

@@ -2,16 +2,50 @@
 
 const npmWhich = require('npm-which')(process.cwd())
 
-module.exports = function findBin(cmd, packageJson, options) {
+module.exports = function findBin(cmd, scripts, options) {
+  const npmArgs = (bin, args) =>
+    ['run', options && options.verbose ? undefined : '--silent', bin]
+      // args could be undefined but we filter that out.
+      .concat(args)
+      .filter(arg => arg !== undefined)
+
   /*
-    * If package.json has script with cmd defined
-    * we want it to be executed first
-    */
-  if (packageJson.scripts && packageJson.scripts[cmd] !== undefined) {
+   * If package.json has script with cmd defined we want it to be executed
+   * first. For finding the bin from scripts defined in package.json, there
+   * are 2 possibilities. It's a command which does not have any arguments
+   * passed to it in the lint-staged config. Or it could be something like
+   * `kcd-scripts` which has arguments such as `format`, `lint` passed to it.
+   * But we always cannot assume that the cmd, which has a space in it, is of
+   * the format `bin argument` because it is legal to define a package.json
+   * script with space in it's name. So we do 2 types of lookup. First a naive
+   * lookup which just looks for the scripts entry with cmd. Then we split on
+   * space, parse the bin and args, and try again.
+   *
+   * Related:
+   *  - https://github.com/kentcdodds/kcd-scripts/pull/3
+   *  - https://github.com/okonet/lint-staged/issues/294
+   *
+   * Example:
+   *
+   *   "scripts": {
+   *     "my cmd": "echo deal-wth-it",
+   *     "demo-bin": "node index.js"
+   *   },
+   *   "lint-staged": {
+   *     "*.js": ["my cmd", "demo-bin hello"]
+   *   }
+   */
+  if (scripts[cmd] !== undefined) {
     // Support for scripts from package.json
-    const args = ['run', options && options.verbose ? undefined : '--silent', cmd].filter(Boolean)
+    return { bin: 'npm', args: npmArgs(cmd) }
+  }
 
-    return { bin: 'npm', args }
+  const parts = cmd.split(' ')
+  let bin = parts[0]
+  const args = parts.splice(1)
+
+  if (scripts[bin] !== undefined) {
+    return { bin: 'npm', args: npmArgs(bin, args) }
   }
 
   /*
@@ -32,10 +66,6 @@ module.exports = function findBin(cmd, packageJson, options) {
     *  }
     */
 
-  const parts = cmd.split(' ')
-  let bin = parts[0]
-  const args = parts.splice(1)
-
   try {
     /* npm-which tries to resolve the bin in local node_modules/.bin */
     /* and if this fails it look in $PATH */

src/index.js

@@ -44,7 +44,9 @@ ${stringifyObject(config)}
 `)
       }
 
-      runAll(packageJson, config)
+      const scripts = packageJson.scripts || {}
+
+      runAll(scripts, config)
         .then(() => {
           // No errors, exiting with 0
           process.exitCode = 0

src/runAll.js

@@ -9,11 +9,11 @@ const resolveGitDir = require('./resolveGitDir')
 
 /**
  * Executes all tasks and either resolves or rejects the promise
- * @param packageJson
+ * @param scripts
  * @param config {Object}
  * @returns {Promise}
  */
-module.exports = function runAll(packageJson, config) {
+module.exports = function runAll(scripts, config) {
   // Config validation
   if (!config || !has(config, 'gitDir') || !has(config, 'concurrent') || !has(config, 'renderer')) {
     throw new Error('Invalid config provided to runAll! Use getConfig instead.')
@@ -35,7 +35,7 @@ module.exports = function runAll(packageJson, config) {
       const tasks = generateTasks(config, filenames).map(task => ({
         title: `Running tasks for ${task.pattern}`,
         task: () =>
-          new Listr(runScript(task.commands, task.fileList, packageJson, config), {
+          new Listr(runScript(task.commands, task.fileList, scripts, config), {
             // In sub-tasks we don't want to run concurrently
             // and we want to abort on errors
             concurrent: false,

src/runScript.js

@@ -8,7 +8,7 @@ const getConfig = require('./getConfig').getConfig
 const calcChunkSize = require('./calcChunkSize')
 const findBin = require('./findBin')
 
-module.exports = function runScript(commands, pathsToLint, packageJson, config) {
+module.exports = function runScript(commands, pathsToLint, scripts, config) {
   const normalizedConfig = getConfig(config)
   const chunkSize = normalizedConfig.chunkSize
   const concurrency = normalizedConfig.subTaskConcurrency
@@ -22,7 +22,7 @@ module.exports = function runScript(commands, pathsToLint, packageJson, config)
     title: linter,
     task: () => {
       try {
-        const res = findBin(linter, packageJson, config)
+        const res = findBin(linter, scripts, config)
 
         const separatorArgs = /npm(\.exe)?$/i.test(res.bin) ? ['--'] : []
 

test/findBin.spec.js

@@ -2,50 +2,47 @@ import findBin from '../src/findBin'
 
 jest.mock('npm-which')
 
-const packageJSON = {
-  scripts: {
-    test: 'noop'
-  },
-  'lint-staged': {}
-}
+const scripts = { test: 'noop' }
 
 describe('findBin', () => {
   it('should favor `npm run` command if exists in both package.json and .bin/', () => {
-    const packageJSONMock = {
-      scripts: {
-        'my-linter': 'my-linter'
-      }
-    }
-    const { bin, args } = findBin('my-linter', packageJSONMock)
+    const { bin, args } = findBin('my-linter', { 'my-linter': 'my-linter' })
     expect(bin).toEqual('npm')
     expect(args).toEqual(['run', '--silent', 'my-linter'])
   })
 
   it('should return npm run command without --silent in verbose mode', () => {
-    const packageJSONMock = {
-      scripts: {
-        eslint: 'eslint'
-      }
-    }
-    const { bin, args } = findBin('eslint', packageJSONMock, { verbose: true })
+    const { bin, args } = findBin('eslint', { eslint: 'eslint' }, { verbose: true })
     expect(bin).toEqual('npm')
     expect(args).toEqual(['run', 'eslint'])
   })
 
+  it('should resolve cmd defined in scripts with args', () => {
+    const { bin, args } = findBin('kcd-scripts format', { 'kcd-scripts': 'node index.js' })
+    expect(bin).toEqual('npm')
+    expect(args).toEqual(['run', '--silent', 'kcd-scripts', 'format'])
+  })
+
+  it('should resolve cmd defined in scripts with space in name', () => {
+    const { bin, args } = findBin('my cmd', { 'my cmd': 'echo deal-with-it' })
+    expect(bin).toEqual('npm')
+    expect(args).toEqual(['run', '--silent', 'my cmd'])
+  })
+
   it('should return path to bin if there is no `script` with name in package.json', () => {
-    const { bin, args } = findBin('my-linter', packageJSON)
+    const { bin, args } = findBin('my-linter', scripts)
     expect(bin).toEqual('my-linter')
     expect(args).toEqual([])
   })
 
   it('should throw an error if bin not found and there is no entry in scripts section', () => {
     expect(() => {
-      findBin('my-missing-linter', packageJSON)
+      findBin('my-missing-linter', scripts)
     }).toThrow('my-missing-linter could not be found. Try `npm install my-missing-linter`.')
   })
 
   it('should parse cmd and add arguments to args', () => {
-    const { bin, args } = findBin('my-linter task --fix', packageJSON)
+    const { bin, args } = findBin('my-linter task --fix', scripts)
     expect(bin).toEqual('my-linter')
     expect(args).toEqual(['task', '--fix'])
   })

test/runAll.spec.js

@@ -8,42 +8,38 @@ sgfMock.mockImplementation((params, callback) => {
   callback(null, [])
 })
 
-const packageJson = {
-  scripts: {
-    mytask: 'echo "Running task"'
-  }
-}
+const scripts = { mytask: 'echo "Running task"' }
 
 describe('runAll', () => {
   afterEach(() => {
     sgfMock.mockClear()
   })
   it('should throw when invalid config is provided', () => {
-    expect(() => runAll(packageJson, {})).toThrowErrorMatchingSnapshot()
-    expect(() => runAll(packageJson)).toThrowErrorMatchingSnapshot()
+    expect(() => runAll(scripts, {})).toThrowErrorMatchingSnapshot()
+    expect(() => runAll(scripts)).toThrowErrorMatchingSnapshot()
   })
 
   it('should not throw when a valid config is provided', () => {
     const config = getConfig({
       concurrent: false
     })
-    expect(() => runAll(packageJson, config)).not.toThrow()
+    expect(() => runAll(scripts, config)).not.toThrow()
   })
 
   it('should return a promise', () => {
-    expect(runAll(packageJson, getConfig({}))).toBeInstanceOf(Promise)
+    expect(runAll(scripts, getConfig({}))).toBeInstanceOf(Promise)
   })
 
   it('should resolve the promise with no tasks', () => {
     expect.assertions(1)
-    return expect(runAll(packageJson, getConfig({}))).resolves.toEqual('No tasks to run.')
+    return expect(runAll(scripts, getConfig({}))).resolves.toEqual('No tasks to run.')
   })
 
   it('should reject the promise when staged-git-files errors', () => {
     sgfMock.mockImplementation((params, callback) => {
       callback('test', undefined)
     })
     expect.assertions(1)
-    return expect(runAll(packageJson, getConfig({}))).rejects.toEqual('test')
+    return expect(runAll(scripts, getConfig({}))).rejects.toEqual('test')
   })
 })

test/runScript.spec.js

@@ -4,12 +4,9 @@ import runScript from '../src/runScript'
 
 jest.mock('execa')
 
-const packageJSON = {
-  scripts: {
-    test: 'noop',
-    test2: 'noop'
-  },
-  'lint-staged': {}
+const scripts = {
+  test: 'noop',
+  test2: 'noop'
 }
 
 describe('runScript', () => {
@@ -18,17 +15,17 @@ describe('runScript', () => {
   })
 
   it('should return an array', () => {
-    expect(runScript('test', ['test.js'], packageJSON)).toBeInstanceOf(Array)
+    expect(runScript('test', ['test.js'], scripts)).toBeInstanceOf(Array)
   })
 
   it('should throw for non-existend script', () => {
     expect(() => {
-      runScript('missing-module', ['test.js'], packageJSON)[0].task()
+      runScript('missing-module', ['test.js'], scripts)[0].task()
     }).toThrow()
   })
 
   it('should work with a single command', async () => {
-    const res = runScript('test', ['test.js'], packageJSON)
+    const res = runScript('test', ['test.js'], scripts)
     expect(res.length).toBe(1)
     expect(res[0].title).toBe('test')
     expect(res[0].task).toBeInstanceOf(Function)
@@ -38,7 +35,7 @@ describe('runScript', () => {
   })
 
   it('should work with multiple commands', async () => {
-    const res = runScript(['test', 'test2'], ['test.js'], packageJSON)
+    const res = runScript(['test', 'test2'], ['test.js'], scripts)
     expect(res.length).toBe(2)
     expect(res[0].title).toBe('test')
     expect(res[1].title).toBe('test2')
@@ -56,7 +53,7 @@ describe('runScript', () => {
   })
 
   it('should respect chunk size', async () => {
-    const res = runScript(['test'], ['test1.js', 'test2.js'], packageJSON, {
+    const res = runScript(['test'], ['test1.js', 'test2.js'], scripts, {
       chunkSize: 1
     })
     const taskPromise = res[0].task()
@@ -68,7 +65,7 @@ describe('runScript', () => {
   })
 
   it('should support non npm scripts', async () => {
-    const res = runScript(['node --arg=true ./myscript.js', 'git add'], ['test.js'], packageJSON)
+    const res = runScript(['node --arg=true ./myscript.js', 'git add'], ['test.js'], scripts)
     expect(res.length).toBe(2)
     expect(res[0].title).toBe('node --arg=true ./myscript.js')
     expect(res[1].title).toBe('git add')
@@ -89,7 +86,7 @@ describe('runScript', () => {
   })
 
   it('should pass cwd to execa if gitDir option is set for non-npm tasks', async () => {
-    const res = runScript(['test', 'git add'], ['test.js'], packageJSON, { gitDir: '../' })
+    const res = runScript(['test', 'git add'], ['test.js'], scripts, { gitDir: '../' })
     let taskPromise = res[0].task()
     expect(taskPromise).toBeInstanceOf(Promise)
     await taskPromise
@@ -106,7 +103,7 @@ describe('runScript', () => {
   })
 
   it('should not pass `gitDir` as `cwd` to `execa()` if a non-git binary is called', async () => {
-    const res = runScript(['jest'], ['test.js'], packageJSON, { gitDir: '../' })
+    const res = runScript(['jest'], ['test.js'], scripts, { gitDir: '../' })
     const taskPromise = res[0].task()
     expect(taskPromise).toBeInstanceOf(Promise)
     await taskPromise
@@ -115,7 +112,7 @@ describe('runScript', () => {
   })
 
   it('should use --silent in non-verbose mode', async () => {
-    const res = runScript('test', ['test.js'], packageJSON, { verbose: false })
+    const res = runScript('test', ['test.js'], scripts, { verbose: false })
     const taskPromise = res[0].task()
     expect(taskPromise).toBeInstanceOf(Promise)
     await taskPromise
@@ -124,7 +121,7 @@ describe('runScript', () => {
   })
 
   it('should not use --silent in verbose mode', async () => {
-    const res = runScript('test', ['test.js'], packageJSON, { verbose: true })
+    const res = runScript('test', ['test.js'], scripts, { verbose: true })
     const taskPromise = res[0].task()
     expect(taskPromise).toBeInstanceOf(Promise)
     await taskPromise
@@ -138,7 +135,7 @@ describe('runScript', () => {
     linterErr.stderr = ''
     mockFn.mockImplementationOnce(() => Promise.reject(linterErr))
 
-    const res = runScript('mock-fail-linter', ['test.js'], packageJSON)
+    const res = runScript('mock-fail-linter', ['test.js'], scripts)
     const taskPromise = res[0].task()
     try {
       await taskPromise