Stop watch process when process.stdin ends closes #1753

[wmonk]

What has been done

• When process.stdin fires an end event, the process exits with a 0 exit code

How has this been tested

• Started the app normally ✅
• Started the app when another app was already running (chose a new port) ✅
• Started the app via phoenix ✅

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[gaearon]

Should this also happen for build and test?
Could you explain more about what resume() does?

[wmonk]

I don't think this needs to happen for build and test, both of those scripts will exit once they've finished processing.

To be honest, i'm not 100% familiar with what resume(), but has been used by all other mentioned projects to enable this behaviour. From what I have read, it essentially starts reading from the stdin stream, which it doesn't do automatically unless you explicitly call process.stdin.on('data', callback). You can test it with:

$ echo "{ foo: 'bar' }" | node -e 'process.stdin.on("data", d => console.log(d.toString()))'
// { foo: 'bar' }

$ echo "{ foo: 'bar' }" | node -e 'process.stdin.on("end", () => console.log("hello world"))'
// 

$ echo "{ foo: 'bar' }" | node -e 'process.stdin.on("end", () => console.log("hello world")); process.stdin.resume()'
// hello world

[gaearon]

I don't think this needs to happen for build and test, both of those scripts will exit once they've finished processing.

True for build, but test runs a watcher by default.

[wmonk]

@gaearon have just tested this, and you are right, the test process hangs around.

[gaearon]

Let's add a comment here (and in the other file) explaining what this does and linking to the issue.

[wmonk]

👍

[wmonk]

Do you think it's worth toggling this with an env var? Webpack uses --stdin to turn this behaviour on - although testing worked fine, I don't want to inadvertently break some other scenarios.

[gaearon]

If we break we'll learn from that. I think it's fine to start with on by default in the next major (technically minor since we're in 0.x), and see what breaks.

[gaearon]

We need to only do this in non-CI mode. Move this into if statement below plz.

[ianmcnally]

Design/style Q: would it make sense to make a helper, since this code is duplicated across two files?

Like:

// in a new dev script

function addExitHandlersToProcess(process) {
  process.stdin.on('end', function() {
    process.exit(0);
  });
  process.stdin.resume();
  }
}

And call it in start.js and test.js. Along with cutting down duplication, it would add some self-documentation.

[gaearon]

We could put it in react-dev-utils I guess, like we do with other helpers. Then this doesn't need to live in user projects after ejecting.

[gaearon]

For some reason this breaks the CI.

[gaearon]

I’m open to getting this in, but the CI needs to pass.