Skip to content

Conversation

@rehno-lindeque
Copy link

Hi, this is a very small patch, just lets you specify the command to use as 'coffee' instead of 'node' when running the (express) server script...
Please let me know if it looks ok?

Adding cmd: 'coffee' to your options allows you to run a script using coffeescript...
When running a foreground task, wait for SIGINT to be signaled before stopping the server
@rehno-lindeque
Copy link
Author

I added another commit because I need the ability to stop a foreground task explicitly using Ctrl+C (SIGINT). The trouble is I think that my express server is running asynchronously in its own server.coffee...

I'm not sure if that this is the best method for doing this because you might want to just run some file to completion, but on the other hand probably 90% of express servers will run themselves asynchronously with node's whole callback model, so it seems to make sense do this...?

Btw. The reason I'm doing this is because I'm running the express server in the foreground on production to avoid running the watch task needlessly, which usually keeps the process active. I've noticed in the https://github.com/gruntjs/grunt-contrib-watch docs that they use this method...

By default the watch task will duck punch grunt.fatal and grunt.warn to try and prevent them from exiting the watch process. If you don't want grunt.fatal and grunt.warn to be overridden set the forever option to false.

I think the reason they don't just use grunt.task.current.async() is because they don't want to block the remaining grunt tasks while running watch, however, in this case I think the expected behavior for a foreground option is probably that it should block remaining tasks until the express server has been killed?

Thanks! I appreciate any advice...

@rehno-lindeque
Copy link
Author

Oh sorry by the way, I probably should have put the second commit in a separate branch, I'll split it up if you'd like...

@rehno-lindeque
Copy link
Author

Ah actually I'm not sure capturing SIGINT is going to work well since it just prevents the server from being interrupted... still investigating...

Run foreground express server similar to background express server, but wait for the spawn function to return when running in the foreground
@rehno-lindeque
Copy link
Author

Ok, I've changed the foreground method to work in a similar way to the background method, except that it blocks until the doneFunction callback passed to grunt.util.spawn is fired... also passes along the the SIGINT signal instead of SIGTERM when it is triggered on the parent process.
Hope that makes sense!

@rehno-lindeque
Copy link
Author

Sorry for the noise :) One more thing I wanted to mention: I also had a look at https://github.com/bustardcelly/grunt-forever but I couldn't quite make out if it would do what I wanted it to do, anyway I think if grunt-express-server has a foreground option available it should probably work like this

Simplify the termination logic and distinguish between the grunt task ending and the child process ending
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Common convention is to call this self, at least in grunt & yeoman.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I was wondering what the norm is

@ericclemmons
Copy link
Owner

Let me know how you need me help on this!

@rehno-lindeque
Copy link
Author

Thanks! Sorry, I'm a bit slow to read notifications on github. I'll update soon

@rehno-lindeque
Copy link
Author

Oh sorry for taking so long to update, it's a bit busy right now... I changed _this into self and prevented the process env from being mutated.

I see the travis-ci fails though on the custom_args test though... could you tell if it's something I broke?

@ericclemmons
Copy link
Owner

I've been busy as well :)

I'll check to see what it takes to get all tests running again. Could you provide me (since I don't see it in the PR) what you want to be ran instead? I'm assuming instead of node path/to/server.js you want to have coffee path/to/server.coffee, correct?

I can tell this change requires spawning a process no matter if background is enabled or not (since you can't require a coffeescript file), so I'll re-evaluate the purpose of that option (which I believe was to run a server and wait, but I can't imagine why anyone would use that by itself...)

@ericclemmons
Copy link
Owner

I think I resolved synchronous in master, but it still doesn't explicitly support coffeescript how you have it defined in this branch.

If you could clarify the final command you expect to be spawned per my previous comment, we can continue this PR (and I may merge your commits with master, since you've done enough work already!)

@rehno-lindeque
Copy link
Author

Oh thanks, sorry for not responding!!

@rehno-lindeque
Copy link
Author

Sorry Eric I have trouble even remembering the details of this - we are actually actively using it in our startup, so I'd certainly like to track grunt-express-server origin. Next time I pull from grunt-express-server I'll revisit, looking at #10 and maybe submit a fresh PR.

@ericclemmons
Copy link
Owner

I'm getting your updates & a coffeescript version of genesis skeleton working together. It'll be tricky, but your code will make it much easier!

(Closing so I can reuse this PR in master while I resolve ericclemmons/genesis-skeleton#102)

Talk to you soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants