-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 'lan' option (modify the option name to ‘useLocalIp’ for more semantic) #901
Conversation
Let the browser open with local IP.
Let the browser open with local IP.
Codecov Report
@@ Coverage Diff @@
## master #901 +/- ##
=========================================
- Coverage 72.23% 72.13% -0.1%
=========================================
Files 4 4
Lines 461 463 +2
Branches 138 139 +1
=========================================
+ Hits 333 334 +1
- Misses 128 129 +1
Continue to review full report at Codecov.
|
lib/util/createDomain.js
Outdated
@@ -1,13 +1,31 @@ | |||
"use strict"; | |||
const url = require("url"); | |||
const networkInterfaces = require("os").networkInterfaces(); | |||
|
|||
function getLocalIP() { |
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.
Can you write tests for 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.
No problem, I will add some tests later.
lib/util/getLocalIP.js
Outdated
@@ -0,0 +1,19 @@ | |||
"use strict"; |
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 use https://www.npmjs.com/package/internal-ip (first choice) or https://www.npmjs.com/package/dev-ip to replace the need for this file.
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.
Good idea, I will use internal-ip
to replace this file.
lib/util/createDomain.js
Outdated
|
||
module.exports = function createDomain(options) { | ||
const protocol = options.https ? "https" : "http"; | ||
|
||
// the formatted domain (url without path) of the webpack server | ||
return options.public ? `${protocol}://${options.public}` : url.format({ | ||
protocol: protocol, | ||
hostname: options.host, | ||
hostname: options.lan ? getLocalIP() : options.host, |
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 don't feel that lan
is semantic for this feature. options.useLocalIp
or some such would probably be easier to understand at first glance.
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, agreed. 👍
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.
@shellscape You're right, I will change the word to useLocalIp
to make it more semantic.
And thank you for giving me all the advice.
Looks good. Please sign the CLA (#901 (comment)) and we can get this merged |
@shellscape I have already signed the CLA. |
Let the browser open with local IP.
What kind of change does this PR introduce?
feature
Did you add or update the
examples/
?no
Summary
According to #147, server can be accessed via IP, but when set
host
to0.0.0.0
andopen
totrue
, the browser will openhttp://0.0.0.0
, but it's inaccessible.This PR allows to set
lan
useLocalIp
totrue
, so that the browser can open with local IP.Example usage in webpack config:
Does this PR introduce a breaking change?
no