fix #752: allow --port 0 again#918
Conversation
Codecov Report
@@ Coverage Diff @@
## master #918 +/- ##
======================================
Coverage 71.3% 71.3%
======================================
Files 4 4
Lines 453 453
Branches 133 133
======================================
Hits 323 323
Misses 130 130Continue to review full report at Codecov.
|
| 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) { |
There was a problem hiding this comment.
You should not add !=null here because it's not consistant with the current code style.
Should keep if(options.port)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
How about if(options.port || options.port === 0) ?
There was a problem hiding this comment.
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.
| return msg; | ||
| } | ||
|
|
||
| const defaultTo = (v, def) => v == null ? def : v; |
There was a problem hiding this comment.
no sense in obfuscating parameters here and one-letter variables make me stabby. please do change v to value.
4 participants
What kind of change does this PR introduce?
Bugfix for #752
Did you add or update the
examples/?Summary
Fixes the check for
options.portandargv.portto allow0. I rely on the operating system to assign a port so that it is guaranteed to be available and doesn't ever bind to the default8080. I can't update beyond v2.1.0-beta.10, because after #6850is treated the same as not passing the option and triggers portfinder.Does this PR introduce a breaking change?
It should only change behavior when
0is passed as the port option.Other information
The behavior when using port 0 could still be improved. This simply brings back the ability to specify port 0 which existed prior to v2.1.0-beta.11.