diff --git a/.changeset/alias-closure-relative-miss-5386.md b/.changeset/alias-closure-relative-miss-5386.md new file mode 100644 index 000000000..f67ab4d32 --- /dev/null +++ b/.changeset/alias-closure-relative-miss-5386.md @@ -0,0 +1,39 @@ +--- +--- + +Tests only — this publishes nothing, declared explicitly with an empty frontmatter +rather than left undeclared. No package `src/` is touched; the only file changed is +`examples/console-starter/test/vite-alias-closure.test.ts`, and that example is private. + +Makes the alias-closure walker record an unresolvable **relative** import instead of +dropping it. + +`computeClosure()` walks two kinds of specifier. The bare-specifier branch pushed a miss +onto `unresolvable`; the relative branch dropped one with no record at all. So +`expect(closure.unresolvable).toEqual([])` was not a weak assertion for relative imports +— it was a structurally empty one. It could not fail no matter how many relative +specifiers the walk failed to follow, and the `filesWalked` floor was the only signal +that anything had gone wrong. + +That is how two earlier conversions to explicit extensions (objectui#4538, objectui#5214) +each truncated this walk while landing green: the floor had enough slack to absorb both, +and only went red once app-shell — the largest package — converted and took the count +under 500. The direct symptom was being swallowed one branch away the whole time. + +Measured on `main` at the time of this change, with the resolver ablated to its +pre-objectui#5357 behaviour to reproduce that regression class: 275 relative specifiers +dropped, `filesWalked` 1245 to 402, packages reached 29 to 22 — and `unresolvable` still +reporting `[]`. With this change the same ablation fails the suite with all 275 named, +each alongside the file that imports it. + +The miss is recorded only for specifiers that are *meant* to be modules — no extension, +or one of the JS/TS emitted extensions. `ts.preProcessFile` also reports `./styles.css`, +`./data.json` and `./logo.svg`, which `resolveModule` cannot resolve by design, so +recording those identically would fail the suite for a reason that is not a defect. +Assets and Vite resource specifiers (`?raw`, `?url`, `?inline`) are skipped into a +separate `nonModuleSkipped` list — explicitly, and observably, rather than by accident. + +Four fixtures pin the class against the real walker so a future edit cannot silently +re-blind the branch: `./Foo.js` resolving through to the `Foo.tsx` on disk, planted +unresolvable modules of both spellings being named, assets staying out of `unresolvable` +while still being accounted for, and the classifier's boundary cases. diff --git a/examples/console-starter/test/vite-alias-closure.test.ts b/examples/console-starter/test/vite-alias-closure.test.ts index 13f0438c8..0fc8387f6 100644 --- a/examples/console-starter/test/vite-alias-closure.test.ts +++ b/examples/console-starter/test/vite-alias-closure.test.ts @@ -32,6 +32,7 @@ import { describe, it, expect } from 'vitest'; import fs from 'node:fs'; +import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; import ts from 'typescript'; @@ -81,6 +82,45 @@ function resolveModule(target: string): string | null { return null; } +/** + * The spellings a specifier that is *meant to be a module* can end with. Derived + * from `RESOLVABLE` so the two cannot drift, plus the two TypeScript module + * extensions that are legal to write but that `resolveModule` never has to + * probe for (a `.mts` source is imported as `.mjs`). + */ +const MODULE_EXTENSIONS = [...RESOLVABLE, '.mts', '.cts']; + +/** + * Does this relative specifier denote a module the walk is supposed to follow? + * + * `ts.preProcessFile` reports EVERY import specifier, including `./styles.css`, + * `./data.json` and `./logo.svg` — which `resolveModule` deliberately cannot + * resolve, because its candidate list is JS and TS only. Those are not walk + * failures, and recording them as such would fail this test for a reason that + * is not a defect. The closure contains two `.css` imports today, both of which + * happen to resolve only because `resolveModule` returns any path that exists + * on disk verbatim; a stylesheet that is virtual, generated, or shipped only in + * `dist` would land in the miss branch the moment it appeared. + * + * So the boundary is drawn on *intent*, not on whether resolution happened to + * succeed: no extension, or one of the JS/TS emitted extensions, is a module. + * Anything else is an asset and is skipped EXPLICITLY — recorded in + * `nonModuleSkipped` rather than dropped — so that the skip stays auditable and + * this branch can never again go quiet by accident. + * + * A specifier carrying a `?query` or `#hash` is a Vite resource specifier + * (`?raw`, `?url`, `?inline`, `?worker`) rather than a plain module path, and + * the walk cannot follow it meaningfully — it counts as a non-module skip. The + * closure contains none today. + */ +function isModuleSpecifier(specifier: string): boolean { + if (specifier.includes('?') || specifier.includes('#')) return false; + // `.` and `..` are extensionless directory imports, not an `.`-extension. + const ext = /\.[a-zA-Z0-9]+$/.exec(specifier)?.[0]; + if (!ext) return true; + return MODULE_EXTENSIONS.includes(ext.toLowerCase()); +} + const packageSrc = (pkg: string) => path.join(repoRoot, 'packages', pkg.slice('@object-ui/'.length), 'src'); @@ -99,15 +139,23 @@ interface Closure { direct: Set; filesWalked: number; unresolvable: string[]; + /** + * Relative specifiers the walk skipped on purpose because they are assets, + * not modules. Never asserted empty — this exists so the skip is observable + * instead of implicit, and so a fixture can prove the boundary is drawn where + * it is documented to be. + */ + nonModuleSkipped: string[]; } -function computeClosure(): Closure { +function computeClosure(entryDir: string = path.join(exampleDir, 'src')): Closure { const packages = new Map>(); const direct = new Set(); const unresolvable: string[] = []; + const nonModuleSkipped: string[] = []; const seen = new Set(); - const exampleSrc = path.join(exampleDir, 'src'); + const exampleSrc = entryDir; function walk(file: string): void { if (seen.has(file)) return; @@ -122,7 +170,24 @@ function computeClosure(): Closure { for (const specifier of specifiers) { if (specifier.startsWith('.')) { const resolved = resolveModule(path.resolve(path.dirname(file), specifier)); - if (resolved) walk(resolved); + if (resolved) { + walk(resolved); + continue; + } + // A miss used to vanish here with no record, which made + // `unresolvable` structurally empty for this entire specifier class: + // `expect(closure.unresolvable).toEqual([])` could not fail for a + // relative import no matter how many the walk failed to follow. That is + // how objectui#4538's and objectui#5214's conversions each truncated + // this walk while landing green — the `filesWalked` floor was the only + // signal there was, and it had enough slack to hide two pull requests. + if (isModuleSpecifier(specifier)) { + unresolvable.push( + `${specifier} (unresolvable relative import, imported by ${path.relative(repoRoot, file)})`, + ); + } else { + nonModuleSkipped.push(`${specifier} (imported by ${path.relative(repoRoot, file)})`); + } continue; } // Third-party and @objectstack/* are real registry dependencies; only @@ -149,7 +214,7 @@ function computeClosure(): Closure { if (/\.(ts|tsx|js|jsx)$/.test(entry)) walk(path.join(exampleSrc, entry)); } - return { packages, direct, filesWalked: seen.size, unresolvable }; + return { packages, direct, filesWalked: seen.size, unresolvable, nonModuleSkipped }; } describe('console-starter vite alias table', () => { @@ -168,6 +233,10 @@ describe('console-starter vite alias table', () => { it('reaches the workspace graph it is meant to cover', () => { // Same guard for the walker: a resolution regression that walked nothing // would make the closure assertion trivially true. + // + // `unresolvable` now covers BOTH walk branches — bare `@object-ui/*` and + // relative — so a resolution regression is reported as itself, by name and + // importer, instead of only as a file count that drifted toward a floor. expect(closure.filesWalked).toBeGreaterThan(500); expect(closure.unresolvable).toEqual([]); }); @@ -197,3 +266,106 @@ describe('console-starter vite alias table', () => { expect(undeclared).toEqual([]); }); }); + +/** + * `closure.unresolvable` above is asserted empty, and on a healthy tree it IS + * empty — which is exactly the reading that cannot be trusted on its own. An + * assertion that receives nothing looks identical to one that cannot receive + * anything, and for the relative branch it *was* the second of those for three + * pull requests. + * + * So the empty reading is only worth what these fixtures are worth: the same + * walker, over a tree built on purpose, must report the misses that genuinely + * are misses and stay silent about the assets that are not. Pinning the class + * here means a future edit cannot re-blind the branch without turning this red. + */ +describe('the closure walker records relative misses instead of dropping them', () => { + /** Builds a throwaway source tree and walks it with the real `computeClosure`. */ + function withFixture(files: Record, run: (dir: string) => T): T { + const dir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'alias-closure-'))); + try { + for (const [name, content] of Object.entries(files)) { + const target = path.join(dir, name); + fs.mkdirSync(path.dirname(target), { recursive: true }); + fs.writeFileSync(target, content); + } + return run(dir); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + } + + it('walks `./Foo.js` through to the `Foo.tsx` on disk', () => { + // The case that produced every row of objectui#5386's table: a package whose + // build preserves specifiers must spell `Foo.tsx` as `./Foo.js`, and the + // walk has to follow it rather than lose the subtree behind it. + const closure = withFixture( + { + 'entry.ts': `import './Foo.js';\n`, + 'Foo.tsx': `import './Bar';\nexport const Foo = 1;\n`, + 'Bar.ts': `export const Bar = 2;\n`, + }, + (dir) => computeClosure(dir), + ); + + // entry.ts + Foo.tsx + Bar.ts — the subtree behind the `.js` spelling is + // reached, not dropped at the first re-export. + expect(closure.filesWalked).toBe(3); + expect(closure.unresolvable).toEqual([]); + }); + + it('reports a relative module specifier that does not resolve', () => { + // The counter-probe for the empty reading above: plant misses of both + // module spellings and require this walker to name them. + const closure = withFixture( + { + 'entry.ts': `import './missing-module.js';\nimport './gone';\n`, + }, + (dir) => computeClosure(dir), + ); + + expect(closure.unresolvable).toHaveLength(2); + expect(closure.unresolvable.join('\n')).toContain('./missing-module.js'); + expect(closure.unresolvable.join('\n')).toContain('./gone'); + }); + + it('skips an unresolvable asset specifier explicitly rather than reporting it', () => { + // The boundary condition: a stylesheet, image or font import is not a + // missing module. `ts.preProcessFile` reports them all, and `resolveModule` + // cannot resolve any of them, so recording them alongside real misses would + // manufacture failures that are not defects. + const closure = withFixture( + { + 'entry.ts': + `import './theme.css';\n` + + `import './logo.svg';\n` + + `import './font.woff2';\n` + + `import './data.json';\n` + + `import './raw.css?inline';\n` + + `import './real.js';\n`, + 'real.ts': `export const real = 1;\n`, + }, + (dir) => computeClosure(dir), + ); + + // None of the five assets is a defect... + expect(closure.unresolvable).toEqual([]); + // ...but each is accounted for, so the skip is a decision and not a drop. + expect(closure.nonModuleSkipped).toHaveLength(5); + expect(closure.nonModuleSkipped.join('\n')).toContain('./theme.css'); + expect(closure.nonModuleSkipped.join('\n')).toContain('./raw.css?inline'); + // The module alongside them still resolves and is walked. + expect(closure.filesWalked).toBe(2); + }); + + it('classifies the specifier spellings that decide the boundary', () => { + // Directly pins `isModuleSpecifier`, so the line stays where it is + // documented even if the walk around it is rewritten. + for (const module of ['./Foo', '.', '..', '../pkg/src', './Foo.js', './Foo.ts', './Foo.mts']) { + expect(isModuleSpecifier(module)).toBe(true); + } + for (const asset of ['./a.css', './a.svg', './a.json', './a.woff2', './a.js?worker']) { + expect(isModuleSpecifier(asset)).toBe(false); + } + }); +});