exit watch process on EOF / Ctrl-D

[tverlaan]

It's quite common to be able to exit from a running process with Ctrl-D. This also makes it possible to integrate postcss-cli in external applications as is also mentioned in this webpack PR: https://github.com/webpack/webpack/pull/1311.

I could also make it an additional flag if that is preferred.

[RyanZim]

What's the use-case for this?

[tverlaan]

I embed the postcss-cli watcher in my own application (more info can be found in the webpack-pr I linked). When my application exits it closes stdin for the postcss-cli watcher, however postcss-cli keeps running in the background. On exiting/starting my application multiple times I create multiple orphaned opencss-cli watcher processes. I have to manually kill them.

[coveralls]

Coverage decreased (-0.9%) to 81.29% when pulling 3f380e2ae07fd1e75fa7a690b3924e47de5be1ca on tverlaan:tv_exit_on_stdin_eof into e279de81cb48a008070fea20f9db2e6db2feb675 on postcss:master.

[coveralls]

Coverage decreased (-1.07%) to 81.169% when pulling d79bb8df7f20cfe3cfe94ddd63fdd2c9a0b2cfa3 on tverlaan:tv_exit_on_stdin_eof into e279de81cb48a008070fea20f9db2e6db2feb675 on postcss:master.

[RyanZim]

Why can't your application just send SIGINT/Ctrl+C to the postcss-cli process when it closes?

[tverlaan]

I think it potentially could, but then it would only be for postcss-cli. An external application that starts postcss-cli usually controls stdin & stdout, therefor it makes most sense to use mechanisms that are already available. Non-daemon applications usually bind to stdin&stdout and therefor it would make sense if it responds to this. The external application is not sending signals, it's just closing and opening stdin/stdout for the child process. Webpack, Brunch & create-react-app all implement it like this and I'd rather stick to the same solution here.

[RyanZim]

OK, fair enough. I still think it's a bit of an odd pattern, but if the ecosystem has it, I guess we can do it. This will need an automated test written, though, before I can merge this.

[tverlaan]

It's quite common for UNIX cli tools to behave like this, shells for example: https://unix.stackexchange.com/questions/110240/why-does-ctrl-d-eof-exit-the-shell

I hope the test is ok like this :-)

[RyanZim]

It's quite common for UNIX cli tools to behave like this, shells for example: https://unix.stackexchange.com/questions/110240/why-does-ctrl-d-eof-exit-the-shell

This behavior is indeed standard for interactive programs, like shells or REPLs, that are controlled by user input on stdin, I find it odd for a program that does not read stdin for anything else in watch mode, that's all.

[RyanZim]

Looks great, only one problem; I tested applying the commit with the test to master, and the new test passes even without your original patch, so something with the test isn't working correctly. :slightly_frowning_face:

[tverlaan]

Nice catch! I changed the implementation a bit, the test works properly now. I think the catch(err) was actually preventing a clean exit which was actually pointing to a flawed implementation from my side. Should be fixed now.

[RyanZim]

I messed around a bit, the test was sort of failing for the wrong reason earlier; here's a patch to make it better:

diff --git a/test/watch.js b/test/watch.js
index a6cf2e4..4a51a11 100644
--- a/test/watch.js
+++ b/test/watch.js
@@ -8,6 +8,7 @@ const chokidar = require('chokidar')

 const ENV = require('./helpers/env.js')
 const read = require('./helpers/read.js')
+const tmp = require('./helpers/tmp.js')

 // XXX: All the tests in this file are skipped on the CI; too flacky there
 const testCb = process.env.CI ? test.cb.skip : test.cb
@@ -287,13 +288,17 @@ testCb("--watch doesn't exit on CssSyntaxError", (t) => {
 })

 testCb('--watch does exit on closing stdin (Ctrl-D/EOF)', (t) => {
-  t.plan(0)
+  t.plan(1)

   const cp = spawn(
-    `node ${path.resolve('bin/postcss')} -o output.css -w --no-map`,
+    `./bin/postcss test/fixtures/a.css -o ${tmp()} -w --no-map`,
     { shell: true }
   )
+
   cp.on('error', t.end)
-  cp.on('exit', t.end)
+  cp.on('exit', (code) => {
+    t.is(code, 0)
+    t.end()
+  })
   cp.stdin.end()
 })

Copy that text into a file, then run git apply <file> from the working directory on your branch, commit & push, and this should be good to merge.

[tverlaan]

Thanks a lot! I'm not familiar enough with the testing framework and checking if the code = 0 explicitly looks a lot better than just passing it along. Also, git apply - allows for reading from stdin, no need for the tempfile :-)

[RyanZim]

Published v8.3.0 :tada:

[bug-tape]

It would have been nice to have this new behaviour only with a new command line argument. This change effectively stopped my docker containers. Luckily this was not a big problem, only surprising. Docker has a simple solution for that: stdin_open: true.

[RyanZim]

@bug-tape Ah, sorry about that. I didn't think it would affect any existing use-cases, but apparently it did yours. I'll try to be more cautious in the future.