Merge pull request #2035 from thelounge/astorije/ensure-packages-are-dirs

Ensure packages loaded are directories

src/command-line/install.js

@@ -35,11 +35,10 @@ program
 			log.info(`Installing ${colors.green(packageName)}...`);
 
 			const packagesPath = Helper.getPackagesPath();
-			const packagesParent = path.dirname(packagesPath);
+			const packagesConfig = path.join(packagesPath, "package.json");
-			const packagesConfig = path.join(packagesParent, "package.json");
 
 			// Create node_modules folder, otherwise npm will start walking upwards to find one
-			fsextra.ensureDirSync(packagesPath);
+			fsextra.ensureDirSync(path.join(packagesPath, "node_modules"));
 
 			// Create package.json with private set to true to avoid npm warnings, if
 			// it doesn't exist already
@@ -61,7 +60,7 @@ program
 					"--no-package-lock",
 					"--no-progress",
 					"--prefix",
-					packagesParent,
+					packagesPath,
 					packageName,
 				],
 				{

src/command-line/uninstall.js

@@ -22,8 +22,7 @@ program
 		log.info(`Uninstalling ${colors.green(packageName)}...`);
 
 		const packagesPath = Helper.getPackagesPath();
-		const packagesParent = path.dirname(packagesPath);
+		const packagesConfig = path.join(packagesPath, "package.json");
-		const packagesConfig = path.join(packagesParent, "package.json");
 		const packageWasNotInstalled = `${colors.green(packageName)} was not installed.`;
 
 		if (!fs.existsSync(packagesConfig)) {
@@ -48,7 +47,7 @@ program
 				"--depth",
 				"0",
 				"--prefix",
-				packagesParent,
+				packagesPath,
 				packageName,
 			],
 			{
@@ -78,9 +77,10 @@ program
 				npm,
 				[
 					"uninstall",
+					"--save",
 					"--no-progress",
 					"--prefix",
-					packagesParent,
+					packagesPath,
 					packageName,
 				],
 				{

src/helper.js

@@ -75,7 +75,7 @@ function setHome(newPath) {
 	configPath = path.join(homePath, "config.js");
 	usersPath = path.join(homePath, "users");
 	storagePath = path.join(homePath, "storage");
-	packagesPath = path.join(homePath, "packages", "node_modules");
+	packagesPath = path.join(homePath, "packages");
 
 	// Reload config from new home location
 	if (fs.existsSync(configPath)) {
@@ -144,7 +144,7 @@ function getPackagesPath() {
 }
 
 function getPackageModulePath(packageName) {
-	return path.join(Helper.getPackagesPath(), packageName);
+	return path.join(Helper.getPackagesPath(), "node_modules", packageName);
 }
 
 function ip2hex(address) {

src/plugins/packages/index.js

@@ -1,7 +1,7 @@
 "use strict";
 
-const fs = require("fs");
 const colors = require("colors/safe");
+const path = require("path");
 const Helper = require("../../helper");
 const themes = require("./themes");
 const packageMap = new Map();
@@ -35,38 +35,43 @@ function getPackage(name) {
 }
 
 function loadPackages() {
-	fs.readdir(Helper.getPackagesPath(), (err, packages) => {
+	const packageJson = path.join(Helper.getPackagesPath(), "package.json");
-		if (err) {
+	let packages;
+
+	try {
+		packages = Object.keys(require(packageJson).dependencies);
+	} catch (e) {
+		packages = [];
+	}
+
+	packages.forEach((packageName) => {
+		const errorMsg = `Package ${colors.bold(packageName)} could not be loaded`;
+		let packageInfo;
+		let packageFile;
+
+		try {
+			packageInfo = require(path.join(Helper.getPackageModulePath(packageName), "package.json"));
+			packageFile = require(Helper.getPackageModulePath(packageName));
+		} catch (e) {
+			log.warn(errorMsg);
 			return;
 		}
-		packages.forEach((packageName) => {
-			const packageFile = getModuleInfo(packageName);
-			if (!packageFile) {
-				return;
-			}
-			packageMap.set(packageName, packageFile);
-			if (packageFile.type === "theme") {
-				themes.addTheme(packageName, packageFile);
-			}
 
-			if (packageFile.onServerStart) {
+		if (!packageInfo.thelounge) {
-				packageFile.onServerStart(packageApis(packageName));
+			log.warn(errorMsg);
-			}
+			return;
-		});
+		}
+
+		packageMap.set(packageName, packageFile);
+
+		if (packageInfo.type === "theme") {
+			themes.addTheme(packageName, packageInfo);
+		}
+
+		if (packageFile.onServerStart) {
+			packageFile.onServerStart(packageApis(packageName));
+		}
+
+		log.info(`Package ${colors.bold(packageName)} loaded`);
 	});
 }
-
-function getModuleInfo(packageName) {
-	let module;
-	try {
-		module = require(Helper.getPackageModulePath(packageName));
-	} catch (e) {
-		log.warn(`Specified package ${colors.yellow(packageName)} is not installed in packages directory`);
-		return;
-	}
-	if (!module.thelounge) {
-		log.warn(`Specified package ${colors.yellow(packageName)} doesn't have required information.`);
-		return;
-	}
-	return module.thelounge;
-}

test/fixtures/.gitignore

@@ -1,2 +1,5 @@
-# files that may be generated by tests
+# Files that may be generated by tests
 .lounge/storage/
+
+# Fixtures contain fake packages, stored in a fake node_modules folder
+!.lounge/packages/node_modules/

test/fixtures/.lounge/packages/node_modules/thelounge-package-foo/index.js

@@ -0,0 +1,7 @@
+"use strict";
+
+module.exports = {
+	onServerStart(apis) {
+		apis.Stylesheets.addFile("style.css");
+	},
+};

test/fixtures/.lounge/packages/node_modules/thelounge-package-foo/package.json

@@ -0,0 +1,12 @@
+{
+  "name": "thelounge-package-foo",
+  "private": true,
+  "main": "index.js",
+  "thelounge": {
+    "type": "package"
+  },
+  "keywords": [
+    "thelounge",
+    "thelounge-package"
+  ]
+}

test/fixtures/.lounge/packages/package.json

@@ -0,0 +1,6 @@
+{
+  "private": true,
+  "dependencies": {
+    "thelounge-package-foo": "*"
+  }
+}

test/plugins/packages/indexTest.js

@@ -0,0 +1,61 @@
+"use strict";
+
+const expect = require("chai").expect;
+const TestUtil = require("../../util");
+
+let packages;
+
+describe("packages", function() {
+	let originalLogInfo;
+
+	beforeEach(function() {
+		originalLogInfo = log.info;
+		log.info = () => {};
+
+		delete require.cache[require.resolve("../../../src/plugins/packages")];
+		packages = require("../../../src/plugins/packages");
+	});
+
+	afterEach(function() {
+		log.info = originalLogInfo;
+	});
+
+	describe(".getStylesheets", function() {
+		it("should contain no stylesheets before packages are loaded", function() {
+			expect(packages.getStylesheets()).to.be.empty;
+		});
+
+		it("should return the list of registered stylesheets for loaded packages", function() {
+			packages.loadPackages();
+
+			expect(packages.getStylesheets()).to.deep.equal([
+				"thelounge-package-foo/style.css",
+			]);
+		});
+	});
+
+	describe(".getPackage", function() {
+		it("should contain no reference to packages before loading them", function() {
+			expect(packages.getPackage("thelounge-package-foo")).to.be.undefined;
+		});
+
+		it("should return details of a registered package after it was loaded", function() {
+			packages.loadPackages();
+
+			expect(packages.getPackage("thelounge-package-foo"))
+				.to.have.key("onServerStart");
+		});
+	});
+
+	describe(".loadPackages", function() {
+		it("should display report about loading packages", function() {
+			// Mock `log.info` to extract its effect into a string
+			let stdout = "";
+			log.info = TestUtil.mockLogger((str) => stdout += str);
+
+			packages.loadPackages();
+
+			expect(stdout).to.deep.equal("Package thelounge-package-foo loaded\n");
+		});
+	});
+});

test/util.js

@@ -27,7 +27,7 @@ function mockLogger(callback) {
 	return function() {
 		// TODO: Use ...args with The Lounge v3: add `...args` as function argument
 		//       and replaced the next line with `args.join(", ")`
-		const stdout = Array.prototype.slice.call(arguments).join(", ")
+		const stdout = Array.prototype.slice.call(arguments).join(" ")
 			.replace( // Removes ANSI colors. See https://stackoverflow.com/a/29497680
 				/[\u001b\u009b][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-ORZcf-nqry=><]/g,
 				""