-
-
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
allow explicitly setting the protocol from the public option #1117
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
eecad00
allow explicitly setting the protocol from the public option
robertaistleitner 3b204ee
add feedback from PR + add example
robertaistleitner 972825e
fix linting issues
robertaistleitner 650182f
provide tests for createDomain helper function
robertaistleitner 21c54c6
increase timeout for https test which exceeds default 2000ms
robertaistleitner b1a4eb8
Merge branch 'master' into master
shellscape ef0e57b
Merge branch 'master' into master
robertaistleitner 9e6b272
move info log to the place where the reload happens
robertaistleitner cfdb070
apply feedback
robertaistleitner f481d0f
increase test timeout for https to 60 seconds
robertaistleitner e170f18
Merge branch 'master' into master
shellscape File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# CLI - public protocol | ||
|
||
NOTE: replace `<insert local ip>` with your local ip. | ||
|
||
```shell | ||
node ../../bin/webpack-dev-server.js | ||
``` | ||
|
||
You're now able to explicitly define the protocol used with the `public` option (have a look to the config provided in `webpack.config.js`). | ||
|
||
## What should happen | ||
|
||
If you open your browser at `http://localhost:8080` you'll see that dev-server tries to establish a connection to `/sockjs-node` via the explicitly defined `https://localhost:8080`. This fails of course since we're not hosting https. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
'use strict'; | ||
|
||
document.open(); | ||
document.write('Please check the sockjs-info request in your dev-tools, it should try to connect to the protocol + server defined in the public setting.'); | ||
document.close(); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
<!DOCTYPE html> | ||
<html> | ||
<head> | ||
<script src="bundle.js" type="text/javascript" charset="utf-8"></script> | ||
</head> | ||
<body> | ||
</body> | ||
</html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
'use strict'; | ||
|
||
module.exports = { | ||
context: __dirname, | ||
entry: './app.js', | ||
devServer: { | ||
host: '0.0.0.0', | ||
public: 'https://localhost:8080', | ||
disableHostCheck: true | ||
} | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
'use strict'; | ||
|
||
const webpack = require('webpack'); | ||
const internalIp = require('internal-ip'); | ||
const Server = require('../lib/Server'); | ||
const createDomain = require('../lib/util/createDomain'); | ||
const config = require('./fixtures/simple-config/webpack.config'); | ||
|
||
describe('check utility funcitons', () => { | ||
let compiler; | ||
before(() => { | ||
compiler = webpack(config); | ||
}); | ||
|
||
const tests = [{ | ||
name: 'default', | ||
options: { | ||
host: 'localhost', | ||
port: 8080 | ||
}, | ||
expected: 'http://localhost:8080' | ||
}, { | ||
name: 'https', | ||
options: { | ||
host: 'localhost', | ||
port: 8080, | ||
https: true | ||
}, | ||
expected: 'https://localhost:8080', | ||
timeout: 60000 | ||
}, { | ||
name: 'override with public', | ||
options: { | ||
host: 'localhost', | ||
port: 8080, | ||
public: 'myhost.test' | ||
}, | ||
expected: 'http://myhost.test' | ||
}, { | ||
name: 'override with public (port)', | ||
options: { | ||
host: 'localhost', | ||
port: 8080, | ||
public: 'myhost.test:9090' | ||
}, | ||
expected: 'http://myhost.test:9090' | ||
}, { | ||
name: 'override with public (protocol)', | ||
options: { | ||
host: 'localhost', | ||
port: 8080, | ||
public: 'https://myhost.test' | ||
}, | ||
expected: 'https://myhost.test' | ||
}, { | ||
name: 'override with public (protocol + port)', | ||
options: { | ||
host: 'localhost', | ||
port: 8080, | ||
public: 'https://myhost.test:9090' | ||
}, | ||
expected: 'https://myhost.test:9090' | ||
}, { | ||
name: 'localIp', | ||
options: { | ||
useLocalIp: true, | ||
port: 8080 | ||
}, | ||
expected: `http://${internalIp.v4()}:8080` | ||
}]; | ||
|
||
tests.forEach((t) => { | ||
const itInstance = it(`test createDomain '${t.name}'`, (done) => { | ||
const options = t.options; | ||
const server = new Server(compiler, options); | ||
const expected = t.expected; | ||
server.listen(options.port, options.host, (err) => { | ||
if (err) { | ||
done(err); | ||
} | ||
const generatedDomain = createDomain(options, server.listeningApp); | ||
if (generatedDomain !== expected) { | ||
done(`generated domain ${generatedDomain} doesn't match expected ${expected}`); | ||
} else { | ||
done(); | ||
} | ||
server.close(); | ||
}); | ||
}); | ||
if (t.timeout) { | ||
itInstance.timeout(t.timeout); | ||
} | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
sorry, but this is not what was requested in previous comments
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 wrote in the commit comment that
url.parse
is not appropriate here.. I did it withurl.parse
before but the new tests I provided showed it's not working as it should..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.
Sorry, but I looked up url.parse documentation again, as well as searching for issues with what I'm experiencing, and there's no easy solution possible by using url.parse.
Why? The
public
parameter can be a non-valid url like "localhost:8080" which is parsed to somethin like this:As you can see, the protocol is incorrect and I would need to do more processing to find out what's going on. I think the solution using regex is the better option here.
Also have a look at nodejs/node#5755, where they're discussing the same issue on parsing urls.
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.
great documentation of the why. that'll certainly be beneficial in the future