Skip to content

fix #752: allow --port 0 again #918

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.

Sign up for GitHub

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

Merged
shellscape merged 3 commits into from
Jun 14, 2017
Merged

fix #752: allow --port 0 again #918

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
85e698b
fix #752: don't rely on truthiness for port
also Jan 23, 2017
a443a16
Merge branch 'master' into port-zero
shellscape Jun 14, 2017
c9fddf7
better variable name
also Jun 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions bin/webpack-dev-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ function colorError(useColor, msg) {
return msg;
}

const defaultTo = (value, def) => value == null ? def : value;

const yargs = require("yargs")
.usage(`${versionInfo()
}\nUsage: https://webpack.js.org/configuration/dev-server/`);
Expand Down Expand Up @@ -317,8 +319,8 @@ function processOptions(wpOpt) {
// that wouldn't throw errors. E.g. both argv.port and options.port
// were specified, but since argv.port is 8080, options.port will be
// tried first instead.
options.port = argv.port === DEFAULT_PORT ? (options.port || argv.port) : (argv.port || options.port);
if(options.port) {
options.port = argv.port === DEFAULT_PORT ? defaultTo(options.port, argv.port) : defaultTo(argv.port, options.port);
if(options.port != null) {
Copy link
Copy Markdown

@hxuanhung hxuanhung Jun 6, 2017

Choose a reason for hiding this comment

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

You should not add !=null here because it's not consistant with the current code style.
Should keep if(options.port)

Copy link
Copy Markdown
Contributor Author

@also also Jun 6, 2017

Choose a reason for hiding this comment

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

if(options.port) is the source of the bug, because it excludes 0 by relying on "truthiness".

if(options.port != null) ensures that any options.port value except null or undefined is respected.

Would a more explicit check with triple-equals like if(options.port !== null && options.port !== undefined) be in line with the code style?

Copy link
Copy Markdown

@hxuanhung hxuanhung Jun 6, 2017

Choose a reason for hiding this comment

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

How about if(options.port || options.port === 0) ?

Copy link
Copy Markdown

@Phoenixmatrix Phoenixmatrix Jun 6, 2017

Choose a reason for hiding this comment

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

It makes sense to me that if you're doing a truthy check, you do it as per your code style...but in this case, you specifically want "if not null and not undefined", for which the most idiomatic way is != null.

Why obfuscate the code? This isn't about stylistic (I'm all about style consistency), it's essentially about correctness in this very particular scenario.

Copy link
Copy Markdown
Contributor

@shellscape shellscape Jun 14, 2017

Choose a reason for hiding this comment

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

I'm in agreement with @Phoenixmatrix and @also

startDevServer(wpOpt, options);
return;
}
Expand Down