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

feat: improve host output and fix open #2892

Merged
merged 7 commits into from
Dec 2, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
91 changes: 84 additions & 7 deletions lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const url = require('url');
const http = require('http');
const https = require('https');
const ip = require('ip');
const internalIp = require('internal-ip');
const killable = require('killable');
const chokidar = require('chokidar');
const express = require('express');
Expand All @@ -21,8 +22,8 @@ const { validate } = require('schema-utils');
const normalizeOptions = require('./utils/normalizeOptions');
const updateCompiler = require('./utils/updateCompiler');
const getCertificate = require('./utils/getCertificate');
const status = require('./utils/status');
const createDomain = require('./utils/createDomain');
const colors = require('./utils/colors');
const runOpen = require('./utils/runOpen');
const runBonjour = require('./utils/runBonjour');
const routes = require('./utils/routes');
const getSocketServerImplementation = require('./utils/getSocketServerImplementation');
Expand Down Expand Up @@ -580,17 +581,93 @@ class Server {
}

showStatus() {
const suffix = '/';
const uri = `${createDomain(this.options, this.server)}${suffix}`;
const useColor = getColorsOption(getCompilerConfigArray(this.compiler));

const configArr = getCompilerConfigArray(this.compiler);
const colors = getColorsOption(configArr);
const protocol = this.options.https ? 'https' : 'http';
const { hostname, port } = this;
const prettyPrintUrl = (newHostname) =>
url.format({ protocol, hostname: newHostname, port, pathname: '/' });

let prettyHostname;
let lanUrlForTerminal;

if (hostname === '0.0.0.0' || hostname === '::') {
prettyHostname = 'localhost';

const localIP = internalIp.v4.sync();
Copy link
Contributor

@ylemkimon ylemkimon Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const localIP = internalIp.v4.sync();
const localIP = hostname === '::' ? internalIp.v6.sync() : internalIp.v4.sync();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is for terminal output, developer should copy past it from terminal, IPv6 is not supported in many browsers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url automatically formats IPv6 hostname with brackets, which most browsers support including IE 11, e.g., http://[::]:8080. I think IPv6 should take precendence over IPv4, if available.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense, I hope this does not scare developers


if (
/^10[.]|^172[.](1[6-9]|2[0-9]|3[0-1])[.]|^192[.]168[.]/.test(localIP)
) {
// Address is private, format it for later use
Copy link
Contributor

@ylemkimon ylemkimon Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check is not needed, as 0.0.0.0 (::) will bind to a public address, if available.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can have local public IP (it is very rare edge case, but it is possible), for example I can run it on my server with public IP

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I think the public IP should be printed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we print two IP - local and network

Copy link
Contributor

@ylemkimon ylemkimon Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, I see no reason not to print the internal IP (the IP address of the NIC) if it's a public one. It's still a network IP, just not a LAN.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have strong opinion here, it was inspired by CRA and their feedback, but maybe you are right

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's simplify it, we will change it in future after feedback

lanUrlForTerminal = prettyPrintUrl(localIP);
}
} else {
prettyHostname = hostname;
}

status(uri, this.options, this.logger, colors);
const localUrlForTerminal = prettyPrintUrl(prettyHostname);

if (lanUrlForTerminal) {
this.logger.info('Project is running at:');
this.logger.info(`Local: ${colors.info(useColor, localUrlForTerminal)}`);
this.logger.info(
`On Your Network: ${colors.info(useColor, lanUrlForTerminal)}`
);
} else {
this.logger.info(
`Project is running at ${colors.info(useColor, localUrlForTerminal)}`
);
}

if (
this.options.dev &&
typeof this.options.dev.publicPath !== 'undefined'
) {
this.options.info(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-akait looks like this.options.info should be replaced with this.logger.info

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already fixed in master

`webpack output is served from '${colors.info(
useColor,
this.options.dev.publicPath === 'auto'
? '/'
: this.options.dev.publicPath
)}' URL`
);
}

if (this.options.static && this.options.static.length > 0) {
this.logger.info(
`Content not from webpack is served from '${colors.info(
useColor,
this.options.static
.map((staticOption) => staticOption.directory)
.join(', ')
)}' directory`
);
}

if (this.options.historyApiFallback) {
this.logger.info(
`404s will fallback to '${colors.info(
useColor,
this.options.historyApiFallback.index || '/index.html'
)}'`
);
}

if (this.options.bonjour) {
this.logger.info(
'Broadcasting "http" with subtype of "webpack" via ZeroConf DNS (Bonjour)'
);
}

if (this.options.open) {
runOpen(localUrlForTerminal, this.options, this.logger);
}
}

listen(port, hostname, fn) {
this.hostname = hostname;

if (typeof port !== 'undefined' && port !== this.options.port) {
this.logger.warn(
'The port specified in options and the port passed as an argument is different.'
Expand Down
10 changes: 5 additions & 5 deletions lib/utils/createDomain.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,27 @@ function createDomain(options, server) {
// use location hostname and port by default in createSocketUrl
Copy link
Contributor

@ylemkimon ylemkimon Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIth showStatus() now using its own logic, this is only used in client entries. I think this can be refactored in the future PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think logic for terminal output and for entries is different, it is similar in some places, but it is for different purpose

// ipv6 detection is not required as 0.0.0.0 is just used as a placeholder
let hostname;

if (options.useLocalIp) {
hostname = ip.v4.sync() || '0.0.0.0';
} else if (server) {
hostname = server.address().address;
} else {
hostname = '0.0.0.0';
}

const port = server ? server.address().port : 0;

// 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}`;
}

// the formatted domain (url without path) of the webpack server
return url.format({
protocol,
hostname,
port,
});
return url.format({ protocol, hostname, port });
}

module.exports = createDomain;
47 changes: 0 additions & 47 deletions lib/utils/status.js

This file was deleted.

8 changes: 4 additions & 4 deletions test/cli/__snapshots__/cli.test.js.snap
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`CLI --hot webpack 4 1`] = `
"<i> [webpack-dev-server] Project is running at http://127.0.0.1:8080/
"<i> [webpack-dev-server] Project is running at http://localhost:8080/
<i> [webpack-dev-middleware] Hash: X
<i> Version: webpack x.x.x Time: Xms
<i> Built at: Thu Jan 01 1970 <CLR=BOLD>00:00:00</CLR> GMT
Expand All @@ -28,7 +28,7 @@ exports[`CLI --hot webpack 4 1`] = `
`;

exports[`CLI --hot webpack 5 1`] = `
Copy link
Contributor

@ylemkimon ylemkimon Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note snapshots should be updated twice, each for webpack 4 and 5.

"<i> [webpack-dev-server] Project is running at http://127.0.0.1:8080/
"<i> [webpack-dev-server] Project is running at http://localhost:8080/
<i> [webpack-dev-middleware] asset main.js X KiB [emitted] (name: main)
<i> runtime modules X KiB 10 modules
<i> cacheable modules X KiB
Expand All @@ -46,7 +46,7 @@ exports[`CLI --hot webpack 5 1`] = `
`;

exports[`CLI --no-hot webpack 4 1`] = `
"<i> [webpack-dev-server] Project is running at http://127.0.0.1:8080/
"<i> [webpack-dev-server] Project is running at http://localhost:8080/
<i> [webpack-dev-middleware] Hash: X
<i> Version: webpack x.x.x Time: Xms
<i> Built at: Thu Jan 01 1970 <CLR=BOLD>00:00:00</CLR> GMT
Expand All @@ -73,7 +73,7 @@ exports[`CLI --no-hot webpack 4 1`] = `
`;

exports[`CLI --no-hot webpack 5 1`] = `
"<i> [webpack-dev-server] Project is running at http://127.0.0.1:8080/
"<i> [webpack-dev-server] Project is running at http://localhost:8080/
<i> [webpack-dev-middleware] asset main.js X KiB [emitted] (name: main)
<i> runtime modules X bytes 3 modules
<i> cacheable modules X KiB
Expand Down
53 changes: 49 additions & 4 deletions test/cli/cli.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const { join, resolve } = require('path');
const execa = require('execa');
const internalIp = require('internal-ip');
const testBin = require('../helpers/test-bin');
const isWebpack5 = require('../helpers/isWebpack5');

Expand Down Expand Up @@ -118,12 +119,56 @@ runCLITest('CLI', () => {
.catch(done);
});

it('unspecified port', (done) => {
it('unspecified host and port', (done) => {
testBin('')
.then((output) => {
expect(/http:\/\/127\.0\.0\.1:[0-9]+/.test(output.stderr)).toEqual(
true
);
expect(/http:\/\/localhost:[0-9]+/.test(output.stderr)).toEqual(true);
done();
})
.catch(done);
});

it('--host 0.0.0.0 (IPv4)', (done) => {
testBin('--host 0.0.0.0')
.then((output) => {
const localIP = internalIp.v4.sync();

expect(/http:\/\/localhost:[0-9]+/.test(output.stderr)).toEqual(true);
expect(
new RegExp(`http://${localIP}:[0-9]+/`).test(output.stderr)
).toEqual(true);
done();
})
.catch(done);
});

it('--host :: (IPv6)', (done) => {
testBin('--host ::')
.then((output) => {
const localIP = internalIp.v4.sync();

expect(/http:\/\/localhost:[0-9]+/.test(output.stderr)).toEqual(true);
expect(
new RegExp(`http://${localIP}:[0-9]+/`).test(output.stderr)
).toEqual(true);
done();
})
.catch(done);
});

it('--host localhost', (done) => {
testBin('--host localhost')
.then((output) => {
expect(/http:\/\/localhost:[0-9]+/.test(output.stderr)).toEqual(true);
done();
})
.catch(done);
});

it('--port', (done) => {
testBin('--port 9999')
.then((output) => {
expect(/http:\/\/localhost:9999/.test(output.stderr)).toEqual(true);
done();
})
.catch(done);
Expand Down