diff --git a/.changeset/18419-config-shadowed-named-export-reported.md b/.changeset/18419-config-shadowed-named-export-reported.md new file mode 100644 index 00000000000..dc87d03c0a0 --- /dev/null +++ b/.changeset/18419-config-shadowed-named-export-reported.md @@ -0,0 +1,20 @@ +--- +"@objectstack/cli": minor +--- + +fix(cli)!: a named export the config's default export already declares is reported instead of silently dropped (#18419) + + + +`objectstack.config.ts` is loaded as a module: `loadConfig()` takes the default export as the base and merges every named export onto it as a top-level stack key. A named export whose name the default export **already carries** loses — the default's value wins — and until now it lost in complete silence. `os build` exited 0, the artifact carried the default's value, and nothing was written at any level: + +```ts +export default defineStack({ manifest, objects: [Task] }); +export const objects = [Task, Invoice]; // Invoice never reached the artifact +``` + +The loader now says so on stderr, names every shadowed key, and states the rule and the remedy. It is an **advisory, not a refusal** — the stack that comes out is valid, it is merely missing what the shadowed export carried — which is the disposition this package already gives the same failure class (`#3786`'s undeclared authoring keys are "advisory, never fatal"; `#4095`'s orphaned runtime members are "reported rather than dropped"). It goes to stderr rather than stdout because `loadConfig()` is handed no `--json` flag and twelve commands call it, so a `--json` run's stdout stays a single parseable document. `LoadedConfig.shadowedNamedExports` carries the same names structurally. + +**BREAKING** in the accept-set sense, landing in the launch window as `minor` (the lockstep convention: `major` is refused by `check-changeset-no-major`, and breaking-ness is carried by this banner plus the ADR-0087 disposition above): the collision test now reads **own keys only**. `key in merged` walked the prototype chain, so every `Object.prototype` member — `toString`, `valueOf`, `constructor`, `hasOwnProperty`, `propertyIsEnumerable`, `toLocaleString`, `isPrototypeOf` — was treated as a key the default export "already carries" when the default carries no such key at all. Such an export was skipped by the merge and therefore never reached the strict parse that refuses an undeclared stack key by name, so `export const toString = …` beside a valid stack built green while `export const collectPackageDirs = …` was refused. That hole is closed: those names now merge like any other and are refused by name, the same sentence every other undeclared helper export has always got. + +Nobody's metadata or stored data changes. A config affected by the narrowing was already shipping that export's value nowhere; what changes is that the build now says so instead of exiting 0. Move the helper into a sibling module and import it, which is what the config-authoring docs have always prescribed for a helper exported beside the stack. diff --git a/content/docs/deployment/cli.mdx b/content/docs/deployment/cli.mdx index bf7bb480570..b8497cd3208 100644 --- a/content/docs/deployment/cli.mdx +++ b/content/docs/deployment/cli.mdx @@ -2063,10 +2063,21 @@ recognisable rather than as patterns to use: |:---|:---| | not a declared stack key | the build **fails**, naming the key (above) | | a declared stack key the default export does **not** carry | merged in and **accepted** — the `onEnable` / `functions` path | -| a key the default export **already** carries | dropped **silently**; the build exits 0 and the exported value is never read | +| a key the default export **already** carries | the default's value wins and the exported one is dropped — the build still exits 0, and the drop is **reported on stderr** | The last row is why every stack key belongs inside `defineStack()`: a second -copy beside it is not a second declaration. +copy beside it is not a second declaration, it is a value nothing reads. + +```console +$ printf '\nexport const objects = [myExtraObject];\n' >> objectstack.config.ts +$ os build + ⚠ `objects` is a named export that was DROPPED — the default-exported stack already declares that key +``` + +The advisory goes to **stderr**, so it reaches a `--json` run's operator without +putting anything but the envelope on stdout. It does not fail the build: the +stack it produces is valid, it is simply missing what the shadowed export +carried. ### Config File Auto-Detection diff --git a/content/docs/getting-started/your-first-project.mdx b/content/docs/getting-started/your-first-project.mdx index e49536f2c50..26f9763498e 100644 --- a/content/docs/getting-started/your-first-project.mdx +++ b/content/docs/getting-started/your-first-project.mdx @@ -154,10 +154,12 @@ them a pattern to rely on: - a named export whose name **is** a declared stack key that the default export does not carry is merged in and accepted — that is the `onEnable` / `functions` path; -- a named export whose name the default export **already carries** is dropped - **silently**, and the build still succeeds. A second - `export const objects = [...]` beside `defineStack({ objects })` is therefore - a value nothing reads. Write every stack key inside `defineStack()`. +- a named export whose name the default export **already carries** loses to the + default, and the build still succeeds — but the loader now **says so on + stderr**, naming the key. A second `export const objects = [...]` beside + `defineStack({ objects })` is still a value nothing reads; it is no longer a + value nothing reads *and nothing mentions*. Write every stack key inside + `defineStack()`. diff --git a/packages/cli/src/utils/config-shadowed-named-export.test.ts b/packages/cli/src/utils/config-shadowed-named-export.test.ts new file mode 100644 index 00000000000..115e587d2e8 --- /dev/null +++ b/packages/cli/src/utils/config-shadowed-named-export.test.ts @@ -0,0 +1,236 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A named export the default-exported stack already declares is DROPPED — and + * the drop is now reported instead of silent (#18419). + * + * ── The finding ────────────────────────────────────────────────────────── + * + * `loadConfig()` merges every named export of `objectstack.config.ts` onto the + * default export as a top-level stack key. A name the default already carries + * was skipped by `if (key === 'default' || key in merged) continue`, so: + * + * export default defineStack({ manifest, objects: [] }); + * export const objects = [oneRow]; // <- never reaches anything + * + * built to exit 0, wrote an artifact whose `objects` is the default's `[]`, and + * logged NOTHING at any level. Authored content vanished on the success path. + * + * ── The positive control this file carries ─────────────────────────────── + * + * The same channel is LOUD for a named export the stack schema does not + * declare: `export const ProbeNamedExport = [1,2,3]` is merged in, refused by + * the strict parse and named in the message (#18171, pinned next door in + * `config-named-export-rule.test.ts`). So silence was specific to this one arm + * rather than a property of the loader — which is why the first two pins below + * assert the UNCHANGED rows. A pin that only proved "a warning appears" could + * not tell a repaired loader from one that warns about everything. + * + * ── The second arm, measured while sweeping for the first ──────────────── + * + * `key in merged` walks the PROTOTYPE chain, so `Object.prototype`'s members + * answered true for a default export that carries no such key at all. An + * `export const toString = …` was therefore skipped by the collision arm and + * never reached the strict parse that would have refused it by name — the loud + * refusal above, silently turned off by the spelling of the key. `loadConfig` + * now tests own keys only, so that row rejoins the control. + * + * ── Disposition: advisory, not refusal ─────────────────────────────────── + * + * Read off this package's own repairs of this class — #3786's undeclared + * authoring keys ("Advisory, never fatal") and #4095's orphaned runtime members + * ("reported rather than dropped") — and recorded in full on + * {@link shadowedNamedExportWarning}. The stack that comes out is valid; it is + * merely missing what the shadowed export carried, so the run continues and the + * author is told. The accept set therefore moves for the prototype-chain row + * only, and in the narrowing direction. + */ + +import { describe, it, expect, afterAll, vi } from 'vitest'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { ObjectStackDefinitionSchema } from '@objectstack/spec'; + +import { loadConfig, shadowedNamedExportWarning } from './config.js'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); +/** `packages/cli/tmp` — throwaway projects, the placement sibling suites use. */ +const TMP_ROOT = path.resolve(HERE, '..', '..', 'tmp'); + +/** The card's own probe export — a name the stack schema does not declare. */ +const PROBE = 'ProbeNamedExport'; + +const MANIFEST = `{ + id: 'com.example.probe', + namespace: 'probe', + version: '0.1.0', + type: 'app', + name: 'Probe', + engines: { protocol: '^17' }, + }`; + +const roots: string[] = []; +afterAll(() => { + for (const dir of roots) fs.rmSync(dir, { recursive: true, force: true }); +}); + +function writeConfig(tag: string, body: string): string { + fs.mkdirSync(TMP_ROOT, { recursive: true }); + const dir = fs.mkdtempSync(path.join(TMP_ROOT, `shadowed-${tag}-`)); + roots.push(dir); + const file = path.join(dir, 'objectstack.config.ts'); + fs.writeFileSync(file, body); + return file; +} + +/** Load `body`, capturing whatever the loader wrote to each stream. */ +async function loadCapturing(tag: string, body: string) { + const err: string[] = []; + const out: string[] = []; + const errSpy = vi.spyOn(console, 'error').mockImplementation((...a) => { err.push(a.join(' ')); }); + const outSpy = vi.spyOn(console, 'log').mockImplementation((...a) => { out.push(a.join(' ')); }); + try { + const loaded = await loadConfig(writeConfig(tag, body)); + return { ...loaded, stderr: err.join('\n'), stdout: out.join('\n') }; + } finally { + errSpy.mockRestore(); + outSpy.mockRestore(); + } +} + +describe('#18419 — a named export the default already declares is dropped, and said so', () => { + it('CONTROL: an undeclared named export is still refused by name, and is not a "drop"', async () => { + const loaded = await loadCapturing('control', `import { defineStack } from '@objectstack/spec'; + +export default defineStack({ + manifest: ${MANIFEST}, +}); + +export const ${PROBE} = [1, 2, 3]; +`); + + // Merged, therefore reaches the parse, therefore refused — the property + // #18171 pinned and this change must not spend. + expect(loaded.namedExports).toEqual([PROBE]); + expect(loaded.shadowedNamedExports).toEqual([]); + const result = ObjectStackDefinitionSchema.safeParse(loaded.config); + expect(result.success).toBe(false); + if (result.success) return; + const unrecognized = result.error.issues.filter((i) => i.code === 'unrecognized_keys'); + expect((unrecognized[0] as unknown as { keys: string[] }).keys).toEqual([PROBE]); + + // Nothing was dropped, so nothing is announced. "Reported" has to be + // distinguishable from "always reported". + expect(loaded.stderr).not.toContain('DROPPED'); + }, 60_000); + + it('CONTROL: a declared key the default does NOT carry is still merged, silently and legally', async () => { + const loaded = await loadCapturing('accepted', `import { defineStack } from '@objectstack/spec'; + +export default defineStack({ + manifest: ${MANIFEST}, +}); + +export const objects = []; +`); + + expect(loaded.namedExports).toEqual(['objects']); + expect(loaded.shadowedNamedExports).toEqual([]); + expect(ObjectStackDefinitionSchema.safeParse(loaded.config).success).toBe(true); + expect(loaded.stderr).toBe(''); + }, 60_000); + + it('the collision is RECORDED and ANNOUNCED — the silence this card is about', async () => { + const loaded = await loadCapturing('collision', `import { defineStack } from '@objectstack/spec'; + +export default defineStack({ + manifest: ${MANIFEST}, + objects: [], +}); + +export const objects = [{ name: 'probe_row', label: 'Probe Row', fields: { name: { type: 'text', label: 'Name' } } }]; +`); + + // The drop itself is unchanged — the default's value still wins… + expect(loaded.config.objects).toEqual([]); + expect(loaded.namedExports).toEqual([]); + // …and it is no longer invisible. + expect(loaded.shadowedNamedExports).toEqual(['objects']); + expect(loaded.stderr).toContain('objects'); + expect(loaded.stderr).toContain('DROPPED'); + // The rule and the remedy, not just the fact. + expect(loaded.stderr).toContain('loaded as a MODULE'); + expect(loaded.stderr).toContain('defineStack'); + + // Advisory, never fatal: the stack that comes out is still valid. + expect(ObjectStackDefinitionSchema.safeParse(loaded.config).success).toBe(true); + }, 60_000); + + it('…including on `functions` — the runtime member an app really does author', async () => { + const loaded = await loadCapturing('functions', `import { defineStack } from '@objectstack/spec'; + +export default defineStack({ + manifest: ${MANIFEST}, + functions: { fromDefault: () => 'default' }, +}); + +export const functions = { fromNamedExport: () => 'named' }; +`); + + expect(loaded.shadowedNamedExports).toEqual(['functions']); + expect(Object.keys(loaded.config.functions)).toEqual(['fromDefault']); + // The handler that vanished is named, because that is the one the author + // has to go looking for. + expect(loaded.stderr).toContain('functions'); + }, 60_000); + + it('a name that is only on Object.prototype is NOT a collision — it rejoins the control', async () => { + const loaded = await loadCapturing('proto', `import { defineStack } from '@objectstack/spec'; + +export default defineStack({ + manifest: ${MANIFEST}, +}); + +export const toString = [1, 2, 3]; +`); + + // The default export carries no `toString` of its own, so nothing shadows + // this — it is an undeclared stack key like any other, and goes the loud way. + expect(loaded.shadowedNamedExports).toEqual([]); + expect(loaded.namedExports).toEqual(['toString']); + const result = ObjectStackDefinitionSchema.safeParse(loaded.config); + expect(result.success).toBe(false); + if (result.success) return; + const unrecognized = result.error.issues.filter((i) => i.code === 'unrecognized_keys'); + expect((unrecognized[0] as unknown as { keys: string[] }).keys).toContain('toString'); + }, 60_000); + + it('the advisory goes to stderr only — a --json run keeps stdout parseable', async () => { + const loaded = await loadCapturing('streams', `import { defineStack } from '@objectstack/spec'; + +export default defineStack({ + manifest: ${MANIFEST}, + objects: [], +}); + +export const objects = [{ name: 'probe_row', label: 'Probe Row', fields: { name: { type: 'text', label: 'Name' } } }]; +`); + + expect(loaded.stderr).toContain('DROPPED'); + // `loadConfig` is handed no `--json` flag, so the one channel it must never + // write to is the one the machine reads. + expect(loaded.stdout).toBe(''); + }, 60_000); + + it('the warning names every key, and says what to do instead', () => { + const one = shadowedNamedExportWarning(['objects']).join('\n'); + expect(one).toContain('`objects`'); + expect(one).toContain('is a named export that was DROPPED'); + expect(one).toContain('move the value inside defineStack'); + + const many = shadowedNamedExportWarning(['objects', 'functions']).join('\n'); + expect(many).toContain('`objects`, `functions`'); + expect(many).toContain('are named exports that were DROPPED'); + }); +}); diff --git a/packages/cli/src/utils/config.ts b/packages/cli/src/utils/config.ts index d0043a8fedb..d57e6434aa6 100644 --- a/packages/cli/src/utils/config.ts +++ b/packages/cli/src/utils/config.ts @@ -7,7 +7,7 @@ import { pathToFileURL } from 'node:url'; import chalk from 'chalk'; import { bundleRequire } from 'bundle-require'; import type { Plugin } from 'esbuild'; -import { printErrorToStderr } from './format.js'; +import { printErrorToStderr, printWarningToStderr } from './format.js'; export interface LoadedConfig { config: any; @@ -25,6 +25,20 @@ export interface LoadedConfig { * them apart and the parse that can runs several steps later. */ namedExports: readonly string[]; + + /** + * The module's named exports the merge DROPPED, in module order, because the + * default-exported stack already declares that key — the {@link namedExports} + * mirror, and empty for every config that authors each stack key once. + * + * These names reached no artifact and no parse. That is the whole reason the + * field exists: a merged key is answerable downstream (the strict parse sees + * it, {@link namedExportRejectionHints} explains it), whereas a dropped one is + * visible nowhere but here, so a caller that wants to say anything about it + * has to be handed it. {@link loadConfig} also reports them on stderr, so the + * finding does not depend on a caller opting in. + */ + shadowedNamedExports: readonly string[]; } /** @@ -373,11 +387,13 @@ export interface LoadConfigOptions { * carry is merged in and ACCEPTED — that is the `onEnable` / `functions` * path, and nothing distinguishes a deliberate hook from a stray export * that happens to collide with a collection name; - * - a named export whose name the default export ALREADY carries is dropped - * **silently** (`if (key in merged) continue`) and the build exits 0. So a - * second `export const objects = [...]` beside a `defineStack({ objects })` - * is not a second declaration, it is a value nothing ever reads. Keep every - * stack key inside `defineStack()`. + * - a named export whose name the default export ALREADY carries is still + * dropped — the default's value wins — but it is no longer dropped + * **silently** (#18419). A second `export const objects = [...]` beside a + * `defineStack({ objects })` is not a second declaration, it is a value + * nothing ever reads, and the loader now says so on stderr and records it in + * {@link LoadedConfig.shadowedNamedExports}. Keep every stack key inside + * `defineStack()`. */ export async function loadConfig(source?: string, options?: LoadConfigOptions): Promise { const absolutePath = resolveConfigPath(source); @@ -407,23 +423,46 @@ export async function loadConfig(source?: string, options?: LoadConfigOptions): // the refusal several steps downstream can say where the key came from; it is // a reading, and changes nothing about which configs load. const namedExports: string[] = []; + const shadowedNamedExports: string[] = []; const config = (baseConfig === mod || mod.default == null) ? baseConfig : (() => { const merged: any = { ...baseConfig }; for (const key of Object.keys(mod)) { - if (key === 'default' || key in merged) continue; + if (key === 'default') continue; + // ⛔ `hasOwnProperty`, never `key in merged` (#18419). `in` walks the + // PROTOTYPE chain, so every `Object.prototype` member answered true + // for a default export carrying no such key at all: `export const + // toString = …` was skipped here and therefore never reached the + // strict parse that refuses an undeclared stack key by name. Measured + // on this tree, that turned the loud refusal `ProbeNamedExport` gets + // into an exit-0 build — a spelling-dependent hole in the rule this + // function's header states, not a property anything wanted. + if (Object.prototype.hasOwnProperty.call(merged, key)) { + shadowedNamedExports.push(key); + continue; + } merged[key] = (mod as any)[key]; namedExports.push(key); } return merged; })(); + // The drop is REPORTED, never swallowed — see {@link shadowedNamedExportWarning} + // for why it is an advisory on stderr rather than a refusal, and why it is + // rendered here rather than by each of the twelve commands that load a config. + if (shadowedNamedExports.length > 0) { + const [headline, ...hints] = shadowedNamedExportWarning(shadowedNamedExports); + printWarningToStderr(headline); + for (const hint of hints) console.error(chalk.dim(hint)); + } + return { config, absolutePath, duration: Date.now() - start, namedExports, + shadowedNamedExports, }; } @@ -512,6 +551,71 @@ export function namedExportRejectionHints( ]; } +/** + * The advisory {@link loadConfig} prints when the module/stack merge DROPPED a + * named export because the default-exported stack already declares that key + * (#18419). + * + * ## What was wrong + * + * The drop itself is correct — one key, one value, and the default export is + * the base. What was wrong is that it happened **silently**: `os build` exited + * 0, the artifact carried the default's value, and the authored export reached + * no log at any level. An author who wrote `export const objects = [row]` + * beside a `defineStack({ objects })` shipped an artifact without `row` in it + * and had nothing to read that said so. + * + * ## Why an advisory and not a refusal + * + * Decided from this package's own repairs of this exact class, not from taste: + * + * - **#3786 / #11643 — "Undeclared authoring keys — dropped at load".** The + * same shape (a build that exits 0 while quietly dropping an authored + * value), and `compile.ts` states the disposition in its own comment: + * *"Advisory, never fatal."* It is surfaced on the text face and in the + * `--json` `warnings` payload, never by failing the run. + * - **#4095 — `graftRuntimeMembers`' `orphaned`.** An authored `onEnable` that + * finds no bundle to land on is "reported rather than dropped, which is the + * failure mode that made this invisible for so long" — `os serve` prints + * `⚠ … exports onEnable but no app bundle claimed it` and keeps serving. + * - **#18171 — the sibling half of this very loop.** It added the explanation + * for a merged-then-refused key and spent nothing on the accept set. + * + * A refusal would also have to live in {@link loadConfig} to reach every face, + * and two of those faces are the ones that exist to read a config the current + * schema is unhappy with: `os doctor` diagnoses broken projects, and + * `os migrate meta` is entitled to read PAST a rejection + * ({@link authoredSourcePlugin}) — a loader-level throw is not something its + * `authoredSource` option can shim away. Refusing here would close the upgrade + * path against exactly the legacy configs a collision is most likely to sit in. + * + * ## Why it renders on stderr, from the loader + * + * {@link loadConfig} is handed no `--json` flag (this file's + * {@link resolveConfigPath} header states the same fact for the same reason), + * and twelve commands call it. Printing from the loader is what puts the + * finding on all twelve faces at once instead of the two that happen to handle + * named exports today; sending it to **stderr** is what keeps a `--json` run's + * stdout a single parseable document — the shape {@link refuseConfig} already + * established here, minus the throw. See {@link printWarningToStderr}. + * + * @param shadowed {@link LoadedConfig.shadowedNamedExports}, non-empty + * @returns the headline first, then the dim lines printed under it + */ +export function shadowedNamedExportWarning(shadowed: readonly string[]): string[] { + const one = shadowed.length === 1; + const list = shadowed.map((k) => `\`${k}\``).join(', '); + return [ + `${list} ${one ? 'is a named export that was DROPPED' : 'are named exports that were DROPPED'} ` + + `— the default-exported stack already declares ${one ? 'that key' : 'those keys'}`, + ' The config file is loaded as a MODULE: every named export is merged onto the default-exported', + ' stack as a top-level key, and a key the default export already carries KEEPS the default value,', + ` so the exported ${one ? 'value is' : 'values are'} read by nothing — not this build, not any other.`, + ` Fix: declare each stack key once — move the value inside defineStack({ … }), or delete the ` + + `named export${one ? '' : 's'}.`, + ]; +} + /** * Check whether a file exists at the given path (relative to cwd). */ diff --git a/packages/cli/src/utils/format.ts b/packages/cli/src/utils/format.ts index e3dda5c853a..1682cd9a11b 100644 --- a/packages/cli/src/utils/format.ts +++ b/packages/cli/src/utils/format.ts @@ -333,8 +333,31 @@ export function printSuccess(msg: string) { console.log(chalk.green(` ✓ ${msg}`)); } +/** The ` ⚠ ` line both warning printers render — one glyph, one source. */ +function warningLine(msg: string): string { + return chalk.yellow(` ⚠ ${msg}`); +} + export function printWarning(msg: string) { - console.log(chalk.yellow(` ⚠ ${msg}`)); + console.log(warningLine(msg)); +} + +/** + * {@link printWarning}'s line, on **stderr** — the advisory counterpart of + * {@link printErrorToStderr}, for the same reason and the same callers. + * + * A shared helper that drops authored input has to say so, and it has no flag + * to branch on: `loadConfig()` is handed no `--json` (its own header says so), + * so the choice is stdout — which `--json` reserves for the machine — or + * stderr. A non-fatal finding cannot take {@link printErrorToStderr}'s `✗` + * without reading as a failed run, so the warning severity gets its own door + * rather than borrowing the error one. + * + * ⚠️ Not a general replacement for {@link printWarning}: a command that has + * already decided it is rendering the text face keeps writing to stdout. + */ +export function printWarningToStderr(msg: string) { + console.error(warningLine(msg)); } /** The ` ✗ ` line both error printers render — one glyph, one source. */