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

Use detect-port to check if 3000 is already used. #68

Closed
wants to merge 1 commit into from

Conversation

STRML
Copy link

@STRML STRML commented Oct 25, 2016

Based on create-react-app and specifically
facebook/create-react-app#101.

$ ./dist/bin/next-start
Something is already running at port 3000.
Would you like to run the app on port 3001 instead? [Y/n] y

Ready on http://localhost:3001
^C⏎

console.log('> Ready on http://localhost:%d', argv.port)
})

run(srv, argv.port)
Copy link
Contributor

@nkzawa nkzawa Oct 26, 2016

Choose a reason for hiding this comment

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

next start is expected to be used for production, thus I think just throwing an error can be better.
Or does this work even on like automated deployment too ?

Copy link
Author

Choose a reason for hiding this comment

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

I agree, let's just throw an error in next-start, will update. Throwing an error is still better than existing behavior, currently it will throw nothing and the previous owner of the port will simply be the only one bound.

@@ -26,7 +26,7 @@ gulp.task('compile', [
])

gulp.task('compile-bin', () => {
return gulp.src('bin/*')
return gulp.src('bin/**/*.js')
Copy link
Contributor

@nkzawa nkzawa Oct 26, 2016

Choose a reason for hiding this comment

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

I think bin/next and next/next-build etc wouldn't be compiled with this setting ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is true due to the extension - fixing

@nkzawa
Copy link
Contributor

nkzawa commented Oct 26, 2016

btw it seems great to support !

Based on create-react-app and specifically
facebook/create-react-app#101

In production, will exit(1) if the port is in use.
@STRML
Copy link
Author

STRML commented Oct 26, 2016

Updated:

~/g/f/next.js (feature/detect-port) $ ./dist/bin/next-start
Error: Something is already running at port 3000. Exiting.
~/g/f/next.js (feature/detect-port) $ ./dist/bin/next-dev
Something is already running at port 3000.
Would you like to run the app on port 3001 instead? [Y/n] y

Ready on http://localhost:3001

Ready for review.

@rauchg
Copy link
Member

rauchg commented Oct 26, 2016

Thank you! Awesome work. I'd like a bit of a simpler approach to this, however.

If you run next without specifying a port, and 3000 is taken, we warn (> Note: default port already in use! Picking an available port), and pick a free one.

Prompting and all that seems a bit much?

@STRML
Copy link
Author

STRML commented Oct 26, 2016

I don't think it's a bit much - you may have a good reason for not picking another port and you want to be prompted. Perhaps you could miss the prompt outright if it just started up, especially if your application prints a lot of logs on startup.

In any case, this is convention from create-react-app and I believe it really works well there.

@mmmeff
Copy link

mmmeff commented Oct 26, 2016

Agreed with @STRML - quietly deviating from default behavior is an anti-pattern - best to be verbose with a bit much in that case.

@iamstarkov
Copy link

you may have a good reason for not picking another port and you want to be prompted

then you better specify exact port.

@iamstarkov
Copy link

iamstarkov commented Oct 26, 2016

So i agree with @rauchg if no port is specified, app should run on whatever port is available

Though, if port is specified, then its better to process.exit(-1), as @STRML noted, that if you need specified port, and its not available, you dont want to run app on other ports

@rauchg
Copy link
Member

rauchg commented Oct 29, 2016

Thanks for continuing the discussion!

If you run next, you're not specifying a port. You're telling next to use default behavior.
In our case, the default behavior would not be 3000, it'd be "a friendly port we can find". In fact, we could try 3000, 8080, 3000+i

you may have a good reason for not picking another port and you want to be prompted.

What would be a good reason that you're likely to run into? And that you would not be better off by running next -p {port you really need}

I think adding so much logic to ask the user a question that they likely will answer "yes" during development is not worth it.

Important to keep in mind: we're using this for next, not for production-ready next start. Therefore, it's not harmful to make more assumptions here.

@daytonna
Copy link

Detect port authority new to routing sorry :)
On Oct 29, 2016 6:49 AM, "Guillermo Rauch" notifications@github.com wrote:

Thanks for continuing the discussion!

If you run next, you're not specifying a port. You're telling next to use
default behavior.
In our case, the default behavior would not be 3000, it'd be "a friendly
port we can find". In fact, we could try 3000, 8080, 3000+i…

you may have a good reason for not picking another port and you want to be
prompted.

What would be a good reason that you're likely to run into? And that you
would not be better off by running next -p {port you really need}

I think adding 2 files to ask the user a question that they likely will
answer "yes" during development is not worth it.

Important to keep in mind: we're using this for next, not for
production-ready next start. Therefore, it's not harmful to make more
assumptions here.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#68 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AWD9F9s6Dpv1Gw8W5rCxCugAbsLjL3Thks5q4yTHgaJpZM4KgdJ0
.

@mbilokonsky
Copy link

Trick with automagic port selection is, what if you have a session running in a different terminal tab? You make your changes, you start again, you go refresh your browser thinking you're on port 3000, but your latest version quietly started on port 3001.

How long do you spend trying to figure out why you're not seeing your latest changes reflected?

I like prompting you to accept port 3001 because it will also force you to think "Oh, crap, what's running on 3000? Oh I left this running..." etc.

We've all been burned by that, right?

@STRML
Copy link
Author

STRML commented Nov 7, 2016

I think adding so much logic to ask the user a question that they likely will answer "yes" during development is not worth it.

It's a few lines - and it's very nice to be prompted as a way of saying "something out of the ordinary is happening here". Because the expectation, if you run it more than once, is that it will open on port 3000. And developers who use it will build that expectation that if you don't start it with -p, it will start on 3000. So if we have reason not to start on 3000, thus breaking expectations, a prompt (as opposed to just another log line) is a very good way to make this clear.

@arunoda
Copy link
Contributor

arunoda commented Nov 14, 2016

Every line of code we put in is a liability

I agree with both @rauchg and @mbilokonsky.
What if we say this:

Port 3000 is already in use.
Use `npm run dev -- -p <some other port>`.

Or something similar.

@rauchg
Copy link
Member

rauchg commented Dec 17, 2016

@arunoda that sounds great to me

@hegelstad
Copy link

Why do we need double dashes before -p?

Spent 15 minutes figuring this out. It's not easy to find in the docs either. Also, there is no error if you are running a docker exposed to port 3000.

@STRML
Copy link
Author

STRML commented Apr 10, 2017

The double dashes are not a next thing, they are how npm run (and many other utilities) mark the end of the parameter list. This indicates that you are sending the -p to the script executed by npm run dev, not to npm run itself.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
ijjk pushed a commit to ijjk/next.js that referenced this pull request Apr 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants