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

feat: Remove support for npm scripts #390

Merged
merged 2 commits into from
Feb 5, 2018
Merged

Conversation

sudo-suhas
Copy link
Collaborator

This removes support for running scripts defined in package.json using lint-staged. The same can be done by specifying the command as npm run script-name --. This also adds a slight perf improvement for findbin by introducing simple caching to avoid repeatedly resolving binaries(ex: git).

Closes #376.

- Update `findBin` to only resolve binaries installed locally or globally. Also
  add simple caching.
- 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"]
@sudo-suhas sudo-suhas mentioned this pull request Feb 1, 2018
@codecov
Copy link

codecov bot commented Feb 1, 2018

Codecov Report

Merging #390 into next will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             next     #390     +/-   ##
=========================================
+ Coverage   98.11%   98.22%   +0.1%     
=========================================
  Files          10       11      +1     
  Lines         212      225     +13     
  Branches       23       24      +1     
=========================================
+ Hits          208      221     +13     
  Misses          4        4
Impacted Files Coverage Δ
src/checkPkgScripts.js 100% <100%> (ø)
src/findBin.js 100% <100%> (ø) ⬆️
src/index.js 89.47% <100%> (-1.01%) ⬇️
src/runScript.js 100% <100%> (ø) ⬆️
src/runAll.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af596b5...60f63a4. Read the comment docs.

Copy link
Collaborator

@okonet okonet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Much cleaner this way. LGTM besides the question I asked.

src/findBin.js Outdated
} catch (err) {
throw new Error(`${bin} could not be found. Try \`npm install ${bin}\`.`)
throw new Error(`${binName} could not be found. Try \`npm install ${binName}\`.`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we try smoothen the migration for users who rely on this feature by checking if it exists in package.json and throwing a different error you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, it slipped my mind.

@sudo-suhas
Copy link
Collaborator Author

Hey @okonet, I have done the changes for the helpful message. Could you take a look?

@sudo-suhas sudo-suhas merged commit fd451dd into next Feb 5, 2018
@sudo-suhas sudo-suhas deleted the drop-npm-script-support branch February 5, 2018 08:14
sudo-suhas added a commit that referenced this pull request Feb 17, 2018
- 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"]
sudo-suhas added a commit that referenced this pull request Feb 17, 2018
- 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"]
sudo-suhas added a commit that referenced this pull request Feb 21, 2018
- 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"]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants