-
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
adding hostname argument to CLI #1017
Conversation
bin/next-start
Outdated
@@ -10,11 +10,13 @@ process.env.NODE_ENV = process.env.NODE_ENV || 'production' | |||
const argv = parseArgs(process.argv.slice(2), { | |||
alias: { | |||
h: 'help', | |||
hn: 'hostname', |
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 -H
or -a
(address) is better.
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.
We use -H
for micro
lets use that to keep it consistent.
-H is used by other tools I've seen
…On Mon, Feb 6, 2017, 6:22 PM Naoyuki Kanezawa ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In bin/next-start
<#1017 (review)>:
> @@ -10,11 +10,13 @@ process.env.NODE_ENV = process.env.NODE_ENV || 'production'
const argv = parseArgs(process.argv.slice(2), {
alias: {
h: 'help',
+ hn: 'hostname',
I think -H or -a (address) is better.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1017 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABGR_HL25-jk5XkMkacpZjikdObR1eSks5rZ9VvgaJpZM4L4lJA>
.
|
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 made the change to the arg alias as requested. It's now -H instead of -hn
bin/next-start
Outdated
p: 'port' | ||
}, | ||
boolean: ['h'], | ||
default: { | ||
p: 3000 | ||
p: 3000, | ||
hostname: 'localhost' |
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 it'll be safe to not have default value for hostname.
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.
Yeah! I think with this it only listen on localhost
not with others like 127.0.0.1
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.
If there's no default then the second argument to listen()
will be undefined
. I'm not sure what that would do.
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.
taken from https://nodejs.org/api/http.html#http_server_listen_port_hostname_backlog_callback:
If the hostname is omitted, the server will accept connections on any IPv6 address (::) when IPv6 is available, or any IPv4 address (0.0.0.0) otherwise.
@nkzawa is right 😄
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.
"omitted", but what if undefined
or null
is passed? There's one way to find out I guess
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.
@jessehattabaugh great though actually. Only now I'm remembering that when you pass -H
without a hostname it will actually turn into true
server/index.js
Outdated
await this.prepare() | ||
this.http = http.createServer(this.getRequestHandler()) | ||
await new Promise((resolve, reject) => { | ||
// This code catches EADDRINUSE error if the port is already in use | ||
this.http.on('error', reject) | ||
this.http.on('listening', () => resolve()) | ||
this.http.listen(port) | ||
if (hostname) this.http.listen(port, hostname) |
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 decided to be careful and only call listen() with a hostname arg if something truthy is passed to start
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.
please make this a if/else with {
and }
, standardjs should have caught this 🤔
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.
taken from my other comment:
@jessehattabaugh great though actually. Only now I'm remembering that when you pass
-H
without a hostname it will actually turn intotrue
If you check for hostname
it will evaluate to true when passing -H
😅 It think it's best to check if it is a valid number or something like that 🤔
cc @nkzawa
await this.prepare() | ||
this.http = http.createServer(this.getRequestHandler()) | ||
await new Promise((resolve, reject) => { | ||
// This code catches EADDRINUSE error if the port is already in use | ||
this.http.on('error', reject) | ||
this.http.on('listening', () => resolve()) | ||
this.http.listen(port) | ||
if (hostname && typeof hostname !== 'boolean') { |
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.
@timneutkens good catch about empty -H evaluating true
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 we can use the string
option of minimist
.
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.
True, merging this in, will fix after merging.
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'll fix @nkzawa's note on minimist's string option.
Would it make sense to have those things ( |
No, as they're runtime values and are only applicable when using |
Hm, ok, that sounds plausible. |
You could create a shell script with predefined options. |
No description provided.