Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible bug in script resolution #294

Closed
sudo-suhas opened this issue Sep 21, 2017 · 1 comment
Closed

Possible bug in script resolution #294

sudo-suhas opened this issue Sep 21, 2017 · 1 comment
Assignees

Comments

@sudo-suhas
Copy link
Collaborator

I am not 100% this is a bug but here's an extremely convoluted demo:

{
  "name": "test-lint-staged-script",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "demo-bin": "node index.js",
    "lint-staged": "lint-staged"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "dependencies": {
    "lint-staged": "^4.2.1"
  },
  "lint-staged": {
    "*.js": "demo-bin hello"
  }
}

I have defined the script entry and am trying to pass additional parameters to it. This is supported if demo-bin was installed through npm. If I try to run lint-staged, this happens:

λ yarn run lint-staged
yarn run v1.0.2
warning ..\package.json: No license field
$ lint-staged
 > Running tasks for *.js
   × demo-bin hello
     → demo-bin could not be found. Try `npm install demo-bin`.
demo-bin could not be found. Try `npm install demo-bin`.
error Command failed with exit code 1.

We split the command into bin and args while using npm-which:

https://github.com/okonet/lint-staged/blob/fd79a318985cdc7d94fa8e2969b0a98817c2b4e1/src/findBin.js#L35-L37

Should we be doing this for packageJson.scripts[cmd] as well?

@okonet
Copy link
Collaborator

okonet commented Sep 21, 2017

Yeah, I think so. We should add tests for this as well. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants