Skip to content

Commit df6170c

Browse files
refactor: centralize server port resolution (#4198)
I took a look at the last remaining FIXME comment in the code - the one in `js/app.js` pointing to PR #673. It's from 2017 and was already flagged as a hotfix back then. The problem: `MM_PORT` got written into a global `global.mmPort` that was then read inconsistently across the code - sometimes via the global, sometimes straight from `process.env`, plus separate logic in `defaults.js`. Three ways to get the same port. I pulled this into a single `getServerPort()` helper (`MM_PORT` -> `config.port` -> `8080`) and switched `app.js`, `defaults.js`, `server.js`, `electron.js` and both serveronly files over to it. The global is gone now. Basically the behavior should be unchanged.
1 parent 0bb9b5f commit df6170c

7 files changed

Lines changed: 21 additions & 22 deletions

File tree

js/app.js

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,12 +42,6 @@ if (process.env.MM_CONFIG_FILE) {
4242
global.configuration_file = process.env.MM_CONFIG_FILE.replace(`${global.root_path}/`, "");
4343
}
4444

45-
// FIXME: Hotfix Pull Request
46-
// https://github.com/MagicMirrorOrg/MagicMirror/pull/673
47-
if (process.env.MM_PORT) {
48-
global.mmPort = process.env.MM_PORT;
49-
}
50-
5145
// The next part is here to prevent a major exception when there
5246
// is no internet connection. This could probable be solved better.
5347
process.on("uncaughtException", function (err) {

js/defaults.js

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,6 @@
1-
/* global mmPort */
2-
3-
const address = "localhost";
4-
let port = 8080;
5-
if (typeof mmPort !== "undefined") {
6-
port = mmPort;
7-
}
81
const defaults = {
9-
address: address,
10-
port: port,
2+
address: "localhost",
3+
port: 8080,
114
basePath: "/",
125
useHttps: false, // Support HTTPS or not, default "false" will use HTTP
136
httpsPrivateKey: "", // HTTPS private key path, only required when useHttps is true

js/electron.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const electron = require("electron");
44
const core = require("./app");
55
const Log = require("./logger");
66
const { applyElectronSwitches } = require("./electron_helper");
7+
const { getServerPort } = require("#server_functions");
78

89
// Config
910
let config = process.env.config ? JSON.parse(process.env.config) : {};
@@ -101,7 +102,7 @@ function createWindow () {
101102
}
102103

103104
const address = (config.address === void 0) | (config.address === "") | (config.address === "0.0.0.0") | (config.address === "::") ? (config.address = "localhost") : config.address;
104-
const port = process.env.MM_PORT || config.port;
105+
const port = getServerPort(config);
105106
mainWindow.loadURL(`${prefix}${address}:${port}`);
106107

107108
// Open the DevTools if run with "node --run start:dev"

js/server.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ const { ipAccessControl, socketIpAccessControl } = require("./ip_access_control"
1111

1212
const vendor = require("./vendor");
1313

14-
const { getHtml, getVersion, getEnvVars, cors } = require("#server_functions");
14+
const { getHtml, getVersion, getEnvVars, getServerPort, cors } = require("#server_functions");
1515

1616
/**
1717
* Server
@@ -21,7 +21,7 @@ const { getHtml, getVersion, getEnvVars, cors } = require("#server_functions");
2121
function Server (configObj) {
2222
const config = configObj.fullConf;
2323
const app = express();
24-
const port = process.env.MM_PORT || config.port;
24+
const port = getServerPort(config);
2525
const serverSockets = new Set();
2626
let server = null;
2727

js/server_functions.js

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,16 @@ function getEnvVars (req, res) {
251251
res.send(obj);
252252
}
253253

254+
/**
255+
* Resolves the HTTP server port. The `MM_PORT` environment variable takes
256+
* precedence over the configured port, falling back to 8080.
257+
* @param {object} [config] the configuration to read the port from (defaults to global.config)
258+
* @returns {number} the port the server should listen on
259+
*/
260+
function getServerPort (config = global.config) {
261+
return Number(process.env.MM_PORT || config?.port || 8080);
262+
}
263+
254264
/**
255265
* Get the config file path from environment or default location
256266
* @returns {string} The absolute config file path
@@ -269,4 +279,4 @@ function getConfigFilePath () {
269279
return path.resolve(global.configuration_file || `${global.root_path}/config/config.js`);
270280
}
271281

272-
module.exports = { cors, getHtml, getVersion, getStartup, getEnvVars, getEnvVarsAsObj, getUserAgent, getConfigFilePath, replaceSecretPlaceholder };
282+
module.exports = { cors, getHtml, getVersion, getStartup, getEnvVars, getEnvVarsAsObj, getUserAgent, getServerPort, getConfigFilePath, replaceSecretPlaceholder };

serveronly/index.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
const app = require("../js/app");
22
const Log = require("../js/logger");
3+
const { getServerPort } = require("#server_functions");
34

45
app.start().then((config) => {
56
const bindAddress = config.address ? config.address : "localhost";
67
const httpType = config.useHttps ? "https" : "http";
7-
Log.info(`\n>>> Ready to go! Please point your browser to: ${httpType}://${bindAddress}:${global.mmPort || config.port} <<<`);
8+
Log.info(`\n>>> Ready to go! Please point your browser to: ${httpType}://${bindAddress}:${getServerPort(config)} <<<`);
89
});

serveronly/watcher.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ const path = require("node:path");
77
const net = require("node:net");
88
const http = require("node:http");
99
const Log = require("logger");
10-
const { getConfigFilePath } = require("#server_functions");
10+
const { getConfigFilePath, getServerPort } = require("#server_functions");
1111

1212
const RESTART_DELAY_MS = 500;
1313
const PORT_CHECK_MAX_ATTEMPTS = 20;
@@ -32,7 +32,7 @@ function getServerConfig () {
3232
delete require.cache[require.resolve(configPath)];
3333
const config = require(configPath);
3434
serverConfig = {
35-
port: global.mmPort || config.port || 8080,
35+
port: getServerPort(config),
3636
address: config.address || "localhost"
3737
};
3838
} catch {

0 commit comments

Comments
 (0)