Handle process stdin

Created by: Kjir

This PR continues the work started in #1754 so that it might be merged.

I rebased the changes on top of master (which should fix the test errors, hopefully) and I created the helper in react-dev-utils as suggested in the previous code review.

I tested that it still works with a very simple python program:

import time

time.sleep(10)

After 10 seconds the program does actually quit, both when watching test and when running the dev server.

I am not sure there is a suitable way of adding a test for this, if I receive some suggestions about this I will gladly implement them!

Fixes #1753 (closed)

added CLA Signed label

Created by: Timer

Great, thank you!

This doesn't seem to play well with our CI; could you please look into why and fixing that?

Created by: Kjir

To be honest it looks to me like this is a pre-existing condition which is shared between many of the latest pull request. Even on master I see the same failure on appveyor: https://ci.appveyor.com/project/gaearon/create-react-app-a3khu/build/1.0.1675/job/appdu6xgv2it578i

I will look into it to see if I can untangle it, but I doubt it has anything to do with the changes introduced here. I will let you know!

Created by: Kjir

Looking more attentively I could replicate the issue on my machine and I will now address it! But the problem I have is the one on Travis and not the one on appveyor...

Created by: Kjir

After a lot of head scratching, I found out what the problem is thanks to this question: https://unix.stackexchange.com/questions/3886/difference-between-nohup-disown-and

What happens is that when bash puts a process as a background job, it will halt that program as soon as it tries to read from STDIN.

What this means is that with this patch will make it harder to run npm start as a background job, requiring you to give it some input (e.g.: npm start </dev/zero &). I do not know whether this affects someone or it doesn't, because I don't know if people use the webserver this way.

Created by: Kjir

Just to be on the safe side, I also added a note in the Troubleshooting section.

Created by: Kjir

There could be an alternative: this functionality is only enabled through a flag that we would provide to npm start. While it goes against the general policy of not having flags and configurations, it is also true that the "Listen to stdin to close" is only needed when used in combination with other tools — namely the Phoenix framework.

Since this decision is not technical I await further insights and will eventually change my PR to go in this other direction, if deemed preferable.

Created by: Kjir

In all this I forgot about Windows! At this point the only viable option is to add the flag to npm start, so that people using Phoenix can fulfill the requirement without disrupting everyone else. The flag is not meant to be used in normal, manual situations, so I think it is an acceptable compromise.

(Sorry about the numerous comments)

mentioned in issue #3581 (closed)

Created by: topaxi

Anything that needs to be done to get this merged / drive this forward? Let me know if I can help out.

Created by: stale[bot]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs.

added stale label

Created by: stale[bot]

This pull request has been automatically closed because it has not had any recent activity. The conversation will be locked in 7 days unless the pull request is reopened. Thank you for your contribution.

closed