-
Notifications
You must be signed in to change notification settings - Fork 681
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
package.json: use npm-run-all
in scripts
#375
Conversation
williamboman
commented
Jun 3, 2016
- Proper exit codes
- Runs all the specified npm scripts sequentially (continues on error)
As I pointed out in #361 (comment) we should switch to a proper build script |
That is one massively overengineered library, ouch. But it does appear to solve our initial problem of preserving the exit code while running all tests, and it supports parallel execution as well. |
@@ -16,9 +16,9 @@ | |||
"build": "npm run build:grunt && npm run build:handlebars", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of uniformity, npm-run-all
should be used for build
as well.
c4b33c1
to
b0a5424
Compare
Why?
Could do |
Yeah, I wanted to look if it was one of these one-liner packages you can find out there, I got the same feeling than @maxpoulin64! I guess that's where the "java" in "javascript" lies :-) Still, I'm 👍, it does the job, and we can easily switch to something else as there is no other addition on our side. |
@@ -13,12 +13,12 @@ | |||
"homepage": "https://thelounge.github.io/", | |||
"scripts": { | |||
"start": "node index", | |||
"build": "npm run build:grunt && npm run build:handlebars", | |||
"build": "npm-run-all build:grunt build:handlebars", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, no need for npm-run-all
here. The only benefit I see in this file is to have correct exit code and run everything when you test. For building, &&
is fine as we need everything. Unless I missed something :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree that using &&
is fine here. However using npm-run-all
results in a shorter line (but -c
is needed to fail builds).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm-run-all
without -c
will abort as soon as a script fails (with a non-0 exit code). -c
will run all scripts no matter what, but in the end exit with a non-0 code if one or more scripts failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking in regards to CIs, we would want to print all the errors at once (as we do with tests).
OK, I'll be fine with this. I think appveyor and travis configs need to be updated to just run the Travis: remove |
7300da2
to
ee55313
Compare
👍 Looks good. Will wait for @maxpoulin64 or @astorije to review the latest changes before merging. |
@@ -13,13 +13,13 @@ | |||
"homepage": "https://thelounge.github.io/", | |||
"scripts": { | |||
"start": "node index", | |||
"build": "npm run build:font-awesome && npm run build:grunt && npm run build:handlebars", | |||
"build": "npm-run-all build:font-awesome build:grunt build:handlebars", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to be that guy, but I'd really be more comfortable keeping &&
here. Reason is that the build
script (and associated prepublish
) is the most critical script of all as it is run by Travis CI when we publish, and releases cannot be modified or removed once published.
I'm OK with keeping test
and lint
short and clean because it's just a matter of convenience in dev, but we've all been surprised by the over-engineering of the npm-run-all
package, and if building works perfectly fine with &&
, we should stick to that. Adding a dependency for this step is another point of failure we loosely control.
Does this make sense?
I would have been fine keeping the individual statements for CIs in order to limit the tie-ins with this package, but it's no big deal, not like it changes a lot of things nor like it's completely immutable if things go wrong. I however added a comment about |
ee55313
to
1d088de
Compare
All good, until the next round of improvements :-) |
package.json: use `npm-run-all` in scripts