feat: Remove support for npm scripts (#390)

- Update `findBin` to only resolve binaries installed locally or globally. Also
  add simple caching.
- Check pkg scripts when `findBin` fails and print useful message to aid users
  in migration to newer version config.
- Update readme.
- Remove dependency `app-root-path`.
- Update tests.

BREAKING CHANGE: Remove implicit support for running npm scripts.

Consider example `lint-staged` config:

    {
      "name": "My project",
      "version": "0.1.0",
      "scripts": {
        "my-custom-script": "linter --arg1 --arg2",
        "precommit": "lint-staged"
      },
      "lint-staged": {
        "*.js": ["my-custom-script", "git add"]
      }
    }

The list of commands should be changed to the following:

    "*.js": ["npm run my-custom-script --", "git add"]

Author: sudo-suhas

README.md

@@ -6,7 +6,7 @@ Run linters against staged git files and don't let :poop: slip into your code ba
 
 Linting makes more sense when running before committing your code. By doing that you can ensure no errors are going into repository and enforce code style. But running a lint process on a whole project is slow and linting results can be irrelevant. Ultimately you only want to lint files that will be committed.
 
-This project contains a script that will run arbitrary npm and shell tasks with a list of staged files as an argument, filtered by a specified glob pattern.
+This project contains a script that will run arbitrary shell tasks with a list of staged files as an argument, filtered by a specified glob pattern.
 
 ## Related blogs posts
 
@@ -96,11 +96,8 @@ Should be an object where each value is a command to run and its key is a glob p
 #### `package.json` example:
 ```json
 {
-  "scripts": {
-    "my-task": "your-command",
-  },
   "lint-staged": {
-    "*": "my-task"
+    "*": "your-cmd"
   }
 }
 ```
@@ -109,15 +106,15 @@ Should be an object where each value is a command to run and its key is a glob p
 
 ```json
 {
-  "*": "my-task"
+  "*": "your-cmd"
 }
 ```
 
-This config will execute `npm run my-task` with the list of currently staged files passed as arguments.
+This config will execute `your-cmd` with the list of currently staged files passed as arguments.
 
 So, considering you did `git add file1.ext file2.ext`, lint-staged will run the following command:
 
-`npm run my-task -- file1.ext file2.ext`
+`your-cmd file1.ext file2.ext`
 
 ### Advanced config format
 To set options and keep lint-staged extensible, advanced format can be used. This should hold linters object in `linters` property.
@@ -164,11 +161,11 @@ Also see [How to use `lint-staged` in a multi package monorepo?](#how-to-use-lin
 
 ## What commands are supported?
 
-Supported are both local npm scripts (`npm run-script`), or any executables installed locally or globally via `npm` as well as any executable from your $PATH.
+Supported are any executables installed locally or globally via `npm` as well as any executable from your $PATH.
 
 > Using globally installed scripts is discouraged, since lint-staged may not work for someone who doesn’t have it installed.
 
-`lint-staged` is using [npm-which](https://github.com/timoxley/npm-which) to locate locally installed scripts, so you don't need to add `{ "eslint": "eslint" }` to the `scripts` section of your `package.json`. So  in your `.lintstagedrc` you can write:
+`lint-staged` is using [npm-which](https://github.com/timoxley/npm-which) to locate locally installed scripts. So in your `.lintstagedrc` you can write:
 
 ```json
 {
@@ -210,13 +207,14 @@ All examples assuming you’ve already set up lint-staged and husky in the `pack
   "name": "My project",
   "version": "0.1.0",
   "scripts": {
+    "my-custom-script": "linter --arg1 --arg2",
     "precommit": "lint-staged"
   },
   "lint-staged": {}
 }
 ```
 
-*Note we don’t pass a path as an argument for the runners. This is important since lint-staged will do this for you. Please don’t reuse your tasks with paths from package.json.*
+*Note we don’t pass a path as an argument for the runners. This is important since lint-staged will do this for you.*
 
 ### ESLint with default parameters for `*.js` and `*.jsx` running as a pre-commit hook
 
@@ -236,6 +234,23 @@ All examples assuming you’ve already set up lint-staged and husky in the `pack
 
 This will run `eslint --fix` and automatically add changes to the commit. Please note, that it doesn’t work well with committing hunks (`git add -p`).
 
+### Reuse npm script
+
+If you wish to reuse a npm script defined in your package.json:
+
+```json
+{
+  "*.js": ["npm run my-custom-script --", "git add"]
+}
+```
+
+The following is equivalent:
+
+```json
+{
+  "*.js": ["linter --arg1 --arg2", "git add"]
+}
+```
 
 ### Automatically fix code style with `prettier` for javascript + flow or typescript
 

package.json

@@ -27,7 +27,7 @@
     "test:watch": "jest --watch"
   },
   "dependencies": {
-    "app-root-path": "^2.0.0",
+    "app-root-path": "^2.0.1",
     "chalk": "^2.1.0",
     "commander": "^2.11.0",
     "cosmiconfig": "^4.0.0",

src/checkPkgScripts.js

@@ -0,0 +1,45 @@
+'use strict'
+
+const chalk = require('chalk')
+const dedent = require('dedent')
+const has = require('lodash/has')
+
+const warn = msg => {
+  console.warn(chalk.yellowBright.bold(msg))
+}
+
+/**
+ * Checks if the given command or binary name is present in the package.json scripts. This would be
+ * called if and when resolving a binary fails in `findBin`.
+ *
+ * @param {Object} pkg package.json
+ * @param {string} cmd
+ * @param {string} binName
+ * @param {Array<string>} args
+ * @throws {Error} If a script is found in the pkg for the given `cmd` or `binName`.
+ */
+module.exports = function checkPkgScripts(pkg, cmd, binName, args) {
+  if (pkg && pkg.scripts) {
+    let scriptName
+    let script
+    if (has(pkg.scripts, cmd)) {
+      scriptName = cmd
+      script = pkg.scripts[cmd]
+    } else if (has(pkg.scripts, binName)) {
+      scriptName = binName
+      script = pkg.scripts[binName]
+    } else {
+      return
+    }
+
+    const argsStr = args && args.length ? args.join(' ') : ''
+    warn(dedent`
+      \`lint-staged\` no longer supports running scripts defined in package.json.
+
+      The same behavior can be achieved by changing the command to any of the following:
+        - \`npm run ${scriptName} -- ${argsStr}\`
+        - \`${script} ${argsStr}\`
+    `)
+    throw new Error(`Could not resolve binary for \`${cmd}\``)
+  }
+}

src/findBin.js

@@ -1,86 +1,47 @@
 'use strict'
 
+const appRoot = require('app-root-path')
 const npmWhich = require('npm-which')(process.cwd())
+const checkPkgScripts = require('./checkPkgScripts')
 
+// Find and load the package.json at the root of the project.
+const pkg = require(appRoot.resolve('package.json')) // eslint-disable-line import/no-dynamic-require
 const debug = require('debug')('lint-staged:find-bin')
 
-module.exports = function findBin(cmd, scripts, debugMode) {
+const cache = new Map()
+
+module.exports = function findBin(cmd) {
   debug('Resolving binary for command `%s`', cmd)
-  const npmArgs = (bin, args) =>
-    // We always add `--` even if args are not defined. This is required
-    // because we pass filenames later.
-    ['run', debugMode ? 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. 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
+   *  Try to locate the binary in node_modules/.bin and if this fails, in
+   *  $PATH.
    *
-   * Example:
+   *  This allows to use linters installed for the project:
    *
-   *   "scripts": {
-   *     "my cmd": "echo deal-wth-it",
-   *     "demo-bin": "node index.js"
-   *   },
-   *   "lint-staged": {
-   *     "*.js": ["my cmd", "demo-bin hello"]
-   *   }
+   *  "lint-staged": {
+   *    "*.js": "eslint"
+   *  }
    */
-  if (scripts[cmd] !== undefined) {
-    // Support for scripts from package.json
-    debug('`%s` resolved to npm script - `%s`', cmd, scripts[cmd])
-    return { bin: 'npm', args: npmArgs(cmd) }
-  }
-
   const parts = cmd.split(' ')
-  let bin = parts[0]
+  const binName = parts[0]
   const args = parts.splice(1)
 
-  if (scripts[bin] !== undefined) {
-    debug('`%s` resolved to npm script - `%s`', bin, scripts[bin])
-    return { bin: 'npm', args: npmArgs(bin, args) }
+  if (cache.has(binName)) {
+    debug('Resolving binary for `%s` from cache', binName)
+    return { bin: cache.get(binName), args }
   }
 
-  /*
-    *  If cmd wasn't found in package.json scripts
-    *  we'll try to locate the binary in node_modules/.bin
-    *  and if this fails in $PATH.
-    *
-    *  This is useful for shorter configs like:
-    *
-    *  "lint-staged": {
-    *    "*.js": "eslint"
-    *  }
-    *
-    *  without adding
-    *
-    *  "scripts" {
-    *    "eslint": "eslint"
-    *  }
-    */
-
   try {
     /* npm-which tries to resolve the bin in local node_modules/.bin */
     /* and if this fails it look in $PATH */
-    bin = npmWhich.sync(bin)
+    const bin = npmWhich.sync(binName)
+    debug('Binary for `%s` resolved to `%s`', cmd, bin)
+    cache.set(binName, bin)
+    return { bin, args }
   } catch (err) {
-    throw new Error(`${bin} could not be found. Try \`npm install ${bin}\`.`)
+    // throw helpful error if matching script is present in package.json
+    checkPkgScripts(pkg, cmd, binName, args)
+    throw new Error(`${binName} could not be found. Try \`npm install ${binName}\`.`)
   }
-
-  debug('Binary for `%s` resolved to `%s`', cmd, bin)
-  return { bin, args }
 }

src/index.js

@@ -2,7 +2,6 @@
 
 'use strict'
 
-const appRoot = require('app-root-path')
 const dedent = require('dedent')
 const cosmiconfig = require('cosmiconfig')
 const stringifyObject = require('stringify-object')
@@ -13,9 +12,6 @@ const runAll = require('./runAll')
 
 const debug = require('debug')('lint-staged')
 
-// Find the right package.json at the root of the project
-const packageJson = require(appRoot.resolve('package.json'))
-
 // Force colors for packages that depend on https://www.npmjs.com/package/supports-color
 // but do this only in TTY mode
 if (process.stdout.isTTY) {
@@ -56,10 +52,7 @@ module.exports = function lintStaged(injectedLogger, configPath, debugMode) {
         debug('Normalized config:\n%O', config)
       }
 
-      const scripts = packageJson.scripts || {}
-      debug('Loaded scripts from package.json:\n%O', scripts)
-
-      runAll(scripts, config, debugMode)
+      runAll(config)
         .then(() => {
           debug('linters were executed successfully!')
           // No errors, exiting with 0

src/runAll.js

@@ -12,11 +12,10 @@ const debug = require('debug')('lint-staged:run')
 
 /**
  * Executes all tasks and either resolves or rejects the promise
- * @param scripts
  * @param config {Object}
  * @returns {Promise}
  */
-module.exports = function runAll(scripts, config, debugMode) {
+module.exports = function runAll(config) {
   debug('Running all linter scripts')
   // Config validation
   if (!config || !has(config, 'concurrent') || !has(config, 'renderer')) {
@@ -37,7 +36,7 @@ module.exports = function runAll(scripts, config, debugMode) {
     const tasks = generateTasks(config, filenames).map(task => ({
       title: `Running tasks for ${task.pattern}`,
       task: () =>
-        new Listr(runScript(task.commands, task.fileList, scripts, config, debugMode), {
+        new Listr(runScript(task.commands, task.fileList, config), {
           // In sub-tasks we don't want to run concurrently
           // and we want to abort on errors
           dateFormat: false,

src/runScript.js

@@ -12,7 +12,7 @@ const resolveGitDir = require('./resolveGitDir')
 
 const debug = require('debug')('lint-staged:run-script')
 
-module.exports = function runScript(commands, pathsToLint, scripts, config, debugMode) {
+module.exports = function runScript(commands, pathsToLint, config) {
   debug('Running script with commands %o', commands)
 
   const normalizedConfig = getConfig(config)
@@ -28,7 +28,7 @@ module.exports = function runScript(commands, pathsToLint, scripts, config, debu
     title: linter,
     task: () => {
       try {
-        const res = findBin(linter, scripts, debugMode)
+        const res = findBin(linter)
 
         // Only use gitDir as CWD if we are using the git binary
         // e.g `npm` should run tasks in the actual CWD

test/__mocks__/npm-which.js

@@ -1,10 +1,14 @@
+const mockFn = jest.fn(path => {
+  if (path.includes('missing')) {
+    throw new Error(`not found: ${path}`)
+  }
+  return path
+})
+
 module.exports = function npmWhich() {
   return {
-    sync(path) {
-      if (path.includes('missing')) {
-        throw new Error(`not found: ${path}`)
-      }
-      return path
-    }
+    sync: mockFn
   }
 }
+
+module.exports.mockFn = mockFn

test/__snapshots__/checkPkgScripts.spec.js.snap

@@ -0,0 +1,19 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`checkPkgScripts throws if the binary name is defined in script 1`] = `
+"
+WARN \`lint-staged\` no longer supports running scripts defined in package.json.
+
+The same behavior can be achieved by changing the command to any of the following:
+  - \`npm run lint -- --fix\`
+  - \`eslint . --fix\`"
+`;
+
+exports[`checkPkgScripts throws if the cmd is defined in script 1`] = `
+"
+WARN \`lint-staged\` no longer supports running scripts defined in package.json.
+
+The same behavior can be achieved by changing the command to any of the following:
+  - \`npm run lint -- \`
+  - \`eslint . \`"
+`;

test/__snapshots__/findBin.spec.js.snap

@@ -0,0 +1,10 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`findBin should throw a helpful error if the cmd is present in pkg scripts 1`] = `
+"
+WARN \`lint-staged\` no longer supports running scripts defined in package.json.
+
+The same behavior can be achieved by changing the command to any of the following:
+  - \`npm run lint -- \`
+  - \`eslint . \`"
+`;