diff --git a/.changeset/serve-config-plugin-host-resolution.md b/.changeset/serve-config-plugin-host-resolution.md new file mode 100644 index 0000000000..b924379d32 --- /dev/null +++ b/.changeset/serve-config-plugin-host-resolution.md @@ -0,0 +1,25 @@ +--- +"@objectstack/cli": patch +--- + +`os serve` now resolves a `plugins: [...]` entry the served app **declares** from +that app, instead of from the CLI (#10908). + +`plugins: [...]` in the app's own `objectstack.config.ts` is the documented way +to extend a deployment, but its string entries were loaded with a bare +`import()`, which Node ESM resolves against the CLI's realpath. An app that +wrote `plugins: ['@acme/my-plugin']` and declared `@acme/my-plugin` in its own +`package.json` could therefore only be served where that package happened to be +hoisted somewhere the CLI could see it — true in a dev checkout, absent on a +real distribution layout. Same mechanism as the cluster and organizations loads +fixed earlier. + +Only the **declared** case moves. A specifier the app does not declare still +resolves from the CLI exactly as before, and a path or `file://` URL keeps the +base it always had, so no deployment loses a plugin it is loading today. Which +plugins are *accepted* is unchanged — the declaration gate is untouched. + +One user-facing message changes: when a declared plugin cannot be loaded, the +`Failed to import plugin ''` error now carries the declaration remedy +("declare it in that app's `package.json`", or the install-problem text when the +app declares it but it is not installed) instead of a bare `Cannot find package`. diff --git a/packages/cli/src/commands/serve-cluster-host-resolution.test.ts b/packages/cli/src/commands/serve-cluster-host-resolution.test.ts index 471797d4b0..6abed90d0a 100644 --- a/packages/cli/src/commands/serve-cluster-host-resolution.test.ts +++ b/packages/cli/src/commands/serve-cluster-host-resolution.test.ts @@ -278,11 +278,18 @@ const UNRESOLVABLE_BARE_IMPORTS: Record = { // Serve.CAPABILITY_PROVIDERS — every `pkg` in that table is CLI-declared. 'spec.pkg': 'Serve.CAPABILITY_PROVIDERS entries are all CLI-declared', 'ex.pkg': 'CAPABILITY_PROVIDERS `extras` entries are all CLI-declared', - // The app's own `plugins: [...]` config entries — an app-supplied specifier, so - // this IS the class, but no source scan can classify it and host-anchoring it - // changes a user-facing error message plus which copy of a CLI-declared plugin - // wins. Filed as #10908 rather than widened here. - plugin: 'app-supplied plugin name from objectstack.config.ts — see #10908', + // The app's own `plugins: [...]` config entries, now routed through + // `Serve.importConfigPlugin` (#10908). Two bare `import()` sites remain there, + // both reached only AFTER the declaration has been consulted, and both are the + // reason this list exists rather than a hole in it: + // • the specifier is not a package name at all (path, `file://`, `node:`) — + // nothing a package.json can declare; + // • the served app does NOT declare it, so it must resolve from this CLI, + // which is exactly the pre-existing behaviour #10908 promised to keep. + // The DECLARED case — the only one this card moves — goes to `importFromHost`. + // Pinned behaviourally, not by this comment, in + // `serve-config-plugin-host-resolution.test.ts`. + pluginSpecifier: 'post-declaration branches: a path/URL, or a package the app does not declare (#10908)', }; /** diff --git a/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts b/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts new file mode 100644 index 0000000000..581da9bbb5 --- /dev/null +++ b/packages/cli/src/commands/serve-config-plugin-host-resolution.test.ts @@ -0,0 +1,202 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { readFileSync } from 'node:fs'; +import { afterAll, describe, expect, it } from 'vitest'; +import Serve from './serve.js'; + +/** + * `plugins: [...]` in the served app's own `objectstack.config.ts` is THE + * documented way to extend a deployment, and its string entries are the most + * app-owned specifiers in `serve.ts`. They were loaded with a bare `import()`, + * which Node ESM resolves against the CLI's realpath — so a plugin the APP + * declares could only be served where it happened to be hoisted somewhere the + * CLI could see. Green in a dev checkout, absent on a real distribution layout + * (#10908; the same mechanism as cloud#1013 and #10645). + * + * The repair moves ONLY the declared case. These tests pin all three branches, + * because two of them exist to keep behaviour that a naive + * `await importFromHost(specifier)` would have taken away — see + * `Serve.importConfigPlugin` for the measurements. + */ + +const roots: string[] = []; +afterAll(() => { + for (const r of roots) rmSync(r, { recursive: true, force: true }); +}); + +/** + * An app root with its own `package.json`, optionally DECLARING `pkgName` and + * optionally carrying it in its own `node_modules`. Nothing is installed or + * built: the package is two files in a temp dir, and its marker export is how a + * test proves WHICH copy loaded. + */ +function makeApp( + pkgName: string, + opts: { declare: boolean; install: boolean; marker?: string }, +): string { + const root = mkdtempSync(join(tmpdir(), 'os-cfg-plugin-')); + roots.push(root); + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'fixture-app', + version: '1.0.0', + type: 'module', + ...(opts.declare ? { dependencies: { [pkgName]: '1.0.0' } } : {}), + }), + ); + if (opts.install) { + const dir = join(root, 'node_modules', ...pkgName.split('/')); + mkdirSync(dir, { recursive: true }); + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ name: pkgName, version: '1.0.0', type: 'module', main: 'index.js' }), + ); + writeFileSync( + join(dir, 'index.js'), + `export default { name: ${JSON.stringify(opts.marker ?? 'app-copy')} };\n`, + ); + } + return root; +} + +// A name no workspace package can satisfy, so a pass can never come from the +// CLI's own node_modules by accident. +const APP_ONLY = '@os-fixture/config-plugin-probe'; + +describe('os serve → an app-declared `plugins: [...]` package resolves from the APP (#10908)', () => { + it('loads the copy the served app declares and carries — the defect, repaired', async () => { + const root = makeApp(APP_ONLY, { declare: true, install: true, marker: 'app-copy' }); + + // The failing hop, reproduced first: this file resolves from `packages/cli` + // exactly as `dist/commands/serve.js` does, and cannot see the app's package. + const bare: string = APP_ONLY; + await expect(import(bare)).rejects.toMatchObject({ + code: expect.stringMatching(/MODULE_NOT_FOUND|ERR_MODULE_NOT_FOUND/), + }); + + const mod = await Serve.importConfigPlugin(APP_ONLY, root); + expect(mod.default).toEqual({ name: 'app-copy' }); + }); + + it("prefers the APP's copy over the CLI's own when BOTH can resolve the name", async () => { + // The resolution-policy question the card names: which copy wins when the + // CLI also ships the package. `chalk` is declared by packages/cli and + // resolves from this file, so a fixture app that declares its OWN `chalk` + // is the only way to tell the two apart — and the app's declaration is the + // contract (#4719), so the app's copy must win. + const root = makeApp('chalk', { declare: true, install: true, marker: 'app-owned-chalk' }); + + const mod = await Serve.importConfigPlugin('chalk', root); + + expect(mod.default).toEqual({ name: 'app-owned-chalk' }); + // Not a tautology: the CLI's own resolution of the same name finds the real + // package, which is what this line would have loaded before the fix. + const cliCopy: any = await import('chalk'); + expect(cliCopy.default).not.toEqual({ name: 'app-owned-chalk' }); + }); + + it('a package the app DECLARES but never installed reports the INSTALL remedy, not an absence', async () => { + const root = makeApp(APP_ONLY, { declare: true, install: false }); + + const err = await Serve.importConfigPlugin(APP_ONLY, root).catch((e: unknown) => e as Error); + + expect(err).toBeInstanceOf(Error); + expect(err.message).toContain(`Failed to import plugin '${APP_ONLY}':`); + // The app asked for it, so re-reading the manifest is not the remedy. + expect(err.message).toContain('DECLARES it'); + expect(err.message).toMatch(/INSTALL problem, not a declaration problem/); + }); +}); + +/** + * Triage ② on #10908: host-anchoring changes the user-facing text a missing + * plugin produces — the wrapper now nests `createHostImporter`'s #4719 remedy. + * That is a better diagnostic, but it is VISIBLE, so it is pinned here as a + * chosen behaviour rather than left to drift. + */ +describe('os serve → the missing-plugin diagnostic is a chosen text (#10908 / #4719)', () => { + it('names the plugin, then tells the author to DECLARE it in that app', async () => { + const root = makeApp(APP_ONLY, { declare: false, install: false }); + + const err = await Serve.importConfigPlugin(APP_ONLY, root).catch((e: unknown) => e as Error); + + expect(err).toBeInstanceOf(Error); + // The wrapper `serve` has always put around a failed plugin load. + expect(err.message).toContain(`Failed to import plugin '${APP_ONLY}':`); + // …now carrying the #4719 remedy instead of a bare "Cannot find package". + expect(err.message).toMatch(/Declare it in that app's package\.json/); + expect(err.message).toContain(root); + // The gate's own reasoning survives into what the user reads: being merely + // reachable is refused ON PURPOSE, so nobody "fixes" this with NODE_PATH. + expect(err.message).toMatch(/merely REACHABLE is not enough/); + }); +}); + +/** + * The two branches that exist so this card could not take working deployments + * away. Both were MEASURED against `createHostImporter` before being written: + * its pass-through and its undeclared fallback both re-enter `import()` from + * inside `@objectstack/types`, which moves the resolution base. + */ +describe('os serve → the branches that must NOT move (#10908 supersedes nothing)', () => { + it('keeps this CLI as the resolver for a package the app does not declare', async () => { + // `chalk` is declared by packages/cli and by no fixture app. Today's bare + // `import()` finds it; through the host importer's fallback — which resolves + // from `@objectstack/types` — it does not. An app that writes + // `plugins: ['@objectstack/plugin-auth']` without declaring it boots today, + // and this is the assertion that says it still does. + const root = makeApp(APP_ONLY, { declare: false, install: false }); + + const mod = await Serve.importConfigPlugin('chalk', root); + expect(mod.default ?? mod).toBeTruthy(); + }); + + it('keeps a RELATIVE specifier anchored to serve.ts, not to @objectstack/types', async () => { + const root = makeApp(APP_ONLY, { declare: false, install: false }); + const missing = './__no_such_config_plugin_10908__.js'; + + const err = await Serve.importConfigPlugin(missing, root).catch((e: unknown) => e as Error); + + expect(err).toBeInstanceOf(Error); + expect(err.message).toContain(`Failed to import plugin '${missing}':`); + // The base is what this pins: the directory holding serve.ts. Routing this + // spelling through the host importer would silently re-base it under + // `@objectstack/types/dist/`, which is the regression this branch prevents. + // Neither base is the served app's root — whether a relative entry SHOULD + // resolve there is #10944, deliberately left open by this card. + expect(err.message).toContain('commands'); + expect(err.message).not.toContain('types/dist'); + }); + + it('loads an absolute path and a file:// URL unchanged (base-independent spellings)', async () => { + const root = makeApp(APP_ONLY, { declare: false, install: false }); + const file = join(root, 'local-plugin.js'); + writeFileSync(file, 'export default { name: "app-local-plugin" };\n'); + + for (const spelling of [file, pathToFileURL(file).href]) { + const mod = await Serve.importConfigPlugin(spelling, root); + expect(mod.default).toEqual({ name: 'app-local-plugin' }); + } + }); +}); + +describe('os serve → the config-plugin load stays wired to the helper', () => { + const SERVE_SOURCE = readFileSync(new URL('./serve.ts', import.meta.url), 'utf8'); + + it('the boot loop calls the helper and no longer bare-imports the entry', () => { + expect(SERVE_SOURCE).toContain('await Serve.importConfigPlugin(plugin, hostRoot)'); + // The exact shape the card was filed against — it must not come back. + expect(SERVE_SOURCE).not.toMatch(/const imported = await import\(plugin\)/); + }); + + it('the declaration decides the resolver, so the gate keeps its say (#4719)', () => { + // A helper that stopped consulting the declaration would still pass every + // behavioural test above that uses a DECLARED fixture, so pin the wiring. + const helper = SERVE_SOURCE.slice(SERVE_SOURCE.indexOf('static async importConfigPlugin')); + expect(helper).toContain('isDeclaredByHost(pluginSpecifier, root)'); + expect(helper).toContain('importFromHost(pluginSpecifier, root)'); + }); +}); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 95a1437c03..424a822d1f 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -54,6 +54,7 @@ import { createHostImporter, hostImportFailureKind, isDeclaredByHost, + packageNameFromSpecifier, readHostDeclaration, type HostImporter, } from '@objectstack/types/node'; @@ -393,6 +394,98 @@ export default class Serve extends Command { return 'off'; } + /** + * Load one `plugins: [...]` entry of the served app's own config that is + * written as a STRING (#10908). + * + * This is the most app-owned specifier in the whole file — it is supplied by + * the app being served, and `plugins: [...]` is THE documented way to extend a + * deployment. It used to be loaded with a bare `import()`, which Node ESM + * resolves against the IMPORTER's realpath: this CLI's. So an app that writes + * `plugins: ['@acme/my-plugin']` and DECLARES `@acme/my-plugin` in its own + * package.json could only be served where that package happened to be hoisted + * somewhere the CLI could see it — true in a dev checkout, false in a real + * distribution layout. Same mechanism as cloud#1013 and #10645, but on the + * surface users are explicitly told to use. + * + * ── Why this is three branches and not `await importFromHost(specifier)` ──── + * + * The obvious repair is to hand every specifier to `importFromHost`. MEASURED, + * that is NOT a superset of what this line does today — in two ways, both of + * which would take working deployments away: + * + * 1. A RELATIVE specifier changes base. `createHostImporter` passes a + * non-package specifier through to an `import()` that physically lives in + * `@objectstack/types`, and ESM resolves a relative specifier against the + * module CONTAINING the call — so `'./local-plugin.js'` would resolve + * against `@objectstack/types/dist/` instead of this file's directory. + * Neither base is the served app's root, so no relative spelling works + * the way an author would expect either way; #10944 carries that + * question, and this branch is why the answer stays open rather than + * being decided by a silent re-base here. + * 2. An UNDECLARED bare name changes base the same way, and this one bites. + * `createHostImporter`'s fallback is documented as "the importing + * package's own resolution", but the import it falls back to also lives + * in `@objectstack/types`, which under a pnpm-isolated layout can see + * only `@objectstack/types`'s own dependencies. Measured from an app that + * declares nothing: `@objectstack/plugin-auth` and `@objectstack/plugin- + * audit` resolve from THIS package and fail through the host importer. + * An app that writes `plugins: ['@objectstack/plugin-auth']` without + * declaring it — a spelling this repo's own fixtures use — boots today + * and would stop booting. The helper's own docblock claims the opposite + * ("falls back to the importing package's own resolution"); that text is + * wrong, and #10943 carries the fix. Until it lands, a caller that needs + * its own resolution has to ask the declaration itself, as below. + * + * So the declaration is what selects the resolver, exactly as #4719 says it + * should, and each branch keeps the resolution it already had: + * + * • not a package name (path, `file://` URL, `node:` builtin) → unchanged; + * nothing a package.json can declare, so the gate has no opinion. + * • DECLARED by the served app → `importFromHost`: the app's own copy wins. + * This is the repair — the whole card is this branch. + * • UNDECLARED → this CLI's own resolution, byte-identical to the bare + * `import()` that has always been here. No app loses a plugin it does not + * declare but the CLI ships. + * + * Nothing about WHICH plugins are accepted changes: this only moves where a + * declared one resolves FROM. The #4719 declaration gate is untouched, and no + * undeclared package gains a way in that it did not already have. + * + * @param pluginSpecifier The string as the app wrote it in `plugins: [...]`. + * @param hostRoot Root of the served app; defaults to the process CWD, the + * same value `serve`'s boot path computes. + */ + static async importConfigPlugin(pluginSpecifier: string, hostRoot?: string): Promise { + const root = hostRoot ?? process.cwd(); + try { + // `await` inside the `try` rather than a bare `return`: a returned promise + // would settle OUTSIDE it and skip the diagnostic wrapper below. + if (packageNameFromSpecifier(pluginSpecifier) === undefined) { + return await import(/* webpackIgnore: true */ pluginSpecifier); + } + if (isDeclaredByHost(pluginSpecifier, root)) { + return await importFromHost(pluginSpecifier, root); + } + try { + return await import(/* webpackIgnore: true */ pluginSpecifier); + } catch (cliError: unknown) { + // Present but broken is a crash, not an absence — never reinterpret it. + if (!Serve.isModuleNotFoundError(cliError)) throw cliError; + // Undeclared AND unresolvable anywhere. Re-enter the host importer for + // the failure alone: it owns the #4719 "declare it in that app's + // package.json" remedy, and having one owner of that wording is why + // this does not compose the message itself. + return await importFromHost(pluginSpecifier, root); + } + } catch (importError: any) { + // The wrapper lives with the load it describes, so the composed + // user-facing string is testable rather than assembled at the call site + // (triage on #10908 requires this text be CHOSEN, not drift). + throw new Error(`Failed to import plugin '${pluginSpecifier}': ${importError.message}`); + } + } + /** * Tier-gated capability tokens → the tier each one opens when listed in * `requires`. These have no CAPABILITY_PROVIDERS entry — their loading is @@ -2673,12 +2766,11 @@ export default class Serve extends Command { // Resolve string references (package names) if (typeof plugin === 'string') { - try { - const imported = await import(plugin); - pluginToLoad = imported.default || imported; - } catch (importError: any) { - throw new Error(`Failed to import plugin '${plugin}': ${importError.message}`); - } + // Host-anchored, NOT a bare `import()`: this specifier comes from + // the served app's own config, so what the app DECLARES about it is + // the contract (#10908). The helper carries the failure wrapper too. + const imported = await Serve.importConfigPlugin(plugin, hostRoot); + pluginToLoad = imported.default || imported; } // Wrap raw config objects (no init/start) into AppPlugin