From 77343e859a273d90996c350cd8fc19e7a813f3dd Mon Sep 17 00:00:00 2001 From: lilleman Date: Mon, 17 Aug 2026 22:50:29 +0200 Subject: [PATCH] Fail loud on a null package.json and a stray plugins/package.json --- Dockerfile | 3 +-- README.md | 9 ++++----- src/plugin-host/discovery.test.ts | 8 ++++++-- src/plugin-host/discovery.ts | 19 +++++++++++-------- src/plugin-host/plugin-api.test.ts | 3 +-- 5 files changed, 23 insertions(+), 19 deletions(-) diff --git a/Dockerfile b/Dockerfile index 59b6dd8..689ccfe 100644 --- a/Dockerfile +++ b/Dockerfile @@ -7,8 +7,7 @@ FROM node:24.19.0-alpine3.24 COPY package.json package-lock.json .npmrc /deps/ RUN cd /deps && npm ci && mv node_modules /node_modules && rm -rf /deps -# The barrel as a package, so a plugin folder can own a package.json. Linked, not copied — it -# re-exports source under /app. +# The barrel as a package, so a plugin folder can own a package.json. Linked because it re-exports /app. RUN mkdir -p /node_modules/@plainpages && ln -s /app/plugin-api /node_modules/@plainpages/plugin-api WORKDIR /app diff --git a/README.md b/README.md index 0e6189a..83efb62 100644 --- a/README.md +++ b/README.md @@ -707,13 +707,10 @@ A plugin in its own repo runs its own `npm ci` instead and mounts the result — included, since the plugin folder *is* the repo. A baked image needs no extra step: the plugin's `node_modules` is part of the build context and is `COPY`'d in with the rest of the folder. -Two rules follow from how Node resolves: - - **Never ship a copy of `@plainpages/plugin-api`.** The host publishes it into `/node_modules`, above every plugin, and a plugin resolves it from there — nothing to declare, just import it. A copy inside your own `node_modules` would shadow it with a *second* instance of the host's - contract, turning a sign-in redirect into a 500, so discovery refuses one at boot. (A type stub for - standalone typechecking is fine — keep it out of what you mount.) + contract, turning a sign-in redirect into a 500, so discovery refuses one at boot. - **The host never upgrades or dedupes your dependencies.** Two plugins depending on the same package each get their own copy at their own version, so neither can break the other by upgrading — and keeping yours current, and audited, is yours to own. Renovate here watches the host's manifests @@ -723,7 +720,9 @@ Two rules follow from how Node resolves: `npm run typecheck` covers `plugins/`, so a dependency shipping no types of its own needs its `@types/…` in your plugin's `devDependencies`. Typechecking a plugin repo standalone still needs the -barrel's types on disk: typecheck it mounted under the host tree, or vendor a type stub. +barrel's types on disk: typecheck it mounted under the host tree, or vendor a type stub **outside +`node_modules`** and point tsconfig `paths` at it — a stub inside is the shadowing copy discovery +refuses, and it would travel with the folder you mount. ### Local dev & test story diff --git a/src/plugin-host/discovery.test.ts b/src/plugin-host/discovery.test.ts index f4be606..0243dc8 100644 --- a/src/plugin-host/discovery.test.ts +++ b/src/plugin-host/discovery.test.ts @@ -60,6 +60,10 @@ const badCases: Array<{ name: string; files: Record; match: RegE { name: "a plugin shipping its own copy of the barrel", files: { "shadow/node_modules/@plainpages/plugin-api/index.js": `export class GuardError extends Error {}`, "shadow/plugin.ts": full("shadow") }, match: /shadow.*@plainpages\/plugin-api/s }, { name: "a plugin package.json that forgets type: module", files: { "cjs/package.json": `{ "name": "cjs" }`, "cjs/plugin.ts": full("cjs") }, match: /cjs.*"type": "module"/s }, { name: "a plugin package.json that is not valid JSON", files: { "bent/package.json": `{`, "bent/plugin.ts": full("bent") }, match: /bent.*package\.json.*JSON/s }, + { name: "a plugin package.json holding null", files: { "nul/package.json": `null`, "nul/plugin.ts": full("nul") }, match: /nul.*"type": "module"/s }, + // Written by the documented install command with one path segment dropped — and it would silently + // make every plugin below it part of its scope. + { name: "a package.json in the scan root itself", files: { "package.json": `{ "name": "oops" }`, "ok/plugin.ts": full("ok") }, match: /plugins\/package\.json must not exist/ }, { name: "two plugins claim the public home", files: { "a/plugin.ts": `export default { apiVersion: "1.0.0", home: () => ({ html: "a" }) };`, "b/plugin.ts": `export default { apiVersion: "1.0.0", home: () => ({ html: "b" }) };` }, match: /home/ }, { name: "two plugins claim the gated dashboard", files: { "a/plugin.ts": `export default { apiVersion: "1.0.0", dashboard: () => ({ html: "a" }) };`, "b/plugin.ts": `export default { apiVersion: "1.0.0", dashboard: () => ({ html: "b" }) };` }, match: /dashboard/ }, ]; @@ -105,8 +109,8 @@ test("a plugin may declare `home` (public /) and `dashboard` (gated /dashboard) assert.equal(typeof plugins[0]?.dashboard, "function"); }); -// The barrel still resolves from a folder holding its own package.json because host deps sit at -// /node_modules, above every plugin scope (README → Plugin dependencies). +// Host deps sit at /node_modules, above every plugin scope, so the barrel resolves from a folder +// that has its own package.json (README → Plugin dependencies). test("a plugin may carry its own package.json, node_modules and dependencies", async (t) => { const dir = scaffold(t, { "shop/package.json": `{ "name": "shop", "version": "0.0.0", "type": "module", "dependencies": { "price-tag": "1.0.0" } }`, diff --git a/src/plugin-host/discovery.ts b/src/plugin-host/discovery.ts index 5da1975..e69d203 100644 --- a/src/plugin-host/discovery.ts +++ b/src/plugin-host/discovery.ts @@ -27,6 +27,10 @@ export async function discoverPlugins(options: DiscoverOptions = {}): Promise, not plugins/`); + } + for (const id of pluginFolders(dir)) { const fail = (msg: string): void => void errors.push(`plugins/${id}: ${msg}`); @@ -89,9 +93,8 @@ function pluginFolders(dir: string): string[] { .sort(); } -// The two ways a plugin's own packaging breaks it. A barrel copy resolves before the host's, and its -// GuardError matches no `instanceof` here — the sign-in redirect silently becomes a 500. Without a -// `type`, which npm never writes, the folder is left CommonJS: a .js helper breaks, every .ts re-parses. +// A barrel copy resolves before the host's, so its GuardError matches no `instanceof` here and a +// sign-in redirect becomes a 500. A typeless folder re-parses every file it loads, and warns on each. function packagingError(folder: string): string | null { if (existsSync(join(folder, "node_modules", "@plainpages", "plugin-api"))) { return "ships its own copy of @plainpages/plugin-api — remove it; the host provides the one instance"; @@ -100,15 +103,15 @@ function packagingError(folder: string): string | null { const file = join(folder, "package.json"); if (!existsSync(file)) return null; - let manifest: { type?: unknown }; + let manifest: { type?: unknown } | null; try { - manifest = JSON.parse(readFileSync(file, "utf8")) as { type?: unknown }; + manifest = JSON.parse(readFileSync(file, "utf8")) as { type?: unknown } | null; } catch (err) { - return `package.json is not valid JSON — ${messageOf(err)}`; + return `package.json could not be read as JSON — ${messageOf(err)}`; } - return manifest.type === "module" + return manifest?.type === "module" ? null - : `package.json must set "type": "module" — npm writes no type, which leaves the folder CommonJS`; + : `package.json must set "type": "module" — npm writes no type, and Node then re-parses every file in the folder`; } function asManifest(value: unknown): PluginManifest | null { diff --git a/src/plugin-host/plugin-api.test.ts b/src/plugin-host/plugin-api.test.ts index a6eed66..7e42cea 100644 --- a/src/plugin-host/plugin-api.test.ts +++ b/src/plugin-host/plugin-api.test.ts @@ -5,8 +5,7 @@ import assert from "node:assert/strict"; import test from "node:test"; import * as api from "./plugin-api.ts"; -// A plugin with its own package.json reaches the barrel only as a package; a second copy landing -// there would fail every `instanceof GuardError` a handler makes. +// Both specifiers must reach one module instance; the Dockerfile symlink is what makes them. test("the barrel resolves by package name to this same module", async () => { const asPackage = await import("@plainpages/plugin-api");