refactor(utils): simplify configuration validation and error handling

js/app.js

@@ -255,10 +255,11 @@ function App () {
 			Log.log("Sockets connected & modules started ...");
 
 			return global.config;
-		} catch (err) {
-			// planned ConfigErrors already logged their message before throwing
-			if (!(err instanceof ConfigError)) {
-				Log.error("Unexpected error during startup:", err);
+		} catch (error) {
+			if (error instanceof ConfigError) {
+				Log.error(error.message);
+			} else {
+				Log.error("Unexpected error during startup:", error);
 			}
 
 			const int32 = new Int32Array(new SharedArrayBuffer(4));

js/check_config.js

@@ -10,7 +10,11 @@ const Utils = require(`${rootPath}/js/utils.js`);
 try {
 	Utils.checkConfigFile();
 } catch (error) {
-	const message = error && error.message ? error.message : error;
-	Log.error(`Unexpected error: ${message}`);
+	if (error instanceof Utils.ConfigError) {
+		Log.error(error.message);
+	} else {
+		const message = error && error.message ? error.message : error;
+		Log.error(`Unexpected error: ${message}`);
+	}
 	process.exit(1);
 }

js/utils.js

@@ -21,23 +21,30 @@ class ConfigError extends Error {
 	}
 }
 
+/**
+ * Executes CommonJS source code from a string and returns its exports.
+ * @param {string} src - JavaScript source code.
+ * @returns {object} The exported module value.
+ */
 const requireFromString = (src) => {
 	const m = new module.constructor();
 	m._compile(src, "");
 	return m.exports;
 };
 
-// return all available module positions
-const getAvailableModulePositions = () => {
-	return modulePositions;
-};
-
-// return if position is on modulePositions Array (true/false)
+/**
+ * Checks whether the provided module position exists.
+ * @param {string} position - Candidate module position.
+ * @returns {boolean} True when the position is known.
+ */
 const moduleHasValidPosition = (position) => {
-	if (getAvailableModulePositions().indexOf(position) === -1) return false;
-	return true;
+	return getModulePositions().includes(position);
 };
 
+/**
+ * Discovers module positions from index.html and caches them.
+ * @returns {Array<string>} Discovered module positions.
+ */
 const getModulePositions = () => {
 	// if not already discovered
 	if (modulePositions.length === 0) {
@@ -172,34 +179,25 @@ const loadConfig = () => {
 		return configObj;
 
 	} catch (error) {
+		let errorMessage = `Cannot access config file: ${configFilename}\n${error.message}`;
 		if (error.code === "ENOENT") {
-			Log.error(`Could not find config file: ${configFilename}`);
+			errorMessage = `Could not find config file: ${configFilename}`;
 		} else if (error.code === "EACCES") {
-			Log.error(`No permission to read config file: ${configFilename}`);
-		} else {
-			Log.error(`Cannot access config file: ${configFilename}\n${error.message}`);
+			errorMessage = `No permission to read config file: ${configFilename}`;
 		}
-		throw new ConfigError("");
+		throw new ConfigError(errorMessage);
 	}
 };
 
 /**
- * Checks the config file using eslint.
- * @param {object} configObject the configuration object
+ * Runs the ESLint checks configured for the config file.
+ * @param {string} configFileName - The filename shown in lint messages.
+ * @param {string} configFileContent - The config file content to lint.
+ * @returns {Array<object>} ESLint messages for linting problems.
  */
-const checkConfigFile = (configObject) => {
-	let configObj = configObject;
-	if (!configObj) configObj = loadConfig();
-	const configFileName = configObj.configFilename;
-
-	// Validate syntax of the configuration file.
-	Log.info(`Checking config file ${configFileName} ...`);
-
-	// I'm not sure if all ever is utf-8
-	const configFile = configObj.configContentFull;
-
-	const errors = linter.verify(
-		configFile,
+const lintConfigFile = (configFileName, configFileContent) => {
+	return linter.verify(
+		configFileContent,
 		{
 			languageOptions: {
 				ecmaVersion: "latest",
@@ -215,19 +213,21 @@ const checkConfigFile = (configObject) => {
 		},
 		configFileName
 	);
+};
 
-	if (errors.length === 0) {
-		Log.info(styleText("green", "Your configuration file doesn't contain syntax errors :)"));
-		validateModulePositions(configObj.fullConf);
-	} else {
-		let errorMessage = "Your configuration file contains syntax errors :(";
+/**
+ * Formats ESLint messages for the config syntax error output.
+ * @param {Array<object>} errors - ESLint messages returned by `linter.verify`.
+ * @returns {string} A user-facing error message.
+ */
+const formatConfigSyntaxErrors = (errors) => {
+	let errorMessage = "Your configuration file contains syntax errors :(";
 
-		for (const error of errors) {
-			errorMessage += `\nLine ${error.line} column ${error.column}: ${error.message}`;
-		}
-		Log.error(errorMessage);
-		throw new ConfigError("");
+	for (const error of errors) {
+		errorMessage += `\nLine ${error.line} column ${error.column}: ${error.message}`;
 	}
+
+	return errorMessage;
 };
 
 /**
@@ -237,8 +237,10 @@ const checkConfigFile = (configObject) => {
  *  - every entry has a `module` property of type string
  *  - every entry's `position` (if set) is a known region from index.html
  *
- * Unknown positions produce a warning; structural errors are fatal.
+ * Unknown positions produce a warning; structural errors throw an error so the
+ * outer validation flow can decide how to report and terminate.
  * @param {object} data - The full config object to validate.
+ * @throws {ConfigError} When the modules structure is invalid.
  */
 const validateModulePositions = (data) => {
 	Log.info("Checking modules structure configuration ...");
@@ -247,28 +249,24 @@ const validateModulePositions = (data) => {
 
 	// `modules` always exists (defaults.js provides a default array), but guard against it being overridden with a non-array value
 	if (data.modules !== undefined && !Array.isArray(data.modules)) {
-		Log.error("This module configuration contains errors:\nmodules must be an array");
-		throw new ConfigError("");
+		throw new ConfigError("This module configuration contains errors:\nmodules must be an array");
 	}
 
 	// Validate each module entry
 	for (const [index, mod] of (data.modules ?? []).entries()) {
 		// Each module entry must be an object so we can safely inspect its fields
 		if (mod === null || typeof mod !== "object" || Array.isArray(mod)) {
-			Log.error(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nmodule entry must be an object`);
-			throw new ConfigError("");
+			throw new ConfigError(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nmodule entry must be an object`);
 		}
 
 		// `module` (the module name) is required and must be a string
 		if (typeof mod.module !== "string") {
-			Log.error(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nmodule: must be a string`);
-			throw new ConfigError("");
+			throw new ConfigError(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nmodule: must be a string`);
 		}
 
 		// `position` is optional, but must be a string when provided
 		if (mod.position !== undefined && typeof mod.position !== "string") {
-			Log.error(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nposition: must be a string`);
-			throw new ConfigError("");
+			throw new ConfigError(`This module configuration contains errors:\n${JSON.stringify(mod, null, 2)}\nposition: must be a string`);
 		}
 
 		// `position` is optional, but when set it must match a known region
@@ -281,4 +279,30 @@ const validateModulePositions = (data) => {
 	Log.info(styleText("green", "Your modules structure configuration doesn't contain errors :)"));
 };
 
-module.exports = { loadConfig, getModulePositions, moduleHasValidPosition, getAvailableModulePositions, checkConfigFile, ConfigError };
+/**
+ * Checks the config file by orchestrating syntax and structure validation.
+ *
+ * Syntax errors and invalid module structures are fatal. Unknown module
+ * positions remain warnings only.
+ * @param {object} [configObject] - The loaded config object. When omitted, the config is loaded first.
+ * @throws {ConfigError} When the configuration contains fatal errors.
+ */
+const checkConfigFile = (configObject) => {
+	let configObj = configObject;
+	if (!configObj) configObj = loadConfig();
+	const configFileName = configObj.configFilename;
+
+	// Validate syntax of the configuration file first.
+	Log.info(`Checking config file ${configFileName} ...`);
+
+	const errors = lintConfigFile(configFileName, configObj.configContentFull);
+
+	if (errors.length === 0) {
+		Log.info(styleText("green", "Your configuration file doesn't contain syntax errors :)"));
+		validateModulePositions(configObj.fullConf);
+	} else {
+		throw new ConfigError(formatConfigSyntaxErrors(errors));
+	}
+};
+
+module.exports = { loadConfig, getModulePositions, moduleHasValidPosition, checkConfigFile, ConfigError };

tests/unit/classes/utils_spec.js

@@ -3,24 +3,21 @@ const fs = require("node:fs");
 const Log = require("../../../js/logger");
 const { checkConfigFile, ConfigError } = require("../../../js/utils");
 
-const createConfigObject = (modules) => ({
+const createConfigObject = (modules, configContentFull = "module.exports = { modules: [] };") => ({
 	configFilename: "config.js",
-	configContentFull: "module.exports = { modules: [] };",
+	configContentFull,
 	fullConf: { modules }
 });
 
-const runCheck = (modules) => {
-	checkConfigFile(createConfigObject(modules));
+const runCheck = (modules, configContentFull) => {
+	checkConfigFile(createConfigObject(modules, configContentFull));
 };
 
-const expectExitForModules = (modules) => {
-	vi.spyOn(process, "exit").mockImplementation(() => {
-		throw new ConfigError("");
-	});
-
+const expectConfigErrorForModules = (modules) => {
 	expect(() => {
 		runCheck(modules);
 	}).toThrow(ConfigError);
+	expect(process.exit).not.toHaveBeenCalled();
 };
 
 describe("utils", () => {
@@ -41,6 +38,7 @@ describe("utils", () => {
 		vi.spyOn(Log, "info").mockImplementation(() => {});
 		vi.spyOn(Log, "warn").mockImplementation(() => {});
 		vi.spyOn(Log, "error").mockImplementation(() => {});
+		vi.spyOn(process, "exit").mockImplementation(() => {});
 	});
 
 	afterEach(() => {
@@ -57,27 +55,29 @@ describe("utils", () => {
 		expect(Log.error).not.toHaveBeenCalled();
 	});
 
-	it("exits when modules is not an array", () => {
-		expectExitForModules("not-an-array");
-		expect(Log.error).toHaveBeenCalledWith("This module configuration contains errors:\nmodules must be an array");
+	it("throws when modules is not an array", () => {
+		expectConfigErrorForModules("not-an-array");
+		expect(Log.error).not.toHaveBeenCalled();
 	});
 
-	it("exits when module field is missing or not a string", () => {
-		expectExitForModules([{ module: 123, position: "top_bar" }]);
-		expect(Log.error).toHaveBeenCalled();
-		expect(Log.error.mock.calls[0][0]).toContain("module: must be a string");
+	it("throws when module field is missing or not a string", () => {
+		expectConfigErrorForModules([{ module: 123, position: "top_bar" }]);
+		expect(Log.error).not.toHaveBeenCalled();
 	});
 
 	it("warns for unknown positions without exiting", () => {
-		const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => {
-			throw new ConfigError("");
-		});
-
 		expect(() => {
 			runCheck([{ module: "clock", position: "made_up_region" }]);
 		}).not.toThrow();
-		expect(exitSpy).not.toHaveBeenCalled();
+		expect(process.exit).not.toHaveBeenCalled();
 		expect(Log.warn).toHaveBeenCalled();
 		expect(Log.warn.mock.calls[0][0]).toContain("uses unknown position");
 	});
+
+	it("throws syntax errors with their lint details", () => {
+		expect(() => {
+			runCheck([], "module.exports = { modules: [ };");
+		}).toThrow(/Your configuration file contains syntax errors/);
+		expect(process.exit).not.toHaveBeenCalled();
+	});
 });