Print npm stderr if npm install fails

[eliangcs]

Fixes #82 and zapier/zapier#16579.

Our current code in zapier build assumes if npm install returns 0, everything is good. But npm install may fail without returning a non-zero exit code. For example, try npm install when the name in package.json has a space character. In this case, npm prints the error to stderr, but exits with 0 as if it was successful.

So what we should do instead, is to check further if APP_DIR/node_modules/zapier-platform-core exists to make sure npm install is really successful before moving on to the next step.

With this PR, now if you zapier build an app with a space in thier name in package.json, we'll show the stderr from npm:

$ zapier build
Building project.

  Copying project to temp directory ⠃ done!
  Installing project dependencies ⠆

Error! Could not install dependencies properly. Error log:
npm WARN Invalid name: "my app"
npm WARN zapier-d1da7977 No description
npm WARN zapier-d1da7977 No repository field.
npm WARN zapier-d1da7977 No README data
npm WARN zapier-d1da7977 No license field.


(Use --debug flag and run this command again to get more details.)

[FokkeZB]

Awesome approach! Perhaps add a test?

[FokkeZB]

I assume you've checked we're not expecting this to resolve with only stdout anywhere else?

[eliangcs]

I was supposed to. Thanks for the reminder! Fixed in a218ba4.

[FokkeZB]

Why the change from console.log to process.stdout?

[eliangcs]

console.log appends a newline on every call, but the stdout from the external command already has newlines. We don't want to print a newline every time we get a piece of text from stdout.

[eliangcs]

@FokkeZB thanks for reviewing. I wouldn't worry about adding a test for now though. To test it, we need to test the build() function in the utils/build module, which requires quite an amount of effort as there's no test like that at the moment. Doesn't seem worthwhile for a UX/DX enhancement.

[xavdid]

I'm pretty sure the fs.exists needs to change. I like the stderr/stdout change.

To take it a step further (out of scope for this PR), we could move this to shelljs, which will handle windows compatibility and simplify this code greatly (I'm all for using other people's tested code over rolling our own).

[xavdid]

fs.exists is both asynchronous and deprecated (docs). I believe this needs to be fs.existsSync or one of the other methods mentioned in that doc link

[eliangcs]

Nice catch! Fixed in 5fb57c1.