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

Add support for non-interactive terminal #1032

Merged
merged 1 commit into from
Nov 23, 2016
Merged

Add support for non-interactive terminal #1032

merged 1 commit into from
Nov 23, 2016

Conversation

sheerun
Copy link
Contributor

@sheerun sheerun commented Nov 10, 2016

This should fix #869

@sheerun
Copy link
Contributor Author

sheerun commented Nov 10, 2016

Also added a commit that should fix #873

@@ -31,6 +32,8 @@ var prompt = require('react-dev-utils/prompt');
var config = require('../config/webpack.config.dev');
var paths = require('../config/paths');

var isInteractive = tty.isatty(1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"A numeric file descriptor", it's correct

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so 1 is a magical number that means the standard output. Perhaps process.stdout.isTTY would be clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure... I've changed it


// We have switched off the default Webpack output in WebpackDevServer
// options so we are going to "massage" the warnings and errors and present
// them in a readable focused way.
var messages = formatWebpackMessages(stats.toJson({}, true));
if (!messages.errors.length && !messages.warnings.length) {
if (!messages.errors.length && !messages.warnings.length && isFirstCompile) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was testing this branch and after making a change the terminal screen just went blank. I thought the compiler had got stuck. Why did we change this behavior?

Copy link
Contributor

@fson fson Nov 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we could only show The app is running at... after the first compile, but keep showing Compiled successfully! after each successful rebuild?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This should be non-interactive only change. It's to prevent duplicate output in non-interactive terminal. I've just fixed it.


// We have switched off the default Webpack output in WebpackDevServer
// options so we are going to "massage" the warnings and errors and present
// them in a readable focused way.
var messages = formatWebpackMessages(stats.toJson({}, true));
if (!messages.errors.length && !messages.warnings.length) {
if (!messages.errors.length && !messages.warnings.length && (isInteractive || isFirstCompile)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please simplify this condition by extracting some boolean variables? It's getting crowdy and hard to tell when it's true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fixed..

@sheerun
Copy link
Contributor Author

sheerun commented Nov 20, 2016

It should be ready to merge..

@fson
Copy link
Contributor

fson commented Nov 20, 2016

I tested the non-interactive shell with concurrently. The output after starting the server, changing a file and waiting a few seconds looks like this:
screen shot 2016-11-21 at 0 25 55
Maybe it's just me, but it feels like the compiler is still recompiling because we print "Compiling..." but never "Compiled successfully!". Can we either remove the "Compiling..." output in non-interactive mode, or alternatively also print "Compiled successfully!" each time compiling has finished?

I'm not normally using a setup like this, so I'm not sure which alternative would be better, but the current behaviour doesn't seem right.

@sheerun
Copy link
Contributor Author

sheerun commented Nov 23, 2016

@fson I've changed code so it always displays "Compiled successfully!". Could we merge?

@fson fson merged commit dc6edce into facebook:master Nov 23, 2016
@fson
Copy link
Contributor

fson commented Nov 23, 2016

Thanks @sheerun, nice work!

@fson fson added this to the 0.8.0 milestone Nov 23, 2016
jarlef pushed a commit to jarlef/create-react-app that referenced this pull request Nov 28, 2016
alexdriaguine pushed a commit to alexdriaguine/create-react-app that referenced this pull request Jan 23, 2017
randycoulman pushed a commit to CodingZeal/create-react-app that referenced this pull request May 8, 2017
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for non-fullscreen "react-scripts start" server
4 participants