Support array of preferred ports #21

nagen010 · 2018-05-04T13:08:34Z

I have added new key ports to options that accept a list of preferred port numbers. Updated tests and documentation. It closes #19. Please review and let me know if I need to update something. Thanks.

As the limitation has been submitted through another brach

supports array of preferred ports

Array.prototype.includes is not supported on Node js versions <= 4

nagen010 · 2018-05-07T14:06:15Z

@sindresorhus Please review and let me know if it is okay :)

sindresorhus · 2018-05-08T13:14:39Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "get-port",
-  "version": "3.2.0",
+  "version": "3.3.0",


Don't bump versions in a PR. It's up to the maintainer to do this.

sindresorhus · 2018-05-08T13:15:27Z

readme.md

@@ -47,6 +55,12 @@ Type: `number`

 The preferred port to use.

+##### ports 


I don't think it should be a separate option. Just let the port option accept either a number or array of numbers.

Sure. I will just use the port option to handle both inputs

sindresorhus · 2018-05-08T13:17:07Z

index.js

+
+	server.unref();
+	server.on('error', () => {
+		callback(false);


This is an unintended change. Before it would reject the promise with an error:

get-port/index.js

Line 16 in 7644e85

server.on('error', reject);

sindresorhus · 2018-05-08T13:17:49Z

index.js

@@ -1,6 +1,22 @@
 'use strict';
 const net = require('net');

+const isAvailable = (options, callback) => {


This should be kept a promise, so it would be exactly like

get-port/index.js

Lines 4 to 24 in 7644e85

const getPort = options => new Promise((resolve, reject) => {

// For backwards compatibility with number-only input

// TODO: Remove this in the next major version

if (typeof options === 'number') {

options = {

port: options

};

}

const server = net.createServer();

server.unref();

server.on('error', reject);

server.listen(options, () => {

const port = server.address().port;

server.close(() => {

resolve(port);

});

});

});

Sure. I will keep the promise.

sindresorhus · 2018-05-08T13:19:19Z

index.js

-			resolve(port);
+	if ('ports' in options) {
+		options.ports.forEach((e, index) => {
+			const input = getOptions(e, options.host);


You can't iterate like this. isAvailable is async, so if item 2 takes longer than item 3, you might get it resolved with item 3 even though item 2 is eventually available.

nagen010 · 2018-05-15T00:35:48Z

@sindresorhus xo latest version v0.21.0 dropped support for Node.js 4 https://github.com/xojs/xo/releases/tag/v0.21.0. Shall I drop Node.js 4 support?

Dropping Node.js 4 support as xo doesn't support it anymore.

sindresorhus · 2018-05-15T15:17:45Z

index.js

+			.then(port => resolve(port))
+			.catch(() => reject());
+	} else {
+		isAvailable(options)


This is not using getOptions

Removed if-else block.

sindresorhus · 2018-05-15T15:19:21Z

index.js

-			resolve(port);
-		});
-	});
+	if (Array.isArray(options.port)) {


No need to have different logic for array and number, just put the number in an array if it's not.

sindresorhus · 2018-05-15T15:20:04Z

index.js

+		});
+	});
+});
+
 const getPort = options => new Promise((resolve, reject) => {


No need to wrap it in new Promise. You can just return the promise instead.

https://twitter.com/sindresorhus/status/989804425584627712

@sindresorhus It looks like util.promisify function was included only in NodeJS version 8.

Use https://github.com/sindresorhus/pify

// @sindresorhus sorry. I am not sure how to use pify here. Could you please help me with some examples? Thanks.

The following should do:

const getPort = pify((options, callback) => { ... });

You will have to adapt the resolve and reject logic accordingly.

@nagen010 what have you tried so far with pify?

sindresorhus · 2018-05-15T15:20:17Z

index.js

 });

+function getOptions(portnumber, hostname) {


portnumber => portNumber

sindresorhus · 2018-05-15T15:22:38Z

index.js

+						return port;
+					})
+					.catch(() => {
+						return new Promise((resolve, reject) => reject());


No need to wrap it in new Promise.

sindresorhus · 2018-05-15T15:23:35Z

Shall I drop Node.js 4 support?

That should be done separately from this PR.

alexander-akait · 2018-05-18T11:38:26Z

/cc @nagen010 wait this for webpack-serve (next modular and plugable webpack dev server), thanks for work!

nagen010 · 2018-05-19T22:13:13Z

@evilebottnawi if I understand correctly, I think there is no dependency to webpack-serve right?

alexander-akait · 2018-05-21T09:57:19Z

@nagen010 https://github.com/webpack-contrib/webpack-serve/blob/master/package.json#L39

alexander-akait · 2018-05-24T14:31:59Z

/cc @nagen010 what is status?

nagen010 · 2018-05-25T02:43:54Z

/cc @evilebottnawi Sorry. I thought you wanted me to wait. It is almost done, I will submit it by tomorrow.

alexander-akait · 2018-05-28T15:29:45Z

/cc @sindresorhus will be great if you find time on review 👍

acostalima · 2018-07-13T16:42:45Z

index.js

+	options.port.reduce((seq, port) => {
+		return seq.catch(() => {
+			const input = getOptions(port, options.host);
+			return isAvailable(input)


Or simply:

return isAvailable(input).then(port => port).catch(Promise.reject.bind(Promise))

acostalima · 2018-07-13T16:46:10Z

index.js

+				});
+		});
+	}, Promise.reject())
+		.then(port => resolve(port))


Or simply: .then(resolve).catch(reject)

Thanks for the feedback!

acostalima · 2018-07-13T17:29:29Z

index.js

+		.catch(() => reject());
+});
+
+function getOptions(portNumber, hostname) {


Or simply:

function getOptions(port, host) { return { port, host }; }

which makes me wonder if this function is really that necessary.

acostalima · 2018-07-18T23:09:23Z

index.js

+	}, Promise.reject()).then(resolve).catch(reject);
+});
+
+function getOptions(port, host) {


Don't you think this function can be removed after all?

The following will do just fine:

if (typeof options === 'number') { options = { port: options }; }

and

isAvailable(Object.assign({}, options, { port }));

With Object.assign, you retain the previously implemented behavior which is the ability to eventually pass more options to server.listen.

nagen010 added 8 commits May 2, 2018 22:19

Update README.md with limitations

b81421a

Update README.md with port availability limitation

68cc4b5

accept port as an array

65df48b

support preferred ports array

c771754

minor edits to README

e2ac69b

Remove Limitation in Readme.md

97fe639

As the limitation has been submitted through another brach

update minor version

2c911e3

supports array of preferred ports

remove Array.includes() in test

11b3f68

Array.prototype.includes is not supported on Node js versions <= 4

sindresorhus requested changes May 8, 2018

View reviewed changes

nagen010 added 3 commits May 10, 2018 00:33

update port to support array

d584354

enable port destructuring

57c876d

Update documentation

50fa4d1

drop Node.js 4 support

225fca3

Dropping Node.js 4 support as xo doesn't support it anymore.

sindresorhus requested changes May 15, 2018

View reviewed changes

Make port accepts number or array input

785095a

acostalima reviewed Jul 13, 2018

View reviewed changes

update the promises

c8e3194

acostalima reviewed Jul 18, 2018

View reviewed changes

nagen010 and others added 2 commits July 23, 2018 19:31

remove getOptions

78ae021

Update readme.md

cce4b2e

sindresorhus approved these changes Jul 24, 2018

View reviewed changes

sindresorhus merged commit f5c9753 into sindresorhus:master Jul 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support array of preferred ports #21

Support array of preferred ports #21

nagen010 commented May 4, 2018 •

edited

nagen010 commented May 7, 2018

sindresorhus May 8, 2018

sindresorhus May 8, 2018

nagen010 May 9, 2018

sindresorhus May 8, 2018

sindresorhus May 8, 2018

nagen010 May 9, 2018

sindresorhus May 8, 2018

nagen010 commented May 15, 2018

sindresorhus May 15, 2018

nagen010 May 15, 2018

sindresorhus May 15, 2018

sindresorhus May 15, 2018

nagen010 May 15, 2018

sindresorhus May 18, 2018

nagen010 May 25, 2018

acostalima Jul 13, 2018 •

edited

acostalima Jul 18, 2018

sindresorhus May 15, 2018

sindresorhus May 15, 2018

sindresorhus commented May 15, 2018 •

edited

alexander-akait commented May 18, 2018 •

edited

nagen010 commented May 19, 2018

alexander-akait commented May 21, 2018

alexander-akait commented May 24, 2018

nagen010 commented May 25, 2018

alexander-akait commented May 28, 2018

acostalima Jul 13, 2018

acostalima Jul 13, 2018

nagen010 Jul 18, 2018

acostalima Jul 13, 2018

acostalima Jul 18, 2018 •

edited

		@@ -47,6 +55,12 @@ Type: `number`

		The preferred port to use.

		##### ports

	const getPort = options => new Promise((resolve, reject) => {
	// For backwards compatibility with number-only input
	// TODO: Remove this in the next major version
	if (typeof options === 'number') {
	options = {
	port: options
	};
	}

	const server = net.createServer();

	server.unref();
	server.on('error', reject);

	server.listen(options, () => {
	const port = server.address().port;
	server.close(() => {
	resolve(port);
	});
	});
	});

Support array of preferred ports #21

Support array of preferred ports #21

Conversation

nagen010 commented May 4, 2018 • edited

nagen010 commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagen010 commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acostalima Jul 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented May 15, 2018 • edited

alexander-akait commented May 18, 2018 • edited

nagen010 commented May 19, 2018

alexander-akait commented May 21, 2018

alexander-akait commented May 24, 2018

nagen010 commented May 25, 2018

alexander-akait commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acostalima Jul 18, 2018 • edited

Choose a reason for hiding this comment

nagen010 commented May 4, 2018 •

edited

acostalima Jul 13, 2018 •

edited

sindresorhus commented May 15, 2018 •

edited

alexander-akait commented May 18, 2018 •

edited

acostalima Jul 18, 2018 •

edited