package.json: use npm-run-all in scripts
#375
Conversation
williamboman
commented
Jun 3, 2016
williamboman
commented
- 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.
For the sake of uniformity, npm-run-all should be used for build as well.
williamboman
force-pushed
the
chore/npm-scripts
branch
from
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. |
astorije
added
Type: Feature
| @@ -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.
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.
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.
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.
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 |
williamboman
force-pushed
the
chore/npm-scripts
branch
2 times, most recently
from
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.
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 |
williamboman
force-pushed
the
chore/npm-scripts
branch
from
ee55313 to
1d088de
Compare
|
All good, until the next round of improvements :-) |
package.json: use `npm-run-all` in scripts
