Fail loud on a null package.json and a stray plugins/package.json

This commit is contained in:
2026-08-17 22:50:29 +02:00
parent 453058c67b
commit 94dc581593
5 changed files with 23 additions and 19 deletions
+1 -2
View File
@@ -7,8 +7,7 @@ FROM node:24.19.0-alpine3.24
COPY package.json package-lock.json .npmrc /deps/ COPY package.json package-lock.json .npmrc /deps/
RUN cd /deps && npm ci && mv node_modules /node_modules && rm -rf /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 # The barrel as a package, so a plugin folder can own a package.json. Linked because it re-exports /app.
# re-exports source under /app.
RUN mkdir -p /node_modules/@plainpages && ln -s /app/plugin-api /node_modules/@plainpages/plugin-api RUN mkdir -p /node_modules/@plainpages && ln -s /app/plugin-api /node_modules/@plainpages/plugin-api
WORKDIR /app WORKDIR /app
+4 -5
View File
@@ -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 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. `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`, - **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 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 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 contract, turning a sign-in redirect into a 500, so discovery refuses one at boot.
standalone typechecking is fine — keep it out of what you mount.)
- **The host never upgrades or dedupes your dependencies.** Two plugins depending on the same package - **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 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 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 `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 `@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 ### Local dev & test story
+6 -2
View File
@@ -60,6 +60,10 @@ const badCases: Array<{ name: string; files: Record<string, string>; 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 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 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 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 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/ }, { 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"); assert.equal(typeof plugins[0]?.dashboard, "function");
}); });
// The barrel still resolves from a folder holding its own package.json because host deps sit at // Host deps sit at /node_modules, above every plugin scope, so the barrel resolves from a folder
// /node_modules, above every plugin scope (README → Plugin dependencies). // that has its own package.json (README → Plugin dependencies).
test("a plugin may carry its own package.json, node_modules and dependencies", async (t) => { test("a plugin may carry its own package.json, node_modules and dependencies", async (t) => {
const dir = scaffold(t, { const dir = scaffold(t, {
"shop/package.json": `{ "name": "shop", "version": "0.0.0", "type": "module", "dependencies": { "price-tag": "1.0.0" } }`, "shop/package.json": `{ "name": "shop", "version": "0.0.0", "type": "module", "dependencies": { "price-tag": "1.0.0" } }`,
+11 -8
View File
@@ -27,6 +27,10 @@ export async function discoverPlugins(options: DiscoverOptions = {}): Promise<Pl
const errors: string[] = []; const errors: string[] = [];
const plugins: Plugin[] = []; const plugins: Plugin[] = [];
if (existsSync(join(dir, "package.json"))) {
errors.push(`plugins/package.json must not exist — it becomes the package scope for every plugin below it; install into plugins/<id>, not plugins/`);
}
for (const id of pluginFolders(dir)) { for (const id of pluginFolders(dir)) {
const fail = (msg: string): void => void errors.push(`plugins/${id}: ${msg}`); const fail = (msg: string): void => void errors.push(`plugins/${id}: ${msg}`);
@@ -89,9 +93,8 @@ function pluginFolders(dir: string): string[] {
.sort(); .sort();
} }
// The two ways a plugin's own packaging breaks it. A barrel copy resolves before the host's, and its // A barrel copy resolves before the host's, so its GuardError matches no `instanceof` here and a
// GuardError matches no `instanceof` here — the sign-in redirect silently becomes a 500. Without a // sign-in redirect becomes a 500. A typeless folder re-parses every file it loads, and warns on each.
// `type`, which npm never writes, the folder is left CommonJS: a .js helper breaks, every .ts re-parses.
function packagingError(folder: string): string | null { function packagingError(folder: string): string | null {
if (existsSync(join(folder, "node_modules", "@plainpages", "plugin-api"))) { 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"; 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"); const file = join(folder, "package.json");
if (!existsSync(file)) return null; if (!existsSync(file)) return null;
let manifest: { type?: unknown }; let manifest: { type?: unknown } | null;
try { try {
manifest = JSON.parse(readFileSync(file, "utf8")) as { type?: unknown }; manifest = JSON.parse(readFileSync(file, "utf8")) as { type?: unknown } | null;
} catch (err) { } 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 ? 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 { function asManifest(value: unknown): PluginManifest | null {
+1 -2
View File
@@ -5,8 +5,7 @@ import assert from "node:assert/strict";
import test from "node:test"; import test from "node:test";
import * as api from "./plugin-api.ts"; 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 // Both specifiers must reach one module instance; the Dockerfile symlink is what makes them.
// there would fail every `instanceof GuardError` a handler makes.
test("the barrel resolves by package name to this same module", async () => { test("the barrel resolves by package name to this same module", async () => {
const asPackage = await import("@plainpages/plugin-api"); const asPackage = await import("@plainpages/plugin-api");