Skip to content
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

coffee spawns own, uncontrollable child process #50

Merged
merged 3 commits into from
Apr 22, 2014
Merged

coffee spawns own, uncontrollable child process #50

merged 3 commits into from
Apr 22, 2014

Conversation

paulpflug
Copy link
Collaborator

when using coffee as cmd the server is not restartable
workaround by using node with local coffe-script instead
(similar to here)

could be tested by comparing pid of child process on grunt/child side

without fix:
Starting background Express server pid of serverchild on grunt side: 3048 pid of serverchild on child side: 8812 Express server listening on port 9000 in development mode
with fix:
Starting background Express server You are using Coffee-script coffee does not allow a restart of the server found local coffee-script: ./node_modules/coffee-script/bin/coffee using nodejs with local coffe-script instead pid of serverchild on grunt side: 8416 pid of serverchild on child side: 8416 Express server listening on port 9000 in development mode

when using coffee as cmd the server is not restartable
workaround by using node with local coffe-script instead
Changed behavior to only warn of coffee
Added cmdArgs option
@paulpflug
Copy link
Collaborator Author

I changed it to only warn the user against the use of coffee
To support coffee-script, I implemented the cmdArgs option which was asked for in #44

@paulpflug paulpflug mentioned this pull request Mar 20, 2014
@paulpflug
Copy link
Collaborator Author

Very similar to #47 >_<

@ericclemmons
Copy link
Owner

@paulpflug This is great work! Dumb question, but is it worth me putting in a coffee example in the Gruntfile.js?

@paulpflug
Copy link
Collaborator Author

Do you mean test? Then I would say no, because it is a bug of coffee.
If you really mean example, then I would say yes, because I love examples ;)

@ericclemmons
Copy link
Owner

Then yes, example :) I'd like to add one to this project's Gruntfile.js that shows the "right" way of doing it based on your PR.

@paulpflug
Copy link
Collaborator Author

I also committed an example to readme.md ;)

@ericclemmons
Copy link
Owner

Good enough for me. Shall I merge?

@paulpflug
Copy link
Collaborator Author

we should discuss the name of the new option..
#47 proposed "opts", which looks nicer than my "cmdArgs" ..

If you merge, you can close #37, #44, #47, #50 and #52
They are all fixed with the new option.

@ericclemmons
Copy link
Owner

I have to say, being able to merge this & close 5 issues sounds... good :)

@paulpflug
Copy link
Collaborator Author

Ok. I renamed cmdArgs to opts, which is clearer.. I think
So in my eyes you can merge.

@paulpflug
Copy link
Collaborator Author

When you are at it you could also merge #54, close #53 and make a new version ;)

@ericclemmons ericclemmons merged commit fa7b3af into ericclemmons:master Apr 22, 2014
@ericclemmons
Copy link
Owner

@paulpflug et. al. (did I use that right? :D)

v0.4.15 is out with this PR merged in. I hope it works great for you guys!

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