diff --git a/scripts/__tests__/check-node-esm-load.test.ts b/scripts/__tests__/check-node-esm-load.test.ts index a7c0a3dbc..d644c6667 100644 --- a/scripts/__tests__/check-node-esm-load.test.ts +++ b/scripts/__tests__/check-node-esm-load.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; +import { fileURLToPath } from 'node:url'; import { ESM_MODULE_EDGE, @@ -16,10 +17,12 @@ import { emittedSources, esmEntryOf, importEntry, + readTsconfig, relativeSpecifiers, resolvesToModule, scanSpecifiers, } from '../check-node-esm-load.mjs'; +import { SKIP_DIRS } from '../check-phantom-dependencies.mjs'; /** * objectui#4538 — a published entry plain Node ESM cannot load. @@ -199,6 +202,182 @@ describe('scope — which builds preserve specifiers', () => { }); }); +/** + * objectui#5367 — the tsconfig reader was the same class as the specifier mask. + * + * `readTsconfig` stripped comments with three ordered regexes and handed the + * result to `JSON.parse`. None of them knew what a JSON string was, so the + * slash-star inside a `paths` key opened a "block comment" that ran to the next + * star-slash — typically the test glob in `exclude` at the bottom of the same + * file — and deleted everything between. Measured before the fix: 61 of this + * repository's 91 tsconfigs threw, and every throw was swallowed by + * `effectiveNoEmit`'s catch, which grades an unreadable config as emitting. + * + * The gate's verdict happened to be safe. It was also never computed, which is + * the property this block exists to keep from coming back. + */ + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const TSCONFIG_NAME = /^tsconfig(\..+)?\.json$/; + +/** + * The slice of a parsed tsconfig these assertions read. + * + * `readTsconfig` returns arbitrary JSON, so the narrowing is written down here + * once rather than asserted inline at every call — and it stays `unknown` at the + * leaves, so a wrong assumption about a value's shape is still a type error. + */ +type ParsedTsconfig = { + compilerOptions?: Record; + extends?: unknown; + include?: unknown; + exclude?: unknown; +}; + +const parseTsconfig = (file: string) => readTsconfig(file) as ParsedTsconfig; + +/** + * Every tsconfig in the repository, walked rather than listed. + * + * A hand-maintained list would answer "the files someone remembered", which is + * exactly the reading that let two poisoned configs sit unnoticed. `SKIP_DIRS` + * is the sibling gates' own exclusion set, so `node_modules` and build output + * are out for the same reason they are out everywhere else. + */ +function everyTsconfig(dir: string, out: string[] = []): string[] { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + if (entry.isDirectory()) { + if (SKIP_DIRS.has(entry.name) || entry.name.startsWith('.')) continue; + everyTsconfig(path.join(dir, entry.name), out); + } else if (TSCONFIG_NAME.test(entry.name)) { + out.push(path.join(dir, entry.name)); + } + } + return out; +} + +describe('readTsconfig reads tsconfigs rather than pattern-matching around them', () => { + it('round-trips EVERY tsconfig in the repository without throwing', () => { + const configs = everyTsconfig(REPO_ROOT); + + // The size assertion is the same doctrine the gate applies to itself: a walk + // that found nothing would throw nothing and report success. 91 exist today; + // 60 is a floor that a real collapse trips and ordinary churn does not. + expect(configs.length).toBeGreaterThanOrEqual(60); + + const threw: string[] = []; + for (const file of configs) { + try { + readTsconfig(file); + } catch (error) { + threw.push(`${path.relative(REPO_ROOT, file)}: ${(error as Error).message}`); + } + } + expect(threw).toEqual([]); + }); + + it('keeps a `paths` key whose value opens a comment, and the keys after it', () => { + // The exact two lines that poisoned `packages/auth/tsconfig.json`: the + // slash-star in the paths key, and the test glob in `exclude` that closed + // the comment it opened. The old strip returned a config with no `paths`, + // no `include`, and a truncated `exclude`. + const dir = tmpdir(); + fs.writeFileSync( + path.join(dir, 'tsconfig.json'), + [ + '{', + ' "compilerOptions": {', + ' "baseUrl": ".",', + ' "paths": { "@/*": ["src/*"] },', + ' "noEmit": false', + ' },', + ' "include": ["src"],', + ' "exclude": ["node_modules", "dist", "**/*.test.ts", "**/*.test.tsx"]', + '}', + '', + ].join('\n'), + ); + + const config = parseTsconfig(path.join(dir, 'tsconfig.json')); + expect(config.compilerOptions?.paths).toEqual({ '@/*': ['src/*'] }); + expect(config.include).toEqual(['src']); + expect(config.exclude).toEqual(['node_modules', 'dist', '**/*.test.ts', '**/*.test.tsx']); + expect(config.compilerOptions?.noEmit).toBe(false); + }); + + it('keeps the config below a line comment whose prose contains a slash-star', () => { + // `packages/fields/tsconfig.json`'s shape: a `//` comment naming a package + // glob. The block-comment pass ran first and had no notion of being inside a + // line comment, so the glob opened a comment that ate the rest of the file — + // which is how a config that plainly inherits `noEmit: true` was graded as + // emitting for as long as this gate has existed. + const dir = tmpdir(); + fs.writeFileSync(path.join(dir, 'base.json'), JSON.stringify({ compilerOptions: { noEmit: true } })); + fs.writeFileSync( + path.join(dir, 'tsconfig.json'), + [ + '{', + ' "extends": "./base.json",', + ' "compilerOptions": {', + ' // maps siblings to their packages/*/src trees', + ' "outDir": "dist"', + ' },', + ' "exclude": ["**/*.test.ts"]', + '}', + '', + ].join('\n'), + ); + + expect(parseTsconfig(path.join(dir, 'tsconfig.json')).compilerOptions?.outDir).toBe('dist'); + expect(effectiveNoEmit(path.join(dir, 'tsconfig.json'))).toBe(true); + }); + + it('still THROWS on a config nobody can read, which is what the catch is for', () => { + // `effectiveNoEmit` is written against this contract: a genuinely unreadable + // config widens the scan rather than dropping a package out of it. The fix + // narrows what counts as unreadable; it does not remove the margin. + const dir = tmpdir(); + fs.writeFileSync(path.join(dir, 'tsconfig.json'), '{ "compilerOptions": }\n'); + expect(() => readTsconfig(path.join(dir, 'tsconfig.json'))).toThrow(); + expect(effectiveNoEmit(path.join(dir, 'tsconfig.json'))).toBe(false); + }); +}); + +describe('the scope this gate ratchets, now that the emit question is answered', () => { + // Fixing the parse moves the specifier leg's membership exactly once, and the + // two packages below are both halves of that move. Pinned here so the change + // is a decision on the record rather than a side effect nobody measured — the + // failure objectui#5367 was filed to end. + + it('reads `@object-ui/fields` as NOT specifier-preserving, so it leaves the leg', () => { + // Its `tsc` step inherits the root's `noEmit: true` and only type-checks; + // `dist` is written by vite-plugin-dts, which resolves relative specifiers + // while bundling. Measured at the time this landed: 0 extensionless of 155 + // relative specifiers in its built `dist`, and 0 extensionless in its + // sources — so nothing live left the leg with it. The sources keep a + // STRICTER guard than this heuristic: that config pins `nodenext`, under + // which a missing relative extension is TS2835 in the package's own build. + const dir = path.join(REPO_ROOT, 'packages/fields'); + const build = JSON.parse(fs.readFileSync(path.join(dir, 'package.json'), 'utf8')).scripts.build; + + expect(effectiveNoEmit(path.join(dir, 'tsconfig.json'))).toBe(true); + expect(buildPreservesSpecifiers(build, dir)).toBe(false); + expect(parseTsconfig(path.join(dir, 'tsconfig.json')).compilerOptions?.noEmit).toBeUndefined(); + }); + + it('keeps `@object-ui/auth` in the leg on its DECLARED noEmit, not on a catch', () => { + // Its config throws under the old reader too, so it was in the leg by + // accident. It declares `"noEmit": false` outright, so the verdict is + // unchanged — what changes is that it is now a reading. + const dir = path.join(REPO_ROOT, 'packages/auth'); + const build = JSON.parse(fs.readFileSync(path.join(dir, 'package.json'), 'utf8')).scripts.build; + + expect(parseTsconfig(path.join(dir, 'tsconfig.json')).compilerOptions?.noEmit).toBe(false); + expect(effectiveNoEmit(path.join(dir, 'tsconfig.json'))).toBe(false); + expect(buildPreservesSpecifiers(build, dir)).toBe(true); + }); +}); + describe('the load leg evaluates rather than resolves', () => { it('fails a module that RESOLVES but throws on evaluation', () => { // The card's sharpest measured point: plugin-charts' entry resolved fine and diff --git a/scripts/check-node-esm-load.mjs b/scripts/check-node-esm-load.mjs index 960415a62..5f4b119e6 100644 --- a/scripts/check-node-esm-load.mjs +++ b/scripts/check-node-esm-load.mjs @@ -87,8 +87,10 @@ * specifiers seen by the mask, 2133 by the parser, the missing one a real * `import`. Leg 1 now reads the file as written and asks the shared * TypeScript scanner what the module edges are (objectui#5382); see - * `relativeSpecifiers`. `readTsconfig` below is the same class, still open - * as objectui#5367. + * `relativeSpecifiers`. `readTsconfig` below was the same class in the same + * file — three regexes over a tsconfig's raw text, blind to JSON strings, + * throwing on 61 of the repository's 91 configs — and took the same remedy + * under objectui#5367: TypeScript's own tsconfig reader. */ import { execFileSync, spawnSync } from 'node:child_process'; @@ -96,6 +98,8 @@ import { existsSync, readdirSync, readFileSync, statSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; + import { SKIP_DIRS, TOOLING_FILE, discoverPackages, moduleSpecifiers } from './check-phantom-dependencies.mjs'; const scriptDir = dirname(fileURLToPath(import.meta.url)); @@ -120,13 +124,52 @@ export const MIN_LOADED = 3; // ── which builds preserve specifiers ───────────────────────────────────────── -/** Parse a tsconfig, tolerating the comments this repository writes in them. */ +/** + * Parse a tsconfig, tolerating the comments this repository writes in them. + * + * ## Why this asks TypeScript rather than blanking comments (objectui#5367) + * + * It used to be three ordered `replace` calls over the raw text — blank block + * comments, blank whole-line comments, drop trailing commas — feeding + * `JSON.parse`. None of them knew what a JSON string was, so a slash-star + * sequence written INSIDE a string opened a "block comment" that ran to the + * next star-slash anywhere in the file and deleted every line between them. + * + * That is not exotic: `"@/*"` in a `paths` map and `"**\/*.test.ts"` in an + * `exclude` list are the two commonest lines in this repository's tsconfigs, + * and they are a matched opener and closer. Measured on `main` at ed35c23bb + * over the 91 tsconfigs tracked by git: **61 of them threw**, not the two the + * card was filed for. Every one of those throws was absorbed by + * `effectiveNoEmit`'s catch, which grades an unreadable config as emitting — so + * the gate's scope was being decided by a fallback rather than by a reading, + * with no signal that it had happened. + * + * This is the same class as the specifier mask objectui#5382 retired further + * down, and it takes the same remedy: ask the parser instead of pattern- + * matching around it. `ts.parseConfigFileTextToJson` is TypeScript's own + * tsconfig reader — comments, trailing commas and BOMs stop being questions + * this gate has an opinion about, and the answer is by construction the one + * `tsc` itself would compute. Re-measured with it: **0 of 91 throw**. + * + * No new dependency edge: `typescript` is a root devDependency and this module + * already loads it on every run, transitively through the shared scanner it + * imports from `check-phantom-dependencies.mjs`. + * + * Still THROWS on a genuinely unparseable config, deliberately — that is the + * contract `effectiveNoEmit`'s conservative catch is written against, and the + * contract `scripts/__tests__/check-node-esm-load.test.ts` asserts over every + * tsconfig in the repository so this class cannot come back silently. + * + * @param {string} path absolute path to a tsconfig + * @returns {Record} the parsed config (`{}` for an empty file, + * which is what TypeScript reads it as: inherit everything) + */ export function readTsconfig(path) { - const raw = readFileSync(path, 'utf8') - .replace(/\/\*[\s\S]*?\*\//g, '') - .replace(/^[ \t]*\/\/.*$/gm, '') - .replace(/,(\s*[}\]])/g, '$1'); - return JSON.parse(raw); + const parsed = ts.parseConfigFileTextToJson(path, readFileSync(path, 'utf8')); + if (parsed.error) { + throw new Error(`${path}: ${ts.flattenDiagnosticMessageText(parsed.error.messageText, ' ')}`); + } + return parsed.config ?? {}; } /** @@ -146,6 +189,13 @@ export function effectiveNoEmit(configPath, seen = new Set()) { } catch { // An unreadable config is graded as emitting, so a parse failure widens the // scan rather than silently dropping a package out of it. + // + // This is the fallback for a config nobody can read, and until objectui#5367 + // it was the NORMAL path: the regex strip `readTsconfig` used threw on 61 of + // this repository's 91 tsconfigs, so most of this gate's scope came from + // here instead of from a reading. With a real JSONC parse the branch is + // reached by nothing in the repository today, which is what it was always + // meant to be — a margin, not the mechanism. return false; } if (typeof config.compilerOptions?.noEmit === 'boolean') return config.compilerOptions.noEmit; @@ -176,10 +226,38 @@ export function effectiveNoEmit(configPath, seen = new Set()) { * the artifact comes from the two steps after it. Grading it on the command * alone reported 124 findings against a package that emits none of them. * - * A pipeline that runs an EMITTING `tsc` and a bundler (`@object-ui/fields`) - * counts as preserving: the `tsc` half still writes files, and judging it is the - * conservative direction — a false positive lands in the ledger with a reason, a - * false negative ships. + * A pipeline that runs an EMITTING `tsc` and a bundler counts as preserving: the + * `tsc` half still writes files, and judging it is the conservative direction — + * a false positive lands in the ledger with a reason, a false negative ships. + * No published package matches that shape today; the rule is kept because the + * shape is legal, not because something wears it. + * + * ## `@object-ui/fields` used to be this paragraph's example, and it was wrong + * + * It was named here as the emitting-tsc-plus-bundler case, and it is the + * opposite: `packages/fields/tsconfig.json` inherits the root's `noEmit: true` + * and its own comment says so in as many words — `tsc` CHECKS, `dist` is written + * by vite-plugin-dts. The example survived only because `readTsconfig` could not + * parse that file at all (objectui#5367): the throw hit `effectiveNoEmit`'s + * catch, the catch answered "emitting", and the wrong answer got written down as + * a fact about the package. + * + * Fixing the parse therefore MOVES this gate's scope, once, in one place: + * `@object-ui/fields` leaves the specifier leg, 13 published specifier- + * preserving ESM packages become 12. It is the only membership change in the + * repository — `@object-ui/auth`'s config throws today too, but it declares + * `"noEmit": false` outright, so its verdict is unchanged and merely stops being + * an accident. + * + * Letting it drop is deliberate, and it costs no coverage. Measured at ed35c23bb: + * `fields`' sources carry 0 extensionless relative specifiers, so no live finding + * leaves with it; its built `dist` carries 0 extensionless out of 155 relative + * specifiers, because rolldown resolves them while bundling, so the property this + * leg ratchets is not observable in what `fields` publishes. And the sources keep + * a STRICTER guard than this heuristic: that tsconfig pins `"moduleResolution": + * "nodenext"`, under which a missing relative extension is TS2835 and a bare + * directory import is TS2834 — the compiler rejects in `fields`' own build step + * what this leg would only have reported. * * @param {string} buildScript the package's `scripts.build`, or '' * @param {string} pkgDir absolute path to the package, for resolving `-p` @@ -400,9 +478,10 @@ export const ESM_MODULE_EDGE = new Set(['import', 'export', 'dynamic import()', * comment", and the two `replace` calls whose ORDER decided the answer were the * defect. Asking the parser removes the whole class at once: comments, strings, * template literals and regex literals stop being questions this gate has an - * opinion about. `readTsconfig()` above still strips comments with regexes and - * is a SEPARATE live instance of the same class — objectui#5367, deliberately - * not touched here. + * opinion about. `readTsconfig()` above was a SEPARATE live instance of the same + * class — regexes over a tsconfig's raw text, deliberately left for its own card + * — and it is closed the same way, by TypeScript's own tsconfig reader + * (objectui#5367). * * ## Why the sibling gate's scanner rather than a second parser *