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

WIP prompt if port is already in use. #101

Closed
wants to merge 14 commits into from

Conversation

chocnut
Copy link
Contributor

@chocnut chocnut commented Jul 22, 2016

Here's my initial attempt to #85

screen shot 2016-07-23 at 5 25 11 am

screen shot 2016-07-23 at 5 25 32 am

@facebook-github-bot
Copy link

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!

@sotojuan
Copy link

FWIW I was working on a solution for the port issue here that doesn't use a prompt/notification but it's clear you put in a lot of work here so happy to give it to you if the powers that be accept.

@facebook-github-bot
Copy link

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

@chocnut
Copy link
Contributor Author

chocnut commented Jul 22, 2016

Hey @sotojuan yeah I saw you push too, since this is my quick hack maybe we can work on this fix.

@ghost ghost added the CLA Signed label Jul 22, 2016
@chocnut chocnut changed the title WIP notify if port is used and ask to confirm to use available port. WIP prompt if port is used and ask to confirm to use available port. Jul 22, 2016
@ghost ghost added the CLA Signed label Jul 22, 2016
@chocnut chocnut changed the title WIP prompt if port is used and ask to confirm to use available port. WIP prompt if port is used. Jul 22, 2016
@chocnut chocnut changed the title WIP prompt if port is used. WIP prompt if port is already in use. Jul 22, 2016
@ghost ghost added the CLA Signed label Jul 22, 2016
@gaearon
Copy link
Contributor

gaearon commented Jul 23, 2016

This doesn’t merge cleanly right now, could you rebase please?

formattedErrors.forEach(message => {
console.log(message);
console.log();
rl.question('Something is already running at http://localhost:' + PORT + '. Would you like to run the app at another port instead? [Y/n] ', function(answer){
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nit, can you add a space between ) and {: function(answer) {

@chocnut
Copy link
Contributor Author

chocnut commented Jul 24, 2016

@eanplatter should be fixed now.

@ghost ghost added the CLA Signed label Jul 24, 2016
@mxstbr
Copy link
Contributor

mxstbr commented Jul 24, 2016

I'm not sure if we're supporting Node 0.12, if we are those will need to be anonymous functions since 0.12 doesn't support arrow functions.

@chocnut
Copy link
Contributor Author

chocnut commented Jul 24, 2016

@mxstbr @eanplatter I've removed arrow functions for consistency

@ghost ghost added the CLA Signed label Jul 24, 2016
@chocnut
Copy link
Contributor Author

chocnut commented Jul 26, 2016

@gaearon moved prompt to utilities.

@ghost ghost added the CLA Signed label Jul 26, 2016
@chocnut
Copy link
Contributor Author

chocnut commented Jul 26, 2016

still checking out why ci failed.

@queso
Copy link

queso commented Jul 26, 2016

Why remove the ability to set a port via the environment? Using a tool like hotel works best when it can set the port for you, so removing that seems like a bad idea. Is there another way to set the port via command line?

@jlbruno
Copy link

jlbruno commented Jul 26, 2016

@queso did this ability exist before this PR? I don't believe any ability is being removed.

@ghost ghost added the CLA Signed label Jul 26, 2016
@brownieboy
Copy link

For me, the error is slightly different:

Error: listen EACCES 127.0.0.1:3000

I'm running Windows, and my company appears to have something called Appsense running something or other on port 3000, which I can't shut down (and would likely be breaking some IT policy if I did).

I don't need any clever port detection/reassignment. Just give me a way of overriding it with a command line switch.

@gaearon
Copy link
Contributor

gaearon commented Jul 27, 2016

Why remove the ability to set a port via the environment? Using a tool like hotel works best when it can set the port for you, so removing that seems like a bad idea. Is there another way to set the port via command line?

It was not removed, this ability did not exist in the first place. We do plan to introduce it in some way. Please read #102.

@gaearon
Copy link
Contributor

gaearon commented Jul 27, 2016

I'm running Windows, and my company appears to have something called Appsense running something or other on port 3000, which I can't shut down (and would likely be breaking some IT policy if I did).

As far as I understand, this PR should solve temporarily solve the problem for you. We can discuss a broader solution in #102. Please bear in mind this project has existed for a couple of weeks, so we can‘t add all requested features at once. 😉

@queso
Copy link

queso commented Jul 27, 2016

I misread a line of code... I was looking at 10c6adf#diff-1ab6f53c8795042e5d1f961d6e6b75e5L22

I read it backwards, false alarm. I actually think this PR would make it easier to override a port from the command line, so I would be interested in this.

@gaearon
Copy link
Contributor

gaearon commented Jul 27, 2016

Squashed and merged with a few fixes via 2edf218.
Thank you very much!
(I credited the result commit to you.)

@gaearon
Copy link
Contributor

gaearon commented Jul 28, 2016

@queso

Newly created apps will now offer to run on a different port if the default is taken. And you can override the default with PORT environment variable if you really need to.

You can either create a new app or update the react-scripts dependency to 0.2.0 for this to be available.

@brownieboy
Copy link

brownieboy commented Jul 29, 2016

Works fine for me with 0.2.0. The code picked up that "Something is already running at port 3000" and offered to switch to another port. It picked port 3001 when I hit "y", and all good after that.

@gaearon
Copy link
Contributor

gaearon commented Jul 29, 2016

(You can also just press as y is the default)

@MoOx
Copy link
Contributor

MoOx commented Aug 10, 2016

Just curious: why do you ask and why don't you just start on another port? In most case this user might answer "yes".

@gaearon
Copy link
Contributor

gaearon commented Aug 10, 2016

You can press Enter to skip the message (it defaults to "yes").

We ask because sometimes people run two instances unintentionally (eg in another terminal tab) and don't actually want two processes.

STRML added a commit to STRML/next.js that referenced this pull request Oct 25, 2016
STRML added a commit to STRML/next.js that referenced this pull request Oct 26, 2016
Based on create-react-app and specifically
facebook/create-react-app#101

In production, will exit(1) if the port is in use.
@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.