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 warning when HOST environment variable is set (#3719) #3730

Merged
merged 3 commits into from
Jan 14, 2018

Conversation

iansu
Copy link
Contributor

@iansu iansu commented Jan 10, 2018

To see the new warning message run HOST=8.8.8.8 npm run start.

Closes #3719

@Timer
Copy link
Contributor

Timer commented Jan 14, 2018

Can you please shorten the link?

Let's make the second log more along these lines:

`Learn more here: ${chalk.yellow('http://bit.ly/2mwWSwH')}`

if (process.env.HOST) {
console.log(
chalk.yellow(
"Attempting to bind to $HOST because it was specified. If this was unintentional, check that you haven't mistakenly set it in your shell."
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we inline the HOST value in the message?

Also it's not 100% clear what "$HOST" is. (For example windows uses a different syntax for environment variables.) It is better to explicitly say "environment variable".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if it was something more like "Attempting to bind to HOST environment variable somehostname.example.com. If this was..."

@Timer Timer modified the milestones: 2.x, 1.0.18 Jan 14, 2018
Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

LGTM (haven't run it though)

@Timer
Copy link
Contributor

Timer commented Jan 14, 2018

Thanks!

@Timer Timer merged commit b86fe05 into facebook:master Jan 14, 2018
@Timer
Copy link
Contributor

Timer commented Jan 14, 2018

Adjusted the colors a bit:

@iansu
Copy link
Contributor Author

iansu commented Jan 14, 2018

Looks good! 👍

Pavek pushed a commit to Pavek/create-react-app that referenced this pull request Jul 10, 2018
* Add warning when HOST environment variable is set (facebook#3719)

* Improve HOST environment variable warning message

* Adjust text and message

Closes facebook#3719
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Aug 14, 2018
* Add warning when HOST environment variable is set (facebook#3719)

* Improve HOST environment variable warning message

* Adjust text and message

Closes facebook#3719
@lock lock bot locked and limited conversation to collaborators Jan 20, 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.

4 participants