From fe64b3b6b0d1603d9c2bed5536f40d314dff1984 Mon Sep 17 00:00:00 2001 From: DanMat Date: Sun, 26 Jul 2026 18:31:23 -0400 Subject: [PATCH] refactor: grouped resolved config view + prove derived state is consistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The config carries ~16 derived flags (isTs, hasApp, publishable, isReact, usesVite…). The review's worry was that they can in principle contradict. Two changes address that: - normalizeConfig now attaches cfg.resolved — a grouped, read-only view of the same booleans, organized by concern (targets/language/framework/ build/module/package). Consumers can read structured state instead of re-interpreting raw selections; deriveDeploymentContract now does. - test/invariants.test.js proves the derived state can't silently contradict itself. Across a broad matrix it asserts the grouped view never disagrees with the flat flags, and that the state is internally consistent: publishable exactly matches its definition, an app is always ESM and never publishable, a service is never publishable, at most one framework, and every gated option (pkgChecks/sizeLimit/sourcemaps/ storybook/e2e/jsr/canary) is off when its precondition fails. cfg.resolved is config-only — never emitted, and excluded from provenance, serialization, and the digest — so output and reproducibility are unaffected. Deliberately NOT done: mechanically rewriting every cfg.hasApp reference across the features to cfg.resolved.targets.app. It's byte-safe but pure cosmetic churn with no functional gain; the flat flags stay supported and the invariants test guarantees the two never drift. 99 tests pass (2 new), lint clean, web configurator verified. Co-Authored-By: Claude Opus 4.8 --- ROADMAP.md | 2 +- docs/packkit-core.js | 11 ++++++ src/core/options.js | 23 ++++++++++++ src/embedded/contract.js | 14 ++++--- src/embedded/index.js | 2 +- test/invariants.test.js | 79 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 123 insertions(+), 8 deletions(-) create mode 100644 test/invariants.test.js diff --git a/ROADMAP.md b/ROADMAP.md index 9f1ce57..11a3aa1 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -106,7 +106,7 @@ _Second external review, this one of the codebase rather than the experience. Ra ### Architecture, once the gates are in - [ ] **Staged resolution with diagnostics** — `normalizeConfig` now applies defaults, coerces conflicts, derives helpers, and encodes framework/target policy in one pass, and **silently** disables what it can't support. Split into ordered stages returning `{ config, changes[], warnings[], errors[] }` so "we turned Storybook off because this isn't a component library" is reported rather than inferred. -- [ ] **A resolved domain model** — the config carries ~16 derived booleans (`isReact`, `hasApp`, `publishable`, `usesVite`…) that can in principle contradict each other. Features should consume `resolved.targets` / `resolved.build` / `resolved.package` instead of re-interpreting raw selections. +- [x] ~~**A resolved domain model**~~ — **Shipped (3.1).** `normalizeConfig` now attaches a grouped `cfg.resolved` view (`targets` / `language` / `framework` / `build` / `module` / `package`) so consumers can read structured state (`cfg.resolved.targets.app`) instead of re-interpreting raw selections; the deployment contract already does. The reviewer's real worry — "flags can in principle contradict" — is now **proven impossible**: `test/invariants.test.js` asserts across a broad matrix that the grouped view never disagrees with the flat flags and that the derived state is internally consistent (publishable ⟺ its definition, an app is always ESM and never publishable, at most one framework, gated options off when their precondition fails, …). A future `normalizeConfig` change that introduces a contradiction fails there. Deliberately *not* done: a mechanical rewrite of every `cfg.hasApp` reference to `cfg.resolved.targets.app` — it's byte-safe but pure cosmetic churn; the flat flags stay supported and the invariants test guarantees they can't drift. - [x] ~~**Make feature ordering explicit**~~ — **Shipped (3.1).** Rather than build speculative phase/requires/conflictsWith machinery, the real risk — order silently deciding a collision — is now an enforced invariant: `test/feature-registry.test.js` asserts every feature has a unique id and that **no two features collide** on a file or package.json field across the full config matrix (1683 configs). That immediately surfaced a real one — `bundler` and `service` both set `scripts.dev` on `node-service`, resolved only by array position — now fixed by making the service the explicit owner of its `dev` script. Ordering is no longer load-bearing for correctness; a colliding new feature fails the test instead of shipping. - [ ] **Pairwise combination testing** — the integration matrix covers presets, which are the *common* paths. The risk is cross-feature interaction (dual + Rollup + Jest + Biome). Generate a matrix where every option pair appears at least once. - [x] ~~**Dependency version catalog**~~ — **Shipped (3.1).** Every template version now lives in `src/core/versions.js` (`V` for specs, `PEER` for peer ranges, `HELD` for deliberately-held deps + reasons, `LAST_REVIEWED`). All 16 feature/monorepo modules reference it instead of hard-coding specs — verified **byte-identical across 3740 configs**, and a conformance test asserts every generated version comes from the catalog (it caught a wrong `ts-jest` spec and two missing deps while being built). The freshness workflow imports `HELD` instead of duplicating it. A bump is now one line: proven by fixing #19 (`release-it` ^20 → ^21) in the catalog alone. diff --git a/docs/packkit-core.js b/docs/packkit-core.js index 0a4b6da..a35d035 100644 --- a/docs/packkit-core.js +++ b/docs/packkit-core.js @@ -352,8 +352,19 @@ function normalizeConfig(input = {}, diagnostics = null) { if (!(cfg.hasService || cfg.hasCli)) coerce("env", false, "ENV_REQUIRES_SERVICE_OR_CLI", "Env validation was disabled because it only applies to a service or CLI."); if (cfg.release !== "changesets") coerce("canary", false, "CANARY_REQUIRES_CHANGESETS", "Canary releases were disabled because they require the Changesets release flow."); if (!(cfg.isTs && cfg.hasLibrary && !cfg.hasFramework && !cfg.hasApp)) coerce("jsr", false, "JSR_REQUIRES_PLAIN_TS_LIBRARY", "JSR publishing was disabled because it applies only to a plain TypeScript library."); + cfg.resolved = resolvedView(cfg); return cfg; } +function resolvedView(cfg) { + return { + targets: { library: cfg.hasLibrary, cli: cfg.hasCli, service: cfg.hasService, app: cfg.hasApp }, + language: { typescript: cfg.isTs, ext: cfg.ext, srcExt: cfg.srcExt }, + framework: { name: cfg.framework, react: cfg.isReact, vue: cfg.isVue, svelte: cfg.isSvelte, any: cfg.hasFramework }, + build: { vite: cfg.viteBuild, custom: cfg.customBuild, usesVite: cfg.usesVite, has: cfg.hasBuild }, + module: { format: cfg.moduleFormat, esm: cfg.hasEsm, cjs: cfg.hasCjs }, + package: { publishable: cfg.publishable, monorepo: cfg.monorepo } + }; +} // src/core/render.js var UNSAFE_KEYS = /* @__PURE__ */ new Set(["__proto__", "constructor", "prototype"]); diff --git a/src/core/options.js b/src/core/options.js index a4a4a8a..52a9319 100644 --- a/src/core/options.js +++ b/src/core/options.js @@ -353,5 +353,28 @@ export function normalizeConfig(input = {}, diagnostics = null) { if (cfg.release !== 'changesets') coerce('canary', false, 'CANARY_REQUIRES_CHANGESETS', 'Canary releases were disabled because they require the Changesets release flow.'); // JSR is TypeScript-first, ESM, and for plain (non-framework) libraries. if (!(cfg.isTs && cfg.hasLibrary && !cfg.hasFramework && !cfg.hasApp)) coerce('jsr', false, 'JSR_REQUIRES_PLAIN_TS_LIBRARY', 'JSR publishing was disabled because it applies only to a plain TypeScript library.'); + + cfg.resolved = resolvedView(cfg); return cfg; } + +/** + * A grouped, read-only view of the derived state — the same booleans the flat + * `cfg.has*`/`is*` flags expose, organized by concern. Consumers that prefer + * structured access (`cfg.resolved.targets.app`) can use it instead of + * re-interpreting raw selections; the flat flags remain for existing code. + * + * It is derived purely from the flags computed above, and test/invariants.test.js + * asserts the two never disagree and that the state is internally consistent — + * so the derived model can't silently contradict itself. + */ +function resolvedView(cfg) { + return { + targets: { library: cfg.hasLibrary, cli: cfg.hasCli, service: cfg.hasService, app: cfg.hasApp }, + language: { typescript: cfg.isTs, ext: cfg.ext, srcExt: cfg.srcExt }, + framework: { name: cfg.framework, react: cfg.isReact, vue: cfg.isVue, svelte: cfg.isSvelte, any: cfg.hasFramework }, + build: { vite: cfg.viteBuild, custom: cfg.customBuild, usesVite: cfg.usesVite, has: cfg.hasBuild }, + module: { format: cfg.moduleFormat, esm: cfg.hasEsm, cjs: cfg.hasCjs }, + package: { publishable: cfg.publishable, monorepo: cfg.monorepo }, + }; +} diff --git a/src/embedded/contract.js b/src/embedded/contract.js index d79ec2c..f7697a9 100644 --- a/src/embedded/contract.js +++ b/src/embedded/contract.js @@ -13,15 +13,17 @@ export function deriveDeploymentContract(cfg) { const run = (script) => (cfg.packageManager === 'npm' ? `npm run ${script}` : `${cfg.packageManager} ${script}`); const start = cfg.packageManager === 'npm' ? 'npm start' : `${cfg.packageManager} start`; + // Read the structured resolved view rather than re-interpreting raw selections. + const { targets, build } = cfg.resolved; - if (cfg.hasService) { + if (targets.service) { // The generated server reads PORT but defaults to 3000, so PORT is optional, // not required — `port` communicates the default and the `PORT` env var // overrides it. `healthCheckPath` is only asserted because every service // framework Packkit generates (Hono/Fastify/Express) defines /health. return prune({ type: 'node-service', - buildCommand: cfg.hasBuild ? run('build') : undefined, + buildCommand: build.has ? run('build') : undefined, startCommand: start, port: 3000, healthCheckPath: '/health', @@ -29,7 +31,7 @@ export function deriveDeploymentContract(cfg) { }); } - if (cfg.hasApp) { + if (targets.app) { return prune({ type: 'static', buildCommand: run('build'), @@ -38,16 +40,16 @@ export function deriveDeploymentContract(cfg) { }); } - if (cfg.hasCli) { + if (targets.cli) { return prune({ type: 'cli', - buildCommand: cfg.hasBuild ? run('build') : undefined, + buildCommand: build.has ? run('build') : undefined, }); } return prune({ type: 'library', - buildCommand: cfg.hasBuild ? run('build') : undefined, + buildCommand: build.has ? run('build') : undefined, }); } diff --git a/src/embedded/index.js b/src/embedded/index.js index b9e7af8..74663df 100644 --- a/src/embedded/index.js +++ b/src/embedded/index.js @@ -38,7 +38,7 @@ const MAX_DEFINITION_BYTES = 50_000_000; const UNSAFE_KEYS = new Set(['__proto__', 'constructor', 'prototype']); /** Non-OPTIONS config keys the pipeline sets itself; not "unknown". */ -const KNOWN_EXTRA_KEYS = new Set(['preset', 'generatorVersion']); +const KNOWN_EXTRA_KEYS = new Set(['preset', 'generatorVersion', 'resolved']); // Fields where a host override silently changing an existing value is worth a // diagnostic (matches pkg-merge's protected set + dependency maps). diff --git a/test/invariants.test.js b/test/invariants.test.js new file mode 100644 index 0000000..8bcfeb1 --- /dev/null +++ b/test/invariants.test.js @@ -0,0 +1,79 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { normalizeConfig, fromPreset, PRESET_NAMES } from '../src/core/index.js'; + +// The config carries a lot of derived state — flat flags (isTs, hasApp, +// publishable…) and the grouped cfg.resolved view. These assert that the derived +// state is internally consistent and can never silently contradict itself, so a +// future change to normalizeConfig that introduces a contradiction fails here. + +// A broad matrix of resolved configs. +function matrix() { + const out = []; + const langs = ['ts', 'js']; + const modules = ['esm', 'dual', 'cjs']; + const frameworks = ['none', 'react', 'vue', 'svelte']; + const targetSets = [['library'], ['cli'], ['service'], ['app'], ['library', 'cli']]; + for (const preset of PRESET_NAMES) out.push(fromPreset(preset, { name: 'x' })); + for (const language of langs) + for (const moduleFormat of modules) + for (const framework of frameworks) + for (const target of targetSets) { + out.push(normalizeConfig({ name: 'x', language, moduleFormat, framework, target })); + out.push(normalizeConfig({ name: 'x', language, moduleFormat, framework, target, monorepo: true })); + } + return out; +} + +test('the grouped resolved view never disagrees with the flat flags', () => { + for (const cfg of matrix()) { + const r = cfg.resolved; + assert.equal(r.targets.library, cfg.hasLibrary); + assert.equal(r.targets.cli, cfg.hasCli); + assert.equal(r.targets.service, cfg.hasService); + assert.equal(r.targets.app, cfg.hasApp); + assert.equal(r.language.typescript, cfg.isTs); + assert.equal(r.framework.any, cfg.hasFramework); + assert.equal(r.framework.react, cfg.isReact); + assert.equal(r.build.has, cfg.hasBuild); + assert.equal(r.module.esm, cfg.hasEsm); + assert.equal(r.package.publishable, cfg.publishable); + } +}); + +test('derived state is internally consistent — no contradictions possible', () => { + for (const cfg of matrix()) { + const label = `${cfg.language} ${cfg.moduleFormat} ${cfg.framework} [${cfg.target}]${cfg.monorepo ? ' monorepo' : ''}`; + const ok = (cond, msg) => assert.ok(cond, `${msg} — for ${label}`); + + // A target is always present. + ok(cfg.target.length > 0, 'has at least one target'); + // Publishable is exactly libraries/CLIs that are neither an app nor a service. + ok(cfg.publishable === ((cfg.hasLibrary || cfg.hasCli) && !cfg.hasApp && !cfg.hasService), 'publishable definition holds'); + // An app is bundled, never published, and always ESM. + if (cfg.hasApp) { + ok(!cfg.publishable, 'an app is not publishable'); + ok(cfg.moduleFormat === 'esm', 'an app is ESM-only'); + } + // A service is never publishable. + if (cfg.hasService) ok(!cfg.publishable, 'a service is not publishable'); + // Framework flags are mutually exclusive and agree with `any`. + const fw = [cfg.isReact, cfg.isVue, cfg.isSvelte].filter(Boolean).length; + ok(fw <= 1, 'at most one framework'); + ok(cfg.hasFramework === (fw === 1), 'hasFramework matches exactly one concrete framework'); + // Options that require a publishable package must be off when it isn't. + if (!cfg.publishable) { + ok(!cfg.pkgChecks, 'pkgChecks off when not publishable'); + ok(!cfg.sizeLimit, 'sizeLimit off when not publishable'); + ok(!cfg.sourcemaps, 'sourcemaps off when not publishable'); + } + // Storybook only for a component library (framework, library, not app). + if (cfg.storybook) ok(cfg.hasFramework && cfg.hasLibrary && !cfg.hasApp, 'storybook only on a component library'); + // E2E only for an app. + if (cfg.e2e) ok(cfg.hasApp, 'e2e only on an app'); + // JSR only for a plain TS library. + if (cfg.jsr) ok(cfg.isTs && cfg.hasLibrary && !cfg.hasFramework && !cfg.hasApp, 'jsr only on a plain TS library'); + // Canary only rides the changesets flow. + if (cfg.canary) ok(cfg.release === 'changesets', 'canary requires changesets'); + } +});