diff --git a/.changeset/host-importer-esm-condition.md b/.changeset/host-importer-esm-condition.md new file mode 100644 index 0000000000..db56ea168c --- /dev/null +++ b/.changeset/host-importer-esm-condition.md @@ -0,0 +1,56 @@ +--- +"@objectstack/types": patch +"@objectstack/service-cluster": minor +"@objectstack/cli": patch +--- + +fix(types,cli): resolve host-declared packages through the `import` condition, and read the cluster registry instead of assuming it (#13330) + +`createHostImporter`'s declared leg resolved with `hostRequire.resolve(pkg)` — a +**CommonJS** resolution, which answers the `require` condition. Every `tsup` +dual build publishes `{ "import": "./dist/index.js", "require": "./dist/index.cjs" }`, +so a package loaded through that leg evaluated as its **CommonJS** build while +the callers (`packages/cli` is `"type": "module"`) held the **ESM** build of the +same package. The process ended up with two instances of everything the loaded +package shares with its caller, each with its own module-scope state. + +Measured consequence, on the shipped EE multi-node path (ADR-0018): `os serve` +loaded `@objectstack/service-cluster-redis` through this leg, the driver's +load-time `registerClusterDriver('redis', …)` ran against the CommonJS copy of +`@objectstack/service-cluster`, and the ESM `Runtime` read the ESM copy and +found nothing — `OS_CLUSTER_DRIVER=redis` died at `defineCluster()` with +`Cluster driver "redis" is not registered`, about a package that was installed, +declared and resolvable. Any module-scope registry crossing this seam had the +same defect; the cluster driver is the instance that shipped. + +**The seam.** The declared leg now imports the entry the `import` condition +names. The host anchor is untouched — the CJS resolver still answers *where* +the package is, because no flagless Node API resolves a bare specifier against +an arbitrary parent; only the *condition* is re-decided, by reading that +package's own `exports` map. Deliberately narrow at the **resolution** level — +no load that works today resolves differently unless the package itself +publishes a valid, existing import-condition target: a package with no +`exports` map is untouched (CJS resolution already returned `main`), a package +publishing no import-condition target is untouched, and anything unreadable or +absent on disk falls back to the CJS-resolved path. That narrowness does not +extend to **evaluation**: a dual-published package whose `import` build exists +but throws while its `require` build works used to mask that break by silently +loading the CJS build, and now surfaces it — arguably the correct reading of a +broken published build, but a behaviour change, not a no-op. + +**The reading.** A residual split is still possible above the seam — two +*physical* copies of one package are two instances in any module system, and no +resolver condition merges them — so `os serve` no longer assumes the driver +registered. `@objectstack/service-cluster` exports `listClusterDrivers()`, the +registry `defineCluster()` itself consults, and `serve` queries it after the +load. The silent `catch` is gone: a driver that loaded but stayed invisible, one +that could not be resolved, and one that resolved and then crashed now read as +three different diagnoses instead of arriving as `not registered` one line +later. An app on an older `@objectstack/service-cluster` has no accessor to +call; that case is silent — `serve` declines to claim either answer rather +than printing one. + +No behaviour downstream of the diagnosis changed: an absent driver still reaches +`defineCluster()`'s documented error (`cluster.mdx` §8.1) rather than silently +downgrading to the in-memory cluster, and the only documented downgrade here — +a multi-node gate denial — is untouched. diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index f77ec4e802..1b7de88d55 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -2406,7 +2406,11 @@ export default class Serve extends Command { // The remote driver self-registers on import; import it dynamically so it // works in BOTH config-boot and compiled-artifact mode. Open-core ships // only the in-memory driver — remote drivers (e.g. redis) come from the EE - // distribution; if absent we fall back to the in-memory cluster. + // distribution. An absent driver does NOT fall back to the in-memory + // cluster: `clusterConfig` still names it and `defineCluster()` raises + // its documented error (cluster.mdx §8.1). The only documented downgrade + // here is a multi-node GATE DENIAL, below. (#13330 — this sentence said + // the opposite for as long as the silent catch below agreed with it.) let clusterConfig: { driver: string; url?: string } | undefined; // The gate's verdict, held for the operator-facing telemetry emitted near // the end of boot (#12667). The gate is consulted exactly once per @@ -2432,9 +2436,15 @@ export default class Serve extends Command { // '@objectstack/service-cluster'` and took the whole boot down — while // app-side code loaded the very same package fine. const __clusterPkg: string = '@objectstack/service-cluster'; - const { checkMultiNodeAllowed } = (await importFromHost(__clusterPkg)) as { + // The whole namespace, not just the gate: the DRIVER REGISTRY read + // further down has to come from this same module instance, because + // that is the instance `defineCluster()` consults (#13330). + const __clusterModule = (await importFromHost(__clusterPkg)) as { checkMultiNodeAllowed: (requested?: number) => MultiNodeGateVerdict; + /** Optional: an app on a pre-#13330 `service-cluster` does not have it. */ + listClusterDrivers?: () => string[]; }; + const { checkMultiNodeAllowed } = __clusterModule; // Ask the gate about the topology the operator actually DECLARED. // Calling zero-arg leaves `requested` undefined, which a cap-aware gate // has nothing to clamp against — so the licensed-overflow verdict was @@ -2467,12 +2477,97 @@ export default class Serve extends Command { const __capAdvisory = formatMultiNodeCapAdvisory(__gate); if (__capAdvisory) console.warn(__capAdvisory); // Same host-anchored resolution as the gate above — the shipped - // drivers (`-redis`, `-postgres`, …) are app-declared too. The catch - // stays deliberately silent: the driver may already have been - // registered by the loaded config, and an absent driver is a - // documented fall-back to the in-memory cluster, not a boot failure. - try { await importFromHost(`@objectstack/service-cluster-${__clusterDriver}`); } - catch { /* may already be registered by the loaded config */ } + // drivers (`-redis`, `-postgres`, …) are app-declared too. + // + // ── Why this is no longer a silent catch (#13330) ──────────────── + // + // A driver package's entire contract is a load-time SIDE EFFECT: + // `registerClusterDriver('', …)` into the module-scope + // registry of `@objectstack/service-cluster`, which `defineCluster()` + // reads two statements below. Whether that side effect landed is a + // fact about THIS process, so it is read here rather than assumed. + // + // It used to be assumed. The catch was silent on two stated grounds — + // "may already be registered by the loaded config" and "an absent + // driver is a documented fall-back to the in-memory cluster" — and a + // single EE boot measured both wrong at once: + // + // • the load SUCCEEDED and the registration was invisible. The + // declared leg of `importFromHost` resolved with CommonJS + // semantics, so the driver ran as its `.cjs` build and registered + // into a SECOND instance of the registry, while the ESM Runtime + // read the first. Fixed at the seam (`@objectstack/types/node`); + // this reading is what makes any residual split audible instead + // of arriving as "not registered" one line later. + // • an absent driver falls back to nothing HERE — `clusterConfig` + // below names the driver either way, so `defineCluster()` raises + // its documented error (cluster.mdx §8.1). That is left exactly + // as it is: downgrading to in-memory instead would boot a silent + // single node for an operator who explicitly asked for a remote + // driver, and on the multi-replica deployments this matters for, + // the ADR-0010 split-brain guard throws on that downgrade anyway. + // What changes is only that the reason is no longer swallowed. + // + // Nothing below throws: every branch is a diagnosis printed ahead of + // behaviour that is unchanged. + let __driverLoadError: unknown; + try { + await importFromHost(`@objectstack/service-cluster-${__clusterDriver}`); + } catch (err) { + __driverLoadError = err; + } + // `undefined` ⇒ the app's `@objectstack/service-cluster` predates + // `listClusterDrivers`, so the registry cannot be read from here. + // That is NOT MEASURED — it is not "registered" and not "missing", + // and no branch below claims either. + const __registeredDrivers = + typeof __clusterModule.listClusterDrivers === 'function' + ? __clusterModule.listClusterDrivers() + : undefined; + const __driverVisible = + __registeredDrivers === undefined + ? undefined + : __registeredDrivers.indexOf(__clusterDriver) >= 0; + if (__driverVisible !== true) { + if (__driverLoadError !== undefined) { + // Resolution failures carry a kind and are already worded for an + // operator by `createHostImporter`; anything else RESOLVED and + // then crashed while evaluating. Swallowing the second is how a + // driver with a broken dependency reported as "not registered", + // sending operators to look for a package already installed. + const __kind = hostImportFailureKind(__driverLoadError); + if (__kind !== undefined) { + console.warn( + `[cluster] driver "${__clusterDriver}" was requested but could not be ` + + `loaded (${__kind}):\n${ + __driverLoadError instanceof Error + ? __driverLoadError.message + : String(__driverLoadError) + }`, + ); + } else { + console.warn( + `[cluster] driver "${__clusterDriver}" resolved but threw while loading — ` + + `this is the driver package's own failure, not a missing package:`, + __driverLoadError, + ); + } + } else if (__driverVisible === false) { + // Loaded cleanly and still not in the registry: two live + // instances of `@objectstack/service-cluster` in one process, + // which is a PHYSICAL-copy split no resolver condition can merge. + console.warn( + `[cluster] driver "${__clusterDriver}" loaded but did not register: ` + + `@objectstack/service-cluster-${__clusterDriver} evaluated without error, yet the ` + + `registry this boot reads holds [${__registeredDrivers?.join(', ') || 'nothing'}]. ` + + `Two instances of @objectstack/service-cluster are live in this process and the ` + + `driver registered into the other one — look for two physical copies (a version ` + + `skew between the app and the framework, or a bundled one). Importing ` + + `"@objectstack/service-cluster-${__clusterDriver}" from objectstack.config.ts ` + + `registers into the instance the Runtime reads.`, + ); + } + } clusterConfig = { driver: __clusterDriver, url: process.env.OS_REDIS_URL }; } } diff --git a/packages/services/service-cluster/src/cluster-driver-registry.test.ts b/packages/services/service-cluster/src/cluster-driver-registry.test.ts new file mode 100644 index 0000000000..c39cbe5dc0 --- /dev/null +++ b/packages/services/service-cluster/src/cluster-driver-registry.test.ts @@ -0,0 +1,60 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #13330 — the driver registry is READABLE, and what it reads is what + * `defineCluster()` consults. + * + * A driver package's whole contract is a load-time side effect into the + * module-scope `driverRegistry` here. Until now a booting process could only + * discover whether that side effect had landed by calling `defineCluster()` + * and catching the throw — which constructs a real cluster on success, so it + * is not a probe anyone can run first. `os serve` therefore ASSUMED the + * registration, in a silent `catch`, and a shipped EE boot proved the + * assumption wrong: the driver had loaded into a second, CommonJS instance of + * this module, and the ESM Runtime read this one and found nothing. + * + * The accessor exists so that boot can read instead of assume. Its whole value + * rests on agreeing with `defineCluster()` — an accessor that could drift from + * the lookup it reports on would make `serve`'s diagnosis a phantom check — + * so the agreement is pinned here in both directions, not just the shape of + * the list. + */ + +import { describe, it, expect } from 'vitest'; +import type { IClusterService } from '@objectstack/spec/contracts'; +import { defineCluster, listClusterDrivers, registerClusterDriver } from './cluster.js'; + +/** A factory whose product is identifiable without connecting to anything. */ +const marker = { driver: 'fixture-marker' } as unknown as IClusterService; + +describe('the driver registry can be read, not only written (#13330)', () => { + it('CONTROL: the reader can return both answers, so an empty list is a reading', () => { + // Nothing has registered yet in this module instance, and the reader is not + // stuck on that answer — every assertion below depends on it moving. + expect(listClusterDrivers()).toEqual([]); + registerClusterDriver('custom', () => marker); + expect(listClusterDrivers()).toEqual(['custom']); + }); + + it('omits `memory`, which defineCluster special-cases rather than registers', () => { + // A true reading of what the REGISTRY holds. Listing `memory` here would + // make an empty registry look populated to the one caller that needs to + // tell those apart. + expect(listClusterDrivers()).not.toContain('memory'); + expect(defineCluster({ driver: 'memory' }).driver).toBe('memory'); + }); + + it('agrees with defineCluster — listed means resolvable', () => { + expect(listClusterDrivers()).toContain('custom'); + expect(defineCluster({ driver: 'custom' })).toBe(marker); + }); + + it('agrees with defineCluster — unlisted means the documented throw', () => { + // The other direction. `postgres` is accepted by the schema and shipped by + // nobody, which is exactly the "requested but not registered" case. + expect(listClusterDrivers()).not.toContain('postgres'); + expect(() => defineCluster({ driver: 'postgres' })).toThrow( + /Cluster driver "postgres" is not registered/, + ); + }); +}); diff --git a/packages/services/service-cluster/src/cluster.ts b/packages/services/service-cluster/src/cluster.ts index 757545b5b1..92dbd389a9 100644 --- a/packages/services/service-cluster/src/cluster.ts +++ b/packages/services/service-cluster/src/cluster.ts @@ -116,6 +116,29 @@ export function registerClusterDriver( driverRegistry.set(name, factory); } +/** + * The driver names currently in this module instance's registry. + * + * Exported so a boot sequence can READ whether a driver package's load-time + * `registerClusterDriver()` actually landed, instead of assuming it did. + * `defineCluster()` consults this same `Map`, so an answer from here is an + * answer about the call that comes next — which is the whole point (#13330: + * `os serve` loaded a driver that registered into a SECOND, CommonJS instance + * of this module and then failed one line later in `defineCluster` with + * "not registered", with nothing between the two to say so). + * + * `memory` is deliberately absent: it is not registered, it is special-cased + * inside `defineCluster`. This lists what the REGISTRY holds, so an empty array + * is a true and useful reading rather than a misleading one. + * + * A list rather than a `has()` predicate because the caller that needs the + * boolean also needs to print what WAS there when the answer is no — one call, + * both readings, no way for the two to drift. + */ +export function listClusterDrivers(): string[] { + return [...driverRegistry.keys()]; +} + // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- diff --git a/packages/services/service-cluster/src/index.ts b/packages/services/service-cluster/src/index.ts index 1b2c44eb28..8880da25af 100644 --- a/packages/services/service-cluster/src/index.ts +++ b/packages/services/service-cluster/src/index.ts @@ -24,6 +24,7 @@ export { defineCluster, registerClusterDriver, + listClusterDrivers, ComposedClusterService, type ClusterDriverFactory, type DriverFactoryConfig, diff --git a/packages/types/src/node.test.ts b/packages/types/src/node.test.ts index 88fa0a091b..4711dd0f82 100644 --- a/packages/types/src/node.test.ts +++ b/packages/types/src/node.test.ts @@ -30,7 +30,7 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; import * as NodeModule from 'node:module'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; +import { dirname, join } from 'node:path'; import { pathToFileURL } from 'node:url'; import { createHostImporter, @@ -575,3 +575,250 @@ describe('the undeclared fallback resolves from the CALLER (#10943)', () => { expect(withBase.message).not.toMatch(/did not pass `fallbackImport`/); }); }); + +/** + * #13330 — the DECLARED leg used to load the CommonJS build of a dual-published + * package, giving the process a SECOND instance of everything that package + * brings with it. + * + * The defect was invisible to every test that imports things the normal way, + * because "the same module" is only ever one instance in a suite that never + * crosses the seam. It surfaced as a shipped EE boot: `os serve` loaded + * `@objectstack/service-cluster-redis` through this leg, the driver's load-time + * `registerClusterDriver('redis', …)` ran against the CJS copy of + * `@objectstack/service-cluster`, and the ESM Runtime read the ESM copy and + * found nothing — `Cluster driver "redis" is not registered`, about a package + * that was installed, declared and resolvable. + * + * The fixtures below are a miniature of exactly that: a dual-published package + * holding module-scope state, and a second dual-published package whose only + * job is a load-time write into it. What is asserted is the SHARED INSTANCE, + * not the file name — a test that only checked which path was imported would + * pass on a fix that loaded the right file into the wrong instance. + */ +describe('the declared leg loads the `import` build, not the `require` one (#13330)', () => { + const REGISTRY = '@fixture/instance-registry'; + const DRIVER = '@fixture/instance-registry-driver'; + const CJS_ONLY = '@fixture/require-only'; + const SUBPATHS = '@fixture/dual-subpaths'; + + /** Module-scope state, published as both builds — the `tsup` dual-build shape. */ + const REGISTRY_ESM = `export const BUILD = 'esm'; +const registered = []; +export function register(name) { registered.push(name); } +export function listRegistered() { return [...registered]; } +`; + const REGISTRY_CJS = `const registered = []; +exports.BUILD = 'cjs'; +exports.register = (name) => { registered.push(name); }; +exports.listRegistered = () => [...registered]; +`; + /** A driver package: its entire contract is the load-time side effect. */ + const DRIVER_ESM = `import { register } from '${REGISTRY}'; +register('probe'); +export const BUILD = 'esm'; +`; + const DRIVER_CJS = `const { register } = require('${REGISTRY}'); +register('probe'); +exports.BUILD = 'cjs'; +`; + + const roots: string[] = []; + + function writePackage(root: string, name: string, exportsField: unknown, files: Record): void { + const dir = join(root, 'node_modules', ...name.split('/')); + mkdirSync(dir, { recursive: true }); + writeFileSync( + join(dir, 'package.json'), + JSON.stringify({ + name, + version: '0.0.0-fixture', + type: 'module', + main: 'dist/index.js', + exports: exportsField, + }), + 'utf8', + ); + for (const rel of Object.keys(files)) { + const target = join(dir, rel); + mkdirSync(dirname(target), { recursive: true }); + writeFileSync(target, files[rel] as string, 'utf8'); + } + } + + /** + * The condition map `tsup` emits: nested, with `types` first, so the + * resolution under test has to walk INTO the `import` branch rather than + * match a flat string. + */ + const DUAL: unknown = { + '.': { + import: { types: './dist/index.d.ts', default: './dist/index.js' }, + require: { types: './dist/index.d.cts', default: './dist/index.cjs' }, + }, + }; + + /** + * A fresh app per case. The ESM module cache is keyed by absolute URL, so a + * shared fixture directory would let one case's load answer the next one's + * question — the failure mode this whole suite exists to detect. + */ + function app(tag: string): string { + const root = mkdtempSync(join(tmpdir(), `os-instance-split-${tag}-`)); + roots.push(root); + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'dual-build-host-fixture', + type: 'module', + dependencies: { + [REGISTRY]: '0.0.0-fixture', + [DRIVER]: '0.0.0-fixture', + [CJS_ONLY]: '0.0.0-fixture', + [SUBPATHS]: '0.0.0-fixture', + }, + }), + 'utf8', + ); + writePackage(root, REGISTRY, DUAL, { + 'dist/index.js': REGISTRY_ESM, + 'dist/index.cjs': REGISTRY_CJS, + }); + writePackage(root, DRIVER, DUAL, { + 'dist/index.js': DRIVER_ESM, + 'dist/index.cjs': DRIVER_CJS, + }); + // Publishes ONE build, under the `require` condition only. + writePackage(root, CJS_ONLY, { '.': { require: './dist/index.cjs' } }, { + 'dist/index.cjs': "exports.BUILD = 'cjs-only';\n", + }); + writePackage( + root, + SUBPATHS, + { + '.': { import: './dist/index.js', require: './dist/index.cjs' }, + './named': { import: './dist/named.js', require: './dist/named.cjs' }, + './deep/*': { import: './dist/deep/*.js', require: './dist/deep/*.cjs' }, + }, + { + 'dist/index.js': "export const WHERE = 'root-esm';\n", + 'dist/index.cjs': "exports.WHERE = 'root-cjs';\n", + 'dist/named.js': "export const WHERE = 'named-esm';\n", + 'dist/named.cjs': "exports.WHERE = 'named-cjs';\n", + 'dist/deep/leaf.js': "export const WHERE = 'leaf-esm';\n", + 'dist/deep/leaf.cjs': "exports.WHERE = 'leaf-cjs';\n", + }, + ); + return root; + } + + /** The instance an ESM consumer chain holds — what the Runtime reads. */ + function esmInstance(root: string): Promise<{ BUILD: string; listRegistered: () => string[]; register: (n: string) => void }> { + return import( + pathToFileURL(join(root, 'node_modules', ...REGISTRY.split('/'), 'dist', 'index.js')).href + ); + } + + /** The instance a CommonJS load lands in — where the registration used to go. */ + function cjsInstance(root: string): Promise<{ BUILD: string; listRegistered: () => string[]; register: (n: string) => void }> { + return import( + pathToFileURL(join(root, 'node_modules', ...REGISTRY.split('/'), 'dist', 'index.cjs')).href + ); + } + + // No `fallbackImport`: every fixture here is DECLARED, so the undeclared leg + // (the only consumer of that base) is never reached. + const importer = (root: string) => createHostImporter(root); + + afterAll(() => { + for (const dir of roots) rmSync(dir, { recursive: true, force: true }); + }); + + it('CONTROL: the reader can see BOTH answers, so an empty registry is a reading', async () => { + // Every assertion below rests on `listRegistered()` being able to come back + // non-empty. A probe that could only ever return `[]` would make the whole + // describe pass on a broken fix — so it is proved here, on the same + // instrument, before anything is measured with it. + const root = app('control'); + const esm = await esmInstance(root); + const cjs = await cjsInstance(root); + + expect(esm.BUILD).toBe('esm'); + expect(cjs.BUILD).toBe('cjs'); + expect(esm.listRegistered()).toEqual([]); + + // And the two really are separate instances: writing one leaves the other + // untouched. That is the split; without it there would be no defect to fix. + cjs.register('control-probe'); + expect(cjs.listRegistered()).toEqual(['control-probe']); + expect(esm.listRegistered()).toEqual([]); + }); + + it('PRECONDITION: host CJS resolution still answers the `require` condition', () => { + // The cause, pinned separately from the fix. `hostRequire.resolve` is still + // the host-anchored half of the answer and is deliberately unchanged; if a + // future Node stopped returning the `require` entry here, this test says so + // rather than leaving the fix looking like a no-op. + const root = app('precondition'); + expect(createHostRequire(root).resolve(DRIVER)).toMatch(/dist[/\\]index\.cjs$/); + }); + + it('loads the `import` build of a declared package', async () => { + const root = app('import-condition'); + expect((await importer(root)(DRIVER)).BUILD).toBe('esm'); + }); + + it("a driver's load-time registration lands in the instance an ESM caller reads", async () => { + // The defect, stated as its consequence. Before the fix this was `[]`. + const root = app('visible'); + expect((await esmInstance(root)).listRegistered()).toEqual([]); + await importer(root)(DRIVER); + expect((await esmInstance(root)).listRegistered()).toEqual(['probe']); + }); + + it('and no longer lands in the CommonJS instance nothing reads', async () => { + // The other direction: the registration MOVED, it was not duplicated. A + // fix that loaded both builds would satisfy the previous case and still + // leave a process holding two live copies of the package's state. + const root = app('cjs-empty'); + await importer(root)(DRIVER); + expect((await cjsInstance(root)).listRegistered()).toEqual([]); + expect((await esmInstance(root)).listRegistered()).toEqual(['probe']); + }); + + it('a package publishing only a `require` condition still loads', async () => { + // Narrowness. There is no import entry to prefer, so the resolved CJS path + // is used exactly as before — the fix may not turn a working load into a + // failure. + const root = app('cjs-only'); + expect((await importer(root)(CJS_ONLY)).BUILD).toBe('cjs-only'); + }); + + it('a package with no `exports` map at all is untouched', async () => { + // `main` is the only entry such a package publishes and CJS resolution + // already returned it; there is nothing to re-decide. + const root = app('no-exports'); + writeFixturePackage(root, '@fixture/no-exports-map', 'export const BUILD = "main";\n'); + writeFileSync( + join(root, 'package.json'), + JSON.stringify({ + name: 'dual-build-host-fixture', + type: 'module', + dependencies: { '@fixture/no-exports-map': '0.0.0-fixture' }, + }), + 'utf8', + ); + expect((await importer(root)('@fixture/no-exports-map')).BUILD).toBe('main'); + }); + + it('resolves a declared SUBPATH under the import condition', async () => { + const root = app('subpath'); + expect((await importer(root)(SUBPATHS)).WHERE).toBe('root-esm'); + expect((await importer(root)(`${SUBPATHS}/named`)).WHERE).toBe('named-esm'); + }); + + it('resolves a wildcard subpath pattern under the import condition', async () => { + const root = app('pattern'); + expect((await importer(root)(`${SUBPATHS}/deep/leaf`)).WHERE).toBe('leaf-esm'); + }); +}); diff --git a/packages/types/src/node.ts b/packages/types/src/node.ts index de7a89b1da..a9d115355d 100644 --- a/packages/types/src/node.ts +++ b/packages/types/src/node.ts @@ -100,9 +100,9 @@ * `packages/verify` `bootStack`, `packages/qa/dogfood`'s enterprise probe). */ -import { readFileSync } from 'node:fs'; +import { existsSync, readFileSync } from 'node:fs'; import { createRequire } from 'node:module'; -import { join } from 'node:path'; +import { dirname, join, resolve, sep } from 'node:path'; import { pathToFileURL } from 'node:url'; import { isModuleNotFoundError } from './module-not-found.js'; @@ -382,6 +382,234 @@ function unresolvableMessage(declaration: HostDeclaration, cause: unknown): stri ); } +/** + * ── #13330: the DECLARED leg must resolve with ESM semantics ───────────────── + * + * `hostRequire.resolve(pkg)` is a **CommonJS** resolution, and CJS resolution + * answers the `require` condition. Every `tsup` dual build in this repo — and + * essentially every dual build anywhere — publishes + * + * "exports": { ".": { "import": "./dist/index.js", "require": "./dist/index.cjs" } } + * + * so that resolve returns `dist/index.cjs`, and `import()`ing a `.cjs` file + * evaluates the package's **CommonJS** build. Everything that build then + * `require`s is CJS too, all the way down. + * + * The importer's callers are ESM (`packages/cli` is `"type": "module"`), so + * anything they load through their OWN import chain is the ESM build of the + * same package. Loading a package here therefore produced a SECOND instance of + * every module it shares with the caller — with its own module-scope state. + * + * That is not a theoretical difference. `serve` loads a cluster driver through + * this leg; the driver's whole job is the side effect + * `registerClusterDriver('redis', …)` against `@objectstack/service-cluster`'s + * module-scope registry. Measured on the EE image, in one process: + * + * ESM instance: redis REGISTERED <- after a bare import() of the driver + * CJS instance: NOT registered <- after this leg loaded the driver + * + * The Runtime reads the ESM instance, so `OS_CLUSTER_DRIVER=redis` on a + * three-replica deployment died at `defineCluster()` with `Cluster driver + * "redis" is not registered` while the package was installed, declared and + * resolvable. Any module-scope registry crossing this seam has the same defect; + * the cluster driver is simply the one that shipped. + * + * The fix is to select the entry the `import` condition names. There is no + * flagless Node API that resolves a bare specifier against an arbitrary parent + * (`import.meta.resolve`'s parent argument is ignored without + * `--experimental-import-meta-resolve` — measured, see `createHostImporter`), + * so the host-anchored ANSWER still comes from the CJS resolver, and only the + * CONDITION is re-decided here: the CJS-resolved file locates the package on + * disk, and the `import` entry of THAT package is what gets imported. + * + * Deliberately narrow at the RESOLUTION level — no load that works today + * resolves differently unless the package itself publishes a valid, existing + * import-condition target: + * + * - a package with no `exports` map is untouched — CJS resolution already + * returned `main`, which is the only entry it publishes; + * - a package whose `exports` names no import-condition target (CJS-only) is + * untouched, and so is one whose two conditions name the same file; + * - anything unreadable, unresolvable or absent on disk falls back to the + * CJS-resolved path, i.e. to exactly the pre-#13330 behaviour. + * + * That narrowness does NOT extend to EVALUATION: every fallback above keys on + * the `import` target being absent, unreadable or escaping the package root, + * so none of them catches an `import` target that is present and broken. A + * dual-published package whose `import` build throws while its `require` build + * works used to mask that break by silently loading the CJS build; it now + * surfaces it. Surfacing a broken published build is arguably the correct + * reading, but it is a behaviour change, not a no-op. + * + * A residual split is still possible above this seam — an app and a framework + * package holding two PHYSICAL copies of the same package are two instances in + * any module system, and no resolver condition can merge them. That case is not + * silent any more: `serve` reads the registry after the load and reports it + * (`packages/cli/src/commands/serve.ts`, the cluster block). + */ + +/** + * The conditions Node matches on an `import()` here. + * + * MEMBERSHIP, not priority: Node walks an exports object's KEYS in insertion + * order and takes the first that names an active condition, so the manifest + * decides precedence and this set only decides eligibility. `require` is absent + * on purpose — selecting it is the defect above. + */ +const ESM_IMPORT_CONDITIONS: ReadonlySet = new Set([ + 'node-addons', + 'node', + 'import', + 'default', +]); + +/** + * Pick a target from one `exports` node under {@link ESM_IMPORT_CONDITIONS}. + * + * A string is a target; an array is a fallback list (first resolvable wins); + * `null` blocks the subpath; an object is a condition map. Nesting is arbitrary + * (`{ import: { types: …, default: … } }` is the shape `tsup` emits). + */ +function selectImportTarget(node: unknown): string | undefined { + if (typeof node === 'string') return node; + if (Array.isArray(node)) { + for (const alternative of node) { + const hit = selectImportTarget(alternative); + if (hit !== undefined) return hit; + } + return undefined; + } + if (node === null || typeof node !== 'object') return undefined; + for (const entry of Object.entries(node as Record)) { + if (!ESM_IMPORT_CONDITIONS.has(entry[0])) continue; + const hit = selectImportTarget(entry[1]); + if (hit !== undefined) return hit; + } + return undefined; +} + +/** + * Resolve one subpath (`.`, `./node`, `./forms/x`) of an `exports` field to the + * relative target its import condition names. + * + * A map is recognised by its KEYS: exports whose keys all begin with `.` is a + * subpath map, anything else is the root-condition sugar for `"."` — the same + * test Node applies, and the reason `{ "import": …, "require": … }` needs no + * special case here. + */ +function resolveExportsSubpath(exportsField: unknown, subpath: string): string | undefined { + if (exportsField === undefined) return undefined; + + const keys = + typeof exportsField === 'object' && exportsField !== null && !Array.isArray(exportsField) + ? Object.keys(exportsField as Record) + : undefined; + const isSubpathMap = + keys !== undefined && keys.length > 0 && keys.every((key) => key === '.' || key.indexOf('./') === 0); + + if (!isSubpathMap) return subpath === '.' ? selectImportTarget(exportsField) : undefined; + + const map = exportsField as Record; + if (Object.prototype.hasOwnProperty.call(map, subpath)) return selectImportTarget(map[subpath]); + + // Pattern keys (`"./*": "./dist/*.js"`). Node takes the key with the longest + // static prefix, breaking ties on the longest suffix, and substitutes the + // matched span into the target's own `*`. + let best: { prefix: string; suffix: string; target: unknown } | undefined; + for (const entry of Object.entries(map)) { + const star = entry[0].indexOf('*'); + if (star < 0 || entry[0].indexOf('*', star + 1) >= 0) continue; + const prefix = entry[0].slice(0, star); + const suffix = entry[0].slice(star + 1); + if (subpath.indexOf(prefix) !== 0) continue; + if (suffix !== '' && subpath.slice(subpath.length - suffix.length) !== suffix) continue; + if (subpath.length < prefix.length + suffix.length) continue; + if ( + best !== undefined && + (best.prefix.length > prefix.length || + (best.prefix.length === prefix.length && best.suffix.length >= suffix.length)) + ) { + continue; + } + best = { prefix, suffix, target: entry[1] }; + } + if (best === undefined) return undefined; + const matched = subpath.slice(best.prefix.length, subpath.length - best.suffix.length); + const target = selectImportTarget(best.target); + return target === undefined ? undefined : target.split('*').join(matched); +} + +/** + * The directory of the package named `packageName` that owns `resolvedFile`. + * + * Walked up from the resolved entry rather than computed from the specifier, + * because the resolver's answer is a REALPATH: under pnpm that is inside + * `.pnpm/@/node_modules/`, which is exactly the directory + * whose `node_modules` the package's own transitive imports resolve against — + * and exactly what makes one physical copy shared between the app and the + * framework. + */ +function packageRootOf(resolvedFile: string, packageName: string): string | undefined { + let dir = dirname(resolvedFile); + // Bounded on purpose: a package root is a few segments above its entry, and + // an unbounded walk on a broken layout would stat every ancestor up to `/`. + for (let hop = 0; hop < 64; hop += 1) { + try { + const manifest = JSON.parse(readFileSync(join(dir, 'package.json'), 'utf8')) as { + name?: unknown; + }; + // A NESTED manifest — the `{"type":"commonjs"}` marker a dual build drops + // in `dist/` — carries no name, so it is walked THROUGH, not stopped at. + if (manifest.name === packageName) return dir; + } catch { + // Not a manifest, or not readable. Keep walking. + } + const parent = dirname(dir); + if (parent === dir) return undefined; + dir = parent; + } + return undefined; +} + +/** + * The file the `import` condition names for `specifier`, or `undefined` when + * this seam has nothing to change — see the narrowness list in the #13330 note. + * + * @param cjsResolved What `hostRequire.resolve(specifier)` answered. It is the + * host-anchored part of the answer and is never second-guessed here; only the + * CONDITION is re-decided. + */ +function esmEntryForDeclared( + specifier: string, + packageName: string, + cjsResolved: string, +): string | undefined { + const root = packageRootOf(cjsResolved, packageName); + if (root === undefined) return undefined; + + let exportsField: unknown; + try { + exportsField = ( + JSON.parse(readFileSync(join(root, 'package.json'), 'utf8')) as { exports?: unknown } + ).exports; + } catch { + return undefined; + } + // No `exports` map ⇒ nothing to choose between: `main` is the only entry the + // package publishes and CJS resolution already returned it. + if (exportsField === undefined || exportsField === null) return undefined; + + const subpath = + specifier === packageName ? '.' : `.${specifier.slice(packageName.length)}`; + const target = resolveExportsSubpath(exportsField, subpath); + if (typeof target !== 'string' || target.indexOf('./') !== 0) return undefined; + + const entry = resolve(root, target); + // Node refuses an exports target that escapes its package; so does this. + if (entry.indexOf(root + sep) !== 0) return undefined; + return existsSync(entry) ? entry : undefined; +} + /** * Build an importer that loads a package **as the host app declares it**, and * otherwise falls back to the importing package's own resolution. @@ -492,7 +720,12 @@ export function createHostImporter( cause, ); } - return import(pathToFileURL(resolved).href); + // #13330: re-decide the CONDITION, never the host anchor. `resolved` + // stays the authority on WHERE the package is; this asks that package + // which entry an `import()` gets, so the caller's ESM chain and this + // load share one instance of everything the package brings with it. + const entry = esmEntryForDeclared(pkg, declaration.packageName, resolved) ?? resolved; + return import(pathToFileURL(entry).href); } try {