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

allow explicitly setting the protocol from the public option #1117

Merged
merged 11 commits into from
Oct 8, 2017
20 changes: 19 additions & 1 deletion client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,25 @@ function reloadApp() {
self.postMessage('webpackHotUpdate' + currentHash, '*');
}
} else {
let rootWindow = self;
// use parent window for reload (in case we're in an iframe with no valid src)
const intervalId = self.setInterval(function findRootWindow() {
if (rootWindow.location.protocol !== 'about:') {
// reload immediately if protocol is valid
applyReload(rootWindow, intervalId);
} else {
rootWindow = rootWindow.parent;
if (rootWindow.parent === rootWindow) {
// if parent equals current window we've reached the root which would continue forever, so trigger a reload anyways
applyReload(rootWindow, intervalId);
}
}
});
}

function applyReload(rootWindow, intervalId) {
clearInterval(intervalId);
log.info('[WDS] App updated. Reloading...');
self.location.reload();
rootWindow.location.reload();
}
}
13 changes: 13 additions & 0 deletions examples/cli-public-protocol/README.md
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.
5 changes: 5 additions & 0 deletions examples/cli-public-protocol/app.js
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();
8 changes: 8 additions & 0 deletions examples/cli-public-protocol/index.html
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>
11 changes: 11 additions & 0 deletions examples/cli-public-protocol/webpack.config.js
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
}
};
6 changes: 5 additions & 1 deletion lib/util/createDomain.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@ module.exports = function createDomain(options, listeningApp) {
const port = options.socket ? 0 : appPort;
const hostname = options.useLocalIp ? internalIp.v4() : options.host;

// use explicitly defined public url (prefix with protocol if not explicitly given)
if (options.public) {
return /^[a-zA-Z]+:\/\//.test(options.public) ? `${options.public}` : `${protocol}://${options.public}`;
Copy link
Contributor

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

this is not a good way to check a url for a protocol. please use url.parse instead, and examine the result for a protocol.

Copy link
Contributor Author

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 with url.parse before but the new tests I provided showed it's not working as it should..

found a bug due to tests: url.parse doesn't properly parse a provided public option since it counts everything before the port colon as the protocol which is just wrong. I replaced it with a protocol regex.

Copy link
Contributor Author

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:

> url.parse("localhost:8080")
Url {
  protocol: 'localhost:',
  slashes: null,
  auth: null,
  host: '8080',
  port: null,
  hostname: '8080',
  hash: null,
  search: null,
  query: null,
  pathname: null,
  path: null,
  href: 'localhost:8080' }

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.

Copy link
Contributor

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

}
// the formatted domain (url without path) of the webpack server
return options.public ? `${protocol}://${options.public}` : url.format({
return url.format({
protocol,
hostname,
port
Expand Down
94 changes: 94 additions & 0 deletions test/Util.test.js
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);
}
});
});