-
Notifications
You must be signed in to change notification settings - Fork 27k
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
Conversation
console.log('> Ready on http://localhost:%d', argv.port) | ||
}) | ||
|
||
run(srv, argv.port) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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') |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
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.
e23b2f2
to
c0a443e
Compare
Updated:
Ready for review. |
Thank you! Awesome work. I'd like a bit of a simpler approach to this, however. If you run Prompting and all that seems a bit much? |
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. |
Agreed with @STRML - quietly deviating from default behavior is an anti-pattern - best to be verbose with a bit much in that case. |
then you better specify exact port. |
Thanks for continuing the discussion! If you run
What would be a good reason that you're likely to run into? And that you would not be better off by running 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 |
Detect port authority new to routing sorry :)
|
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? |
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 |
Every line of code we put in is a liability I agree with both @rauchg and @mbilokonsky.
Or something similar. |
@arunoda that sounds great to me |
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. |
The double dashes are not a |
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⏎