Fix returning promise with passing options #172
Conversation
Signed-off-by: Matthew Peveler <[email protected]>
| } | ||
| if(options.stopPort < options.startPort) { | ||
| throw Error('Provided options.stopPort(' + options.stopPort + 'is less than options.startPort (' + options.startPort + ')'); | ||
| throw Error('Provided options.stopPort(' + options.stopPort + ') is less than options.startPort (' + options.startPort + ')'); |
There was a problem hiding this comment.
Unrelated to the fix, but something I noticed when writing up the tests.
| if (!callback) { | ||
| callback = options; | ||
| options = {}; | ||
| } |
There was a problem hiding this comment.
Removed these from the internals call as these methods are not expected to be called by external user, and so we should be confident that options and callback are passed correctly.
| const idx = exports._defaultHosts.indexOf(currentHost); | ||
| exports._defaultHosts.splice(idx, 1); | ||
| return exports.getPort(options, callback); | ||
| return internals.getPort(options, callback); |
There was a problem hiding this comment.
Replaced all of these calls with internals.getPort as I don't think it's worth going through the user facing function.
There was a problem hiding this comment.
This and your other comment https://github.com/http-party/node-portfinder/pull/172/files#r1994080054 made me realize we can fix the error message to provide the port that getPort failed on and also that we had a bad test b/c we were passing startPort through instead of port for the port 80 test which was passing b/c it tested that port finder returned a port > 80 - but it wasn't even using startPort, it was using the default port :)
I'll submit pr's and cc u
thanks for working on this stuff, this was a lot of fun tonight :)
| } | ||
| }); | ||
|
|
||
| test('with stopPort smaller than 3 available ports', function (done) { |
There was a problem hiding this comment.
Test does show a difference in behavior between the promise and callback. I think that the error message should maybe be different here, and should any ports be returned? 🤷
Out of scope of this PR though.
There was a problem hiding this comment.
I agree - sending in a pr now for this good call
| method(3, { stopPort: 32774 }, function (err, ports) { | ||
| expect(err).not.toBeNull(); | ||
| expect(err.message).toEqual('No open ports found in between 32768 and 32774'); | ||
| expect(ports).toEqual([32773, 32774, undefined]); |
There was a problem hiding this comment.
yep good call - I made an initial pr for this @ #178 - so that the message is better.
2 participants
PR fixes a bug introduced by #168 where when passing
optionsto thegetPortorgetPortsfunction and no callback, then the options object would be assigned to the callback, and the whole thing would break.As part of this, I've refactored the test suite so that all
getPortandgetPortstests are now called for the callback and promise variants of the functions.