Fix/port config precedence

.changeset/little-toys-greet.md

@@ -0,0 +1,5 @@
+---
+'preact-cli': patch
+---
+
+Changes port config precedence to: --port -> \$PORT -> 8080 (default)

packages/cli/lib/commands/watch.js

@@ -1,6 +1,7 @@
 const runWebpack = require('../lib/webpack/run-webpack');
-const { toBool, warn } = require('../util');
+const { isPortFree, toBool, warn } = require('../util');
 const { validateArgs } = require('./validate-args');
+const getPort = require('get-port');
 
 const options = [
 	{
@@ -88,8 +89,7 @@ const options = [
 	},
 	{
 		name: '-p, --port',
-		description: 'Set server port',
-		default: 8080,
+		description: 'Set server port (default 8080)',
 	},
 ];
 
@@ -105,6 +105,8 @@ async function command(src, argv) {
 		argv.sw = toBool(argv.sw);
 	}
 
+	argv.port = await determinePort(argv.port);
+
 	if (argv.https || process.env.HTTPS) {
 		let { key, cert, cacert } = argv;
 		if (key && cert) {
@@ -117,7 +119,23 @@ async function command(src, argv) {
 	return runWebpack(argv, true);
 }
 
+async function determinePort(port) {
+	port = parseInt(port, 10);
+	if (port) {
+		if (!(await isPortFree(port))) {
+			throw new Error(
+				`Another process is already running on port ${port}. Please choose a different port.`
+			);
+		}
+	} else {
+		port = await getPort({ port: parseInt(process.env.PORT, 10) || 8080 });
+	}
+
+	return port;
+}
+
 module.exports = {
 	command,
 	options,
+	determinePort,
 };

packages/cli/lib/lib/webpack/run-webpack.js

@@ -1,6 +1,5 @@
 const ip = require('ip');
 const webpack = require('webpack');
-const getPort = require('get-port');
 const { resolve } = require('path');
 const clear = require('console-clear');
 const { writeFile } = require('fs').promises;
@@ -12,15 +11,10 @@ const transformConfig = require('./transform-config');
 const { error, isDir, warn } = require('../../util');
 
 async function devBuild(env) {
-	let userPort = parseInt(process.env.PORT || env.port, 10) || 8080;
-	env.port = await getPort({ port: userPort });
-
 	let config = await clientConfig(env);
 
 	await transformConfig(env, config);
 
-	let port = config.devServer.port;
-
 	let compiler = webpack(config);
 	return new Promise((res, rej) => {
 		compiler.hooks.emit.tapAsync('CliDevPlugin', (compilation, callback) => {
@@ -46,22 +40,15 @@ async function devBuild(env) {
 			if (host === '0.0.0.0' && process.platform === 'win32') {
 				host = 'localhost';
 			}
-
-			let serverAddr = `${protocol}://${host}:${bold(port)}`;
-			let localIpAddr = `${protocol}://${ip.address()}:${bold(port)}`;
+			let serverAddr = `${protocol}://${host}:${bold(env.port)}`;
+			let localIpAddr = `${protocol}://${ip.address()}:${bold(env.port)}`;
 
 			if (env['clear']) clear(true);
 
 			if (stats.hasErrors()) {
 				process.stdout.write(red('Build failed!\n\n'));
 			} else {
 				process.stdout.write(green('Compiled successfully!\n\n'));
-
-				if (userPort !== port) {
-					process.stdout.write(
-						`Port ${bold(userPort)} is in use, using ${bold(port)} instead\n\n`
-					);
-				}
 				process.stdout.write('You can view the application in browser.\n\n');
 				process.stdout.write(`${bold('Local:')}            ${serverAddr}\n`);
 				process.stdout.write(`${bold('On Your Network:')}  ${localIpAddr}\n`);
@@ -77,7 +64,7 @@ async function devBuild(env) {
 		});
 
 		let server = new DevServer(compiler, c);
-		server.listen(port);
+		server.listen(env.port);
 		res(server);
 	});
 }

packages/cli/lib/util.js

@@ -3,6 +3,7 @@ const { normalize, resolve } = require('path');
 const { statSync, existsSync } = require('fs');
 const symbols = require('./symbols');
 const which = require('which');
+const net = require('net');
 
 exports.isDir = function (str) {
 	return existsSync(str) && statSync(str).isDirectory();
@@ -54,3 +55,26 @@ exports.normalizeTemplatesResponse = function (repos = []) {
 exports.toBool = function (val) {
 	return val === void 0 || (val === 'false' ? false : val);
 };
+
+/**
+ * Taken from: https://github.com/preactjs/wmr/blob/3401a9bfa6491d25108ad68688c067a7e17d0de5/packages/wmr/src/lib/net-utils.js#L4-Ll4
+ * Check if a port is free
+ * @param {number} port
+ * @returns {Promise<boolean>}
+ */
+exports.isPortFree = async function (port) {
+	try {
+		await new Promise((resolve, reject) => {
+			const server = net.createServer();
+			server.unref();
+			server.on('error', reject);
+			server.listen({ port }, () => {
+				server.close(resolve);
+			});
+		});
+		return true;
+	} catch (err) {
+		if (err.code !== 'EADDRINUSE') throw err;
+		return false;
+	}
+};

packages/cli/tests/watch.test.js

@@ -2,6 +2,7 @@ const { readFile, writeFile } = require('fs').promises;
 const { resolve } = require('path');
 const startChrome = require('./lib/chrome');
 const { create, watch } = require('./lib/cli');
+const { determinePort } = require('../lib/commands/watch');
 
 const { loadPage, waitUntilExpression } = startChrome;
 let chrome, server;
@@ -34,3 +35,59 @@ describe('preact', () => {
 		server.close();
 	});
 });
+
+describe('should determine the correct port', () => {
+	it('should prefer --port over $PORT', async () => {
+		process.env.PORT = '4000';
+		expect(await determinePort('3999')).toBe(3999);
+	});
+
+	it('should use $PORT in the abscence of --port', async () => {
+		process.env.PORT = '4001';
+		expect(await determinePort()).toBe(4001);
+	});
+
+	it('should use $PORT if --port is invalid', async () => {
+		process.env.PORT = '4002';
+		expect(await determinePort('invalid-port')).toBe(4002);
+	});
+
+	it('should use 8080 if $PORT and --port are invalid', async () => {
+		process.env.PORT = 'invalid-port-too';
+		expect(await determinePort('invalid-port')).toBe(8080);
+	});
+
+	it('should return an error if requested --port is taken', async () => {
+		await Promise.all([determinePort(4003), determinePort(4003)]).catch(
+			error => {
+				expect(error.message).toMatch(
+					new RegExp(
+						/^Another process is already running on port 4003. Please choose a different port./g
+					)
+				);
+			}
+		);
+	});
+
+	it('should fallback to random if $PORT or 8080 are taken and --port is not specified', async () => {
+		process.env.PORT = '4004';
+		await Promise.all([determinePort(), determinePort()]).then(values => {
+			expect(values[0]).toBe(4004);
+			expect(values[1]).toBeGreaterThanOrEqual(1024);
+			expect(values[1]).toBeLessThanOrEqual(65535);
+		});
+
+		// This is pretty awful, but would be the way to do it. get-port locks the port for ~30 seconds,
+		// so if we want to test any behavior with our default (8080) twice, we'd have to wait :/
+		//
+		//await sleep(35000);
+
+		//process.env.PORT = undefined;
+		//await Promise.all([determinePort(), determinePort()]).then(values => {
+		//	console.log(values);
+		//	expect(values[0]).toBe(8080);
+		//	expect(values[1]).toBeGreaterThanOrEqual(1024);
+		//	expect(values[1]).toBeLessThanOrEqual(65535);
+		//});
+	});
+});