Rerun tests using "r" key in watch mode #658

vadimdemedes · 2016-03-18T17:46:04Z

This PR fixes #615.

mention-bot · 2016-03-18T17:46:07Z

By analyzing the blame information on this pull request, we identified @sotojuan, @ivogabe and @jokeyrhyme to be potential reviewers

sindresorhus · 2016-03-19T12:37:15Z

lib/watcher.js

-		if (data !== 'r' && data !== 'rs') {
+	stdin.on('keypress', function (char, key) {
+		if (key.ctrl && key.name === 'c') {
+			process.exit();


process.exit(2);

?

👍 (why 2?)

Control-C is fatal error signal 2, (130 = 128 + 2, see above)

http://tldp.org/LDP/abs/html/exitcodes.html

I've seen 2 used many times for SIGINT overrides.

Fixed, thx!

Rerun tests using "r" key in watch mode

novemberborn · 2016-03-20T18:49:40Z

Sorry didn't have a chance earlier to play with this. I have some concerns around the keyboard interaction. For instance if you run the watcher through npm (say npm run test:watch), ctrl+c now causes npm to spit out an error log. Previously the SIGINT would be handled by npm run. One cannot use Enter to insert some linebreaks either, which can be useful to discern repeat output.

ctrl+\ (SIGQUIT) causes a crash. ctrl+z is ignored.

I suppose we could switch to a 0 exit signal for the npm interaction, and even make Enter insert linebreaks. But there's a bunch of other shell behaviors that are broken by this. How much would we want to replicate them and is it worth the hassle?

jamestalmage · 2016-03-21T00:50:15Z

ctrl+\ (SIGQUIT) causes a crash. ctrl+z is ignored.

Do people actually use those?

ctrl+c now causes npm to spit out an error log

Agreed. That is a problem. Is there a way to 1) detect if you have a parent process, and 2) send a SIGINT to it? I would rather not exit with 0 (though that may be preferable to npms big old log when it sees an error.

Aside: It would be really nice if npm didn't print that huge mess of log statements when a script fails. So annoying to scroll up past that. I have a hard time believing all that "this isn't a problem with npm" language significantly reduces their support requests.

But there's a bunch of other shell behaviors that are broken by this. How much would we want to replicate them and is it worth the hassle?

I guess that depends on what those behaviors are, and what problems we are creating for users. If it creates real problems, lets just revert to the r+enter behavior (one more keypress is not that big of deal). We should definitely make a decision on that before publishing this though.

One last thought. I believe the original r+enter behavior was modeled off existing libraries. I would assume they knew about rawMode as well. They may have evaluated this same behavior and rejected it for the reasons being discussed now. @novemberborn It might be worth doing an issue search on some of those repos to see if they discussed this.

sindresorhus · 2016-03-21T06:20:16Z

Do people actually use those?

Never used either personally.

It would be really nice if npm didn't print that huge mess of log statements when a script fails. So annoying to scroll up past that.

Couldn't agree more! The npm CLI generally has pretty awful UX.

Related issue: npm/npm#8821 (Please comment there!)

"this isn't a problem with npm" language significantly reduces their support requests.

The irony is that usually it is a problem with npm even when they output that. So the usual flow is a user opening an issue on one of my repos, I have to waste time triaging, and just send them back to the npm issue tracker...

novemberborn · 2016-03-21T11:05:22Z

Do people actually use those?

They can be useful when the app has trapped the SIGINT signal, but yea I don't really use them either. Just raising their existence and incompatibility with the restart mode in this PR.

Agreed. That is a problem. Is there a way to 1) detect if you have a parent process, and 2) send a SIGINT to it? I would rather not exit with 0

Not sure. I don't think the exit code matters all that much when in watch mode though.

I believe the original r+enter behavior was modeled off existing libraries. I would assume they knew about rawMode as well. They may have evaluated this same behavior and rejected it for the reasons being discussed now.

Yes, see remy/nodemon#162 (comment). Also:

to listen for a keypress requires changing process.stdin to use setRawMode(true) which lets me read a character at a time, but this then passes on the stdin reading method to the child process (i.e. your node script) which would cause unexpected behaviour.

Not sure how much this impacts us, it might be due to how the child processes are forked.

novemberborn · 2016-03-25T16:30:53Z

Just learned about SIGINFO https://twitter.com/robsmallshire/status/706763512903819265, another signal broken by this PR.

(Not that it's very useful, it just outputs stuff like load: 1.85 cmd: node 18425 waiting 0.44u 0.06s)

vadimdemedes · 2016-03-25T21:55:14Z

I guess we should rollback this PR and just rerun via r + Enter. Will be easier for everyone and additional "Enter" is not big a deal.

jamestalmage · 2016-03-26T00:10:47Z

Argument for keeping this:

We can change this back at any point. It's not going to break builds. People are just going to have to retrain "r" vs "r + enter". For all the problems we've listed, I haven't seen one I think is a practical problem yet.

Argument for reverting:

It's the "safer" option. Guaranteed not to cause problems (however unlikely problems seem).

If anyone can describe a situation where this prevents you from using watch mode then let's revert. Heck, I would be convinced if just one person said "I frequently use Ctrl + SomethingOtherThanC".

novemberborn · 2016-03-26T14:01:42Z

Even if we don't revert we need to fix the exit code upon Ctrl + C.

I'm for reverting though, seems like an uphill battle trying to solve the various edge cases when they surface.

jamestalmage · 2016-03-26T18:40:37Z

Even if we don't revert we need to fix the exit code upon Ctrl + C.

Oh yeah, forgot about that. I don't love the idea of exiting with 0 on Ctrl + C. It's another one of those "probably not a problem, but just feels wrong" issues.

I think I'm convinced. Let's revert. @sindresorhus @vdemedes ?

vadimdemedes · 2016-03-27T18:38:06Z

I'm for reverting, one less thing to maintain. Didn't think such a simple PR would have such complications afterwards :)

sindresorhus · 2016-03-28T07:03:20Z

Ok, let's revert to R+Enter. I don't think people will need it that often anyways. Our dependency tracking is pretty good.

This reverts commit 5f109b6. See discussion in <#658 (comment)>.

sindresorhus reviewed Mar 19, 2016
View reviewed changes

vadimdemedes force-pushed the rerun-watch branch from 56078f3 to b0e44d8 Compare March 20, 2016 08:27

rerun tests using "r" key

5f109b6

vadimdemedes force-pushed the rerun-watch branch from b0e44d8 to 5f109b6 Compare March 20, 2016 08:41

vadimdemedes pushed a commit that referenced this pull request Mar 20, 2016

Merge pull request #658 from sindresorhus/rerun-watch

ad02c32

Rerun tests using "r" key in watch mode

vadimdemedes merged commit ad02c32 into master Mar 20, 2016

vadimdemedes deleted the rerun-watch branch March 20, 2016 13:02

novemberborn added a commit that referenced this pull request Mar 30, 2016

Revert "rerun tests using "r" key"

ba1f5e8

This reverts commit 5f109b6. See discussion in <#658 (comment)>.

novemberborn mentioned this pull request Mar 30, 2016

Revert "rerun tests using "r" key" #692

Merged

novemberborn mentioned this pull request Sep 24, 2017

Add support of tty raw mode #1533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rerun tests using "r" key in watch mode #658

Rerun tests using "r" key in watch mode #658

vadimdemedes commented Mar 18, 2016

mention-bot commented Mar 18, 2016

sindresorhus Mar 19, 2016

jamestalmage Mar 19, 2016

sindresorhus Mar 19, 2016

vadimdemedes Mar 20, 2016

novemberborn commented Mar 20, 2016

jamestalmage commented Mar 21, 2016

sindresorhus commented Mar 21, 2016

novemberborn commented Mar 21, 2016

novemberborn commented Mar 25, 2016

vadimdemedes commented Mar 25, 2016

jamestalmage commented Mar 26, 2016

novemberborn commented Mar 26, 2016

jamestalmage commented Mar 26, 2016

vadimdemedes commented Mar 27, 2016

sindresorhus commented Mar 28, 2016

Rerun tests using "r" key in watch mode #658

Rerun tests using "r" key in watch mode #658

Conversation

vadimdemedes commented Mar 18, 2016

mention-bot commented Mar 18, 2016

sindresorhus Mar 19, 2016

Choose a reason for hiding this comment

jamestalmage Mar 19, 2016

Choose a reason for hiding this comment

sindresorhus Mar 19, 2016

Choose a reason for hiding this comment

vadimdemedes Mar 20, 2016

Choose a reason for hiding this comment

novemberborn commented Mar 20, 2016

jamestalmage commented Mar 21, 2016

sindresorhus commented Mar 21, 2016

novemberborn commented Mar 21, 2016

novemberborn commented Mar 25, 2016

vadimdemedes commented Mar 25, 2016

jamestalmage commented Mar 26, 2016

novemberborn commented Mar 26, 2016

jamestalmage commented Mar 26, 2016

vadimdemedes commented Mar 27, 2016

sindresorhus commented Mar 28, 2016