Skip to content

Fix returning promise with passing options #172

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
merged 1 commit into from
Mar 14, 2025
Merged

Fix returning promise with passing options #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 10 additions & 16 deletions lib/portfinder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,6 @@ exports.setBasePath = function (path) {
};

internals.getPort = function (options, callback) {
if (!callback) {
callback = options;
options = {};
}
Comment on lines -127 to -130
Copy link
Contributor Author

@MasterOdin MasterOdin Mar 13, 2025

Choose a reason for hiding this comment

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

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.


options.port = Number(options.port) || Number(exports.basePort);
options.host = options.host || null;
options.stopPort = Number(options.stopPort) || Number(exports.highestPort);
Expand All @@ -139,7 +134,7 @@ internals.getPort = function (options, callback) {
throw Error('Provided options.startPort(' + options.startPort + ') is less than 0, which are cannot be bound.');
}
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 + ')');
Copy link
Contributor Author

@MasterOdin MasterOdin Mar 13, 2025

Choose a reason for hiding this comment

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

Unrelated to the fix, but something I noticed when writing up the tests.

}
}

Expand Down Expand Up @@ -182,7 +177,7 @@ internals.getPort = function (options, callback) {
} else {
const idx = exports._defaultHosts.indexOf(currentHost);
exports._defaultHosts.splice(idx, 1);
return exports.getPort(options, callback);
return internals.getPort(options, callback);
Copy link
Contributor Author

@MasterOdin MasterOdin Mar 13, 2025

Choose a reason for hiding this comment

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

Replaced all of these calls with internals.getPort as I don't think it's worth going through the user facing function.

Copy link
Member

@eriktrom eriktrom Mar 14, 2025

Choose a reason for hiding this comment

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

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 :)

}
} else {
// error is not accounted for, file ticket, handle special case
Expand All @@ -208,7 +203,7 @@ internals.getPort = function (options, callback) {
}
} else {
// otherwise, try again, using sorted port, aka, highest open for >= 1 host
return exports.getPort({ port: openPorts.pop(), host: options.host, startPort: options.startPort, stopPort: options.stopPort }, callback);
return internals.getPort({ port: openPorts.pop(), host: options.host, startPort: options.startPort, stopPort: options.stopPort }, callback);
}

});
Expand All @@ -220,11 +215,13 @@ internals.getPort = function (options, callback) {
// Responds with a unbound port on the current machine.
//
exports.getPort = function (options, callback) {
if (!callback) {
if (typeof options === 'function') {
callback = options;
options = {};
}

options = options || {};

if (!callback) {
return new Promise(function (resolve, reject) {
internals.getPort(options, function (err, port) {
Expand All @@ -247,18 +244,13 @@ exports.getPort = function (options, callback) {
exports.getPortPromise = exports.getPort;

internals.getPorts = function (count, options, callback) {
if (!callback) {
callback = options;
options = {};
}

let lastPort = null;
_async.timesSeries(count, function(index, asyncCallback) {
if (lastPort) {
options.port = exports.nextPort(lastPort);
}

exports.getPort(options, function (err, port) {
internals.getPort(options, function (err, port) {
if (err) {
asyncCallback(err);
} else {
Expand All @@ -277,11 +269,13 @@ internals.getPorts = function (count, options, callback) {
// Responds with an array of unbound ports on the current machine.
//
exports.getPorts = function (count, options, callback) {
if (!callback) {
if (typeof options === 'function') {
callback = options;
options = {};
}

options = options || {};

if (!callback) {
return new Promise(function(resolve, reject) {
internals.getPorts(count, options, function(err, ports) {
Expand Down
97 changes: 73 additions & 24 deletions test/port-finder-multiple.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,35 +22,84 @@ describe('with 5 existing servers', function () {
helper.stopServers(servers, done);
});

test('the getPorts() method with an argument of 3 should respond with the first three available ports (32773, 32774, 32775)', function (done) {
portfinder.getPorts(3, function (err, ports) {
expect(err).toBeNull();
expect(ports).toEqual([32773, 32774, 32775]);
done();
describe.each([
['getPorts()', false, portfinder.getPorts],
['getPorts()', true, portfinder.getPorts],
['getPortsPromise()', true, portfinder.getPortsPromise],
])(`the %s method (promise: %p)`, function (name, isPromise, method) {
test('with an argument of 3 should respond with the first three available ports (32773, 32774, 32775)', function (done) {
if (isPromise) {
method(3)
.then(function (ports) {
expect(ports).toEqual([32773, 32774, 32775]);
done();
})
.catch(function (err) {
done(err);
});
} else {
method(3, function (err, ports) {
if (err) {
done(err);
return;
}
expect(err).toBeNull();
expect(ports).toEqual([32773, 32774, 32775]);
done();
});
}
});

test('with stopPort smaller than 3 available ports', function (done) {
Copy link
Contributor Author

@MasterOdin MasterOdin Mar 13, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@eriktrom eriktrom Mar 14, 2025

Choose a reason for hiding this comment

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

I agree - sending in a pr now for this good call

if (isPromise) {
method(3, { stopPort: 32774 })
.then(function () {
done('Expected error to be thrown');
})
.catch(function (err) {
expect(err).not.toBeNull();
expect(err.message).toEqual('No open ports found in between 32768 and 32774');
done();
});
} else {
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]);
Copy link
Member

@eriktrom eriktrom Mar 14, 2025

Choose a reason for hiding this comment

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

yep good call - I made an initial pr for this @ #178 - so that the message is better.

done();
});
}
});
});
});

describe('with no existing servers', function () {
test('the getPorts() method with an argument of 3 should respond with the first three available ports (32768, 32769, 32770)', function (done) {
portfinder.getPorts(3, function (err, ports) {
expect(err).toBeNull();
expect(ports).toEqual([32768, 32769, 32770]);
done();
describe.each([
['getPorts()', false, portfinder.getPorts],
['getPorts()', true, portfinder.getPorts],
['getPortsPromise()', true, portfinder.getPortsPromise],
])(`the %s method (promise: %p)`, function (name, isPromise, method) {
test('with an argument of 3 should respond with the first three available ports (32768, 32769, 32770)', function (done) {
if (isPromise) {
method(3)
.then(function (ports) {
expect(ports).toEqual([32768, 32769, 32770]);
done();
})
.catch(function (err) {
done(err);
});
} else {
method(3, function (err, ports) {
if (err) {
done(err);
return;
}
expect(err).toBeNull();
expect(ports).toEqual([32768, 32769, 32770]);
done();
});
}
});
});

test.each([
['getPorts()', portfinder.getPorts],
['getPortsPromise()', portfinder.getPortsPromise],
])('the %s promise method with an argument of 3 should respond with the first three available ports (32768, 32769, 32770)', function (name, method, done) {
method(3)
.then(function (ports) {
expect(ports).toEqual([32768, 32769, 32770]);
done();
})
.catch(function (err) {
done(err);
});
});
});
Loading