finish startup on error #56

[paulpflug]

No description provided.

[mhchen]

Hi @paulpflug, thanks for getting on this so quickly. One issue with this fix that I noticed when I run in debug mode is that that I get the output "debugger listening on port 5858" from node/express. I have no idea why, but this message displays to stderr. This causes Grunt to regain control too early, as you can see from the output of my Grunt task execution with your code change:

Running "express:dev" (express) task
Starting background Express server
debugger listening on port 5858

Running "open:server" (open) task

Running "watch" task
Waiting...Express server listening on localhost:9000, in development mode

As you can see, my open and watch tasks ran before my Express server was actually ready.
I'm not 100% sure about the solution to this but I was thinking of incorporating something like options.errOutput similar to output that would monitor for a specific type of error (in this case a Coffeescript compile error) rather than returning on all stderr output. What do you think?

[ericclemmons]

@mhchen You must be running node debug for this to appear:

http://nodejs.org/api/debugger.html

What you may want to consider is modifying the output option to match something you expect, like /started/ or /Started|Exception/:

https://github.com/ericclemmons/grunt-express-server#default-options

[ericclemmons]

As for @paulpflug's PR, it passes on my machine & travis. @mhchen Can you post your Gruntfile config (the express part) so we can write a test for your situation? Thanks!

[mhchen]

I am indeed running my dev version of express in Coffeescript. Here's the relevant portion of my gruntfile (also in Coffeescript):

     express:
        options:
          port: process.env.PORT || 9000
        dev:
          options:
            opts: ['node_modules/coffee-script/bin/coffee']
            script: 'server.coffee',
            debug: true

Maybe it might make sense for @paulpflug's PR to instead listen for any instances of the word "error" (case-insensitive) on stderr? I think that should cast a pretty wide net over at least 99% of what makes this change useful.

[ericclemmons]

That's what the output can do for you in your Gruntfile. Try setting that to a regular expression matching only what your app would output.

[mhchen]

New commit looks good to me @ericclemmons. Thanks @paulpflug (note that there is a missing semicolon on line 103 though)

[mhchen]

Well, in #56 I indicated that with a Coffeescript compile error you get no output from stdout, that's why we need to monitor stderr too. Does that make sense?

[ericclemmons]

There's no output from the compile error? That's unpossible!

[mhchen]

Yup, it all goes to stderr unfortunately (probably not unfortunately, but unfortunately for this case)

[paulpflug]

Yep it does indeed.
nothing written to stdout.
I would prefer just not listen to the "debugger listening .." statement and give control back on every other error..

[ericclemmons]

So you're saying maybe the default output option is anything that's not "debugger listening"?

Has someone tried modifying that option with success, or are y'all waiting on me?

[paulpflug]

no. it has nothing to do with output ;)

[ericclemmons]

Ok.

[ericclemmons]

@paulpflug I merged this down trying to resolve #66, but the tests are immediately failing on master, despite passing on travis previously.

BTW, I've made you a collaborator since you've been so helpful :)

[ericclemmons]

@paulpflug I'm an idiot. My other node project was still running )

[gsouza75]

Hey guys, just letting you know that this change introduces some unexpected behavior, at least in my case.

Scenario: I've been living with a couple of body-parser related deprecation warnings since upgrading to Express 4. Basically, the new body-parser middleware writes the following to stderr right before I start my Express server:

Thu, 14 Aug 2014 22:33:29 GMT body-parser deprecated bodyParser: use individual json/urlencoded middlewares at server/server.coffee:314:24
Thu, 14 Aug 2014 22:33:29 GMT body-parser deprecated undefined extended: provide extended option at node_modules/body-parser/index.js:75:29

This triggers the server.stderr.on 'data' event and finished() gets called so the grunt process ends and my server never really starts.

Since I had been using "grunt-express-server": "^0.4.17" in my package.json, 0.4.18 got picked up when I deleted node_modules and npm install'ed.

Not a big deal to fix on my end, but just a heads up that this could throw off folks that are using '^' or '~' and expecting semantic versioning.

[ericclemmons]

That's good to know. Since most of my effort on this has been merging others' work ( much thanks! ), and the tests didn't reflect any changes, I assumed a patch release was sufficient.

I'll create a test, revert, and minor bump.

[paulpflug]

ok. the problem is we need to call finish() on compile error. Maybe there is a better way than listening to stderr, as there is obviously also other output there.

[paulpflug]

I created a dev branch. I fixed a bug and converted from grunt.util.spawn to the native node spawn.
The new version catches and prints syntax errors and closes the server, but doesn't halt on stderr.

I'm not very familiar to nodeunit and I see no way in testing syntax errors with it.

[ericclemmons]

Should I continue reverting this break, patch, bump to v0.5.x, then continue working with your branch there?

[paulpflug]

I think dev branch is stable. There are also no breaking changes, so a minor bump would suffice.
Unit tests are working.. but I added none, as I don't know how to test syntax errors with nodeunit?
But my manual tests are doing great ;)

Maybe we wait for #48 and then merge and bump ?