Special chars

[erikkemperman]

Hi,

First of all: thanks for this great module!

I ran into some issues with tasks whose names begin with special characters like '!' or '?'. These work fine when I invoke npm run but fail if I run them with npm-run-all.

Example:

{
  "scripts": {
    "?clean": "echo Clean the project",
    "!clean": "rimraf dist/**/*",
    "clean": "run-s -ls ?clean !clean"
  }
}

$ npm run ?clean  # works
$ npm run clean  # fails

I've tracked this down to two distinct issues:

• minimatch treats a leading exclamation mark '!' as a negation -- I think this is almost never what we want for matching tasks (it would mean "run everything except ..."). On that assumption I've added an option to the minimatch constructor telling it to treat exclamation marks the same as other chars.
• shell-quote's parse() method sometimes returns objects instead of strings, for example when it thinks something is a glob. But npm-cli doesn't understand anything except strings, and throws errors otherwise. Since we've already handled any globbing by the time we get to shell-quote parse(), I've added a simple mapper function which cleans up the parse output to ensure we pass valid input to npm-cli.
• There are tests in this PR for both changes.

I would really appreciate if you might consider merging these changes and include them in a new release. Of course, if you have suggestions for improvements please let me know and I'll amend the PR.

PS -- I've had to restart the Travis builds a couple of times, due to an error which is unrelated to my changes... I wasn't able to figure it out quickly but you might want to have a look at why the following test is (sometimes) failing:
https://github.com/mysticatea/npm-run-all/blob/master/test/parallel.js#L240

[erikkemperman]

The unrelated error I mentioned above just happened again on the Travis build:

https://travis-ci.org/mysticatea/npm-run-all/jobs/334638286#L1010

I'll try to close and reopen this PR to force a rebuild on Travis. Sorry about the noise in your inbox...

[codecov-io]

Codecov Report

Merging #124 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   92.71%   92.73%   +0.01%     
==========================================
  Files          22       22              
  Lines         467      468       +1     
==========================================
+ Hits          433      434       +1     
  Misses         34       34

Impacted Files	Coverage Δ
lib/run-task.js	86.76% <100%> (+0.19%)	⬆️
lib/match-tasks.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 c76db0e...4972be9. Read the comment docs.

[erikkemperman]

Ugh, now Travis passes but Appveyor fails. I'm going to leave this as it is, pretty confident that these errors are unrelated to my changes... Here is the commit history on my branch, please notice the green check marks :-)

https://github.com/erikkemperman/npm-run-all/commits/special-chars

[erikkemperman]

Cleaned up last two commits a bit, still CI fails here but works on my branch:
https://github.com/erikkemperman/npm-run-all/commits/special-chars

[mysticatea]

Thank you for the PR!

I re-run the CI builds and confirmed success. npm-run-all tests have a problem that those are whimsy.
I will check this PR within the weekend (sorry I'm pretty busy now).

[erikkemperman]

@mysticatea
That’d be great, thanks! There’s no rush, I’ll just npm link my fork for the time being.

Very strange about those tests, I couldn’t reproduce the problem locally so I’m not sure what is going wrong. It was the same test that failed every time though...

[mysticatea]

LGTM, great!

[mysticatea]

About tests, it's timing problem between parent and child processes. Especially, Node.js doesn't seem to wait for the actually dead of child processes after kill (Or the change of a file appeared for other processes after child process is dead).

[erikkemperman]

Thanks for review and merge!

[erikkemperman]

@mysticatea Would you consider making a release with this change in? I'm just about ready to do an initial release of the project where I am using this.

[erikkemperman]

@mysticatea Sorry to bother you about this again, but would you please consider making a new release with this fix in it? Thanks!

[mysticatea]

Uga, I apologize that I have lost this in my busy. I will do.

[erikkemperman]

@mysticatea Looking forward to it. Much appreciated, thanks!