Fix installer plugins by relying on automatic discovery (#1082)

* Rely on automatic discovery for installer plugins and document plugin package discovery requirements

docs/devs/plugin-migration-to-2.0.md

@@ -18,6 +18,12 @@ See [Breaking Changes](/docs/decisions.md#breaking-changes) in the decision log.
 Clusterio's packages are now ES modules and plugins should be too.
 Add `"type": "module"` to the plugin's `package.json` and replace `require()` and `module.exports` with `import` and `export`.
 
+Keep `"clusterio-plugin"` in the package's `"keywords"` for automatic discovery, and keep `"main"` pointing to the plugin info module (or use the default `index.js`).
+An `"exports"` map is not required for ES modules.
+If you add one, expose `"./package.json": "./package.json"`; otherwise Clusterio cannot discover or load the plugin by its package name.
+Clusterio currently reads the implementation path from `"main"` rather than conditional exports.
+See [Plugin Structure](../writing-plugins.md#plugin-structure) for a compatible package example.
+
 Relative imports must include the full filename with the `.js` extension.
 This also applies in TypeScript, where you import `./messages.js` even though the source file is `messages.ts`.
 Importing a directory no longer resolves to its `index.js`.

docs/writing-plugins.md

@@ -39,6 +39,8 @@ The basic file structure of a plugin is the following.
 Clusterio plugins are Node.js packages that can be published on npm for ease of installation and distribution.
 The usual guides for creating such packages apply.
 At minimum the `package.json` file must contain a version entry.
+For automatic discovery, include `"clusterio-plugin"` in the package's `"keywords"` array.
+Installed npm plugins are discovered from the installation's dependencies when controller, host or ctl loads its configuration, and added to `plugin-list.json`.
 
 A possible workflow for developing plugins is to place the plugin in a sub-directory of where clusterio has been installed, and rely on Node.js searching up the folder heirarchy for it to find `@clusterio/lib`.
 After creating a directory for your plugin, the boilerplate can be generated automaticlly from a set of tempaltes using `npm exec @clusterio/create -y -- --plugin-template` after which you will be asked which areas your plugin will interact with and your plugin's name.
@@ -47,6 +49,28 @@ Note that it's important that the path starts with ./ or ../ (use .\ or ..\ on W
 
 For a plugin to be recognised by Clusterio it needs to export an entry named `plugin` from its main entrypoint.
 By default the main entrypoint is the `index.js` file, but this may be changed by setting the `"main"` entry to a different file in `package.json`.
+Keep this `"main"` entry when using a package `"exports"` map: Clusterio currently loads the plugin implementation from `"main"` (or `index.js`), rather than selecting a conditional export.
+
+If a plugin published on npm defines an `"exports"` map, it must also expose `"./package.json"`.
+Both automatic discovery and runtime loading resolve `<package-name>/package.json` to read the plugin's metadata.
+Packages that hide this file are currently skipped by discovery, and manually adding their package names to `plugin-list.json` does not make them loadable.
+The generated plugin templates omit `"exports"`, which leaves `package.json` accessible.
+For example, a plugin that needs an export map can use:
+
+```json
+{
+    "name": "example-clusterio-plugin",
+    "version": "1.0.0",
+    "type": "module",
+    "main": "./index.js",
+    "keywords": ["clusterio-plugin"],
+    "exports": {
+        ".": "./index.js",
+        "./package.json": "./package.json"
+    }
+}
+```
+
 Here's an example of `index.js`:
 
 ```js

packages/create/create.js

@@ -392,31 +392,7 @@ async function installClusterio(mode, plugins) {
 		throw new InstallError(`Failed to install: ${err.message}`);
 	}
 
-	if (plugins.length) {
-		logger.info("Setting up plugins");
-		let pluginList;
-		try {
-			pluginList = new Map(JSON.parse(await fs.readFile("plugin-list.json")));
-		} catch (err) {
-			if (err.code === "ENOENT") {
-				pluginList = new Map();
-			} else {
-				throw new InstallError(`Error loading plugin-list.json: ${err.message}`);
-			}
-		}
-		for (let plugin of plugins) {
-			if (!pluginList.has(plugin)) {
-
-				let pluginInfo = (await import(plugin)).plugin;
-				pluginList.set(pluginInfo.name, plugin);
-			}
-		}
-		try {
-			await safeOutputFile("plugin-list.json", JSON.stringify([...pluginList], null, "\t"));
-		} catch (err) {
-			throw new InstallError(`Error writing plugin-list.json: ${err.message}`);
-		}
-	}
+	// Controller, host and ctl discover installed plugins when loading their configuration.
 }
 
 async function groupIdToName(gid) {
@@ -905,6 +881,7 @@ async function main() {
 
 if (import.meta.main) {
 	main().catch(err => {
+		process.exitCode = 1;
 		if (err instanceof InstallError) {
 			logger.error(err.message);
 		} else {

test/create.js

@@ -0,0 +1,126 @@
+import assert from "node:assert/strict";
+import { execFile } from "node:child_process";
+import fs from "node:fs/promises";
+import os from "node:os";
+import path from "node:path";
+import { promisify } from "node:util";
+
+const execFileAsync = promisify(execFile);
+const installer = path.resolve(import.meta.dirname, "../packages/create/create.js");
+const discoveryUrl = new URL("../packages/lib/dist/node/src/load_plugin_list.js", import.meta.url).href;
+
+describe("create installer CLI", function() {
+	this.timeout(10000);
+	let directory;
+
+	beforeEach(async function() {
+		directory = await fs.mkdtemp(path.join(os.tmpdir(), "clusterio install #"));
+		await fs.writeFile(path.join(directory, "package.json"), JSON.stringify({
+			name: "clusterio-install", private: true,
+		}));
+	});
+
+	afterEach(async function() {
+		await fs.rm(directory, { recursive: true, force: true, maxRetries: 10 });
+	});
+
+	async function addPlugin(name, type, source) {
+		const pluginDir = path.join(directory, "node_modules", name);
+		await fs.mkdir(pluginDir, { recursive: true });
+		await fs.writeFile(path.join(pluginDir, "package.json"), JSON.stringify({
+			name, version: "1.0.0", type, main: "index.js", keywords: ["clusterio-plugin"],
+		}));
+		await fs.writeFile(path.join(pluginDir, "index.js"), source);
+		const packagePath = path.join(directory, "package.json");
+		const pkg = JSON.parse(await fs.readFile(packagePath));
+		pkg.dependencies = { ...pkg.dependencies, [name]: "1.0.0" };
+		await fs.writeFile(packagePath, JSON.stringify(pkg));
+	}
+
+	function runInstaller(plugins, installFails = false) {
+		// Only replace npm's subprocess: fixtures stand in for downloaded packages.
+		// Run the actual CLI from outside the installation, like npm init does.
+		const mockNpm = `
+			import assert from "node:assert/strict";
+			import childProcess from "node:child_process";
+			import { PassThrough } from "node:stream";
+			childProcess.execFile = (command, options, callback) => {
+				assert.match(command, /^npm(?:\\.cmd)? install /);
+				const stdout = new PassThrough();
+				const stderr = new PassThrough();
+				queueMicrotask(() => {
+					stdout.end();
+					stderr.end();
+					callback(${installFails} ? new Error("npm install failed") : null, "", "");
+				});
+				return { stdout, stderr };
+			};
+		`;
+		return execFileAsync(process.execPath, [
+			"--import", `data:text/javascript,${encodeURIComponent(mockNpm)}`,
+			installer, "--mode", "plugins",
+			...(process.platform === "linux" ? ["--allow-install-as-root"] : []),
+			"--plugins", ...plugins,
+		], { cwd: directory });
+	}
+
+	it("installs plugins without importing them or writing a plugin list", async function() {
+		await addPlugin("@installer-test/deferred", "module", 'throw new Error("installer must not import plugins");');
+		const { stdout } = await runInstaller(["@installer-test/deferred"]);
+		assert.match(stdout, /Successfully installed plugins/);
+		await assert.rejects(fs.access(path.join(directory, "plugin-list.json")), { code: "ENOENT" });
+	});
+
+	[false, true].forEach(existingList => {
+		it(`defers registration to discovery ${existingList ? "preserving existing entries" : "with no list"}`,
+			async function() {
+				await addPlugin("@installer-test/esm", "module", 'export const plugin = { name: "esm_plugin" };');
+				await addPlugin("@installer-test/commonjs", "commonjs", 'exports.plugin = { name: "cjs_plugin" };');
+				const listPath = path.join(directory, "plugin-list.json");
+				const previousEntries = existingList ? [["manual", "./plugins/manual"]] : [];
+				const previousContents = JSON.stringify(previousEntries);
+				if (existingList) {
+					await fs.writeFile(listPath, previousContents);
+				}
+				const { stdout } = await runInstaller(["@installer-test/esm", "@installer-test/commonjs"]);
+				assert.match(stdout, /Successfully installed plugins/);
+				if (existingList) {
+					assert.equal(await fs.readFile(listPath, "utf8"), previousContents);
+				} else {
+					await assert.rejects(fs.access(listPath), { code: "ENOENT" });
+				}
+
+				// Use the real discovery function invoked by controller/host/ctl commands.
+				await execFileAsync(process.execPath, [
+					"--input-type=module", "--eval",
+					`import { loadPluginList } from ${JSON.stringify(discoveryUrl)};
+					await loadPluginList("plugin-list.json");`,
+				], { cwd: directory });
+				assert.deepEqual(JSON.parse(await fs.readFile(listPath)), [
+					...previousEntries,
+					["esm_plugin", "@installer-test/esm"],
+					["cjs_plugin", "@installer-test/commonjs"],
+				]);
+			}
+		);
+	});
+
+	it("exits with failure when npm installation fails", async function() {
+		await assert.rejects(runInstaller(["@installer-test/missing"], true), err => {
+			assert.equal(err.code, 1);
+			assert.match(err.stdout + err.stderr, /Failed to install: npm install failed/);
+			assert.doesNotMatch(err.stdout, /Successfully installed/);
+			return true;
+		});
+	});
+
+	it("exits with failure on an unexpected installer error", async function() {
+		await fs.writeFile(path.join(directory, "package.json"), "null");
+		await assert.rejects(runInstaller([]), err => {
+			assert.equal(err.code, 1);
+			assert.match(err.stdout + err.stderr, /TypeError/);
+			assert.doesNotMatch(err.stdout, /Successfully installed/);
+			return true;
+		});
+	});
+});