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

Allow built-in install task #110

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hugojosefson
Copy link

Wow! What a nice, well-structured repo and tool you have here!

I want to be able to have a run-script "install-sub-dependencies": "cd sub/ && run-s install", which does npm install or yarn install to install the dependencies in the sub/ project, depending on how the main script was called.

This PR enables the install task, which is run without "run" before it.

@codecov-io
Copy link

codecov-io commented Oct 12, 2017

Codecov Report

Merging #110 into master will decrease coverage by 0.93%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
- Coverage   92.88%   91.95%   -0.94%     
==========================================
  Files          22       22              
  Lines         464      460       -4     
==========================================
- Hits          431      423       -8     
- Misses         33       37       +4
Impacted Files Coverage Δ
lib/match-tasks.js 100% <100%> (ø) ⬆️
lib/run-task.js 83.87% <80%> (-3.63%) ⬇️
bin/common/parse-cli-args.js 90.8% <0%> (-2.46%) ⬇️
lib/index.js 93.58% <0%> (-0.17%) ⬇️
lib/run-tasks.js 85.07% <0%> (ø) ⬆️
bin/npm-run-all/help.js 100% <0%> (ø) ⬆️
bin/npm-run-all/main.js 100% <0%> (ø) ⬆️

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 43a6b16...f6d5b74. Read the comment docs.

@mysticatea
Copy link
Owner

mysticatea commented Oct 12, 2017

Thank you for this PR.

However, because of npm has a ton of commands, I don't think that npm-run-all should handle only npm install as special.

I think that this is out of scope because npm-run-all is a tool to do npm run command with multiple tasks, but I can accept if there is a proper way to specify commands except run. The way should be that users can distinguish between npm run and other commands easily.

@hugojosefson
Copy link
Author

Thank you @mysticatea so much for your feedback!

That's a good idea to make sure the user knows what they are doing.

I was also thinking that npm-run-all is very good at choosing yarn or npm, depending on how it was initially run. So it could be quite useful to be able to run commands too, if the commands are the same in npm and yarn.

Previously, I thought install was the only command that was the same and did the same thing in both npm and yarn, but now I see that for example publish is also the same command. I agree that it should be possible to distinguish between commands and run-scripts.

How about using for example command: as a prefix for commands, such as for example:

{
  "scripts": {
    "install-sub-dependencies": "cd sub && run-s clean command:install"
  }
}

Or maybe even better by for example adding a specific argument to specify a command:

{
  "scripts": {
    "install-sub-dependencies": "cd sub && run-s clean --command install"
  }
}

Would that be a good way forward?

Thanks for the consideration!
/Hugo

@mysticatea
Copy link
Owner

I apology that I'm late to respond.

About the command:install looks, I'm afraid because it runs npm run command:install currently. npm-run-all recommends to use : as separators (Glob-like pattern matching for script names).

The --command install looks nice to me, but it's long a bit.
Hmm, how about :install? It's still a breaking change, but it's shorter and distinguishable. I can image absolute path to specify outside of run command from the :install syntax.

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

Successfully merging this pull request may close these issues.

None yet

3 participants