Skip to content

Commit 422fe8c

Browse files
committed
test(exports): check every implementation key and cross-sibling order
Two coverage gaps in the exports guard. Both are latent — no workspace manifest violates either rule today (168 branches across 38 manifests, 0 new violations), which is exactly why they went unnoticed. Only the first implementation key per object was examined. `IMPLEMENTATION_KEYS.find(...)` stopped at `import`, and the recursion skipped string-valued implementation keys, so a flat dual-package object `{types, import: "./a.js", require: "./a.cjs"}` never had its `require` paired against anything — a `.cjs` declared by a `.d.ts` sailed through. The file's own `.cjs` fixtures use the nested `require: {types, default}` form, which is what hid it. Now every string-valued implementation key yields a branch. That makes one object produce several branches, which would collide in `label()` — the key for ALLOWED_MISMATCHES and the staleness check — so the implementation key joins the branch identity and the label reads `[condition > key]`. The shorthand form keeps its bare `[condition]`: its value IS the implementation, so there is no key to name. ALLOWED_MISMATCHES ships empty, so no allowlist migration is needed; only failure text changes. Condition ORDER across sibling keys was never checked. `typesBeforeImplementation` compares indices within one object, so a map whose branches are each internally well formed but ordered `{types, browser: {…}, import}` passes every existing check while TypeScript matches the outer `types` and never looks at `browser` — the browser-typed-as-node bug this file exists to prevent, expressed through ordering rather than through a wrong target. `{import, browser: {…}}` is the runtime equivalent. Adds `orderViolations`, covering object and string-shorthand condition keys alike, and asserts it over every workspace manifest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H797qbH356jjznKgUax63o
1 parent f8a9718 commit 422fe8c

1 file changed

Lines changed: 201 additions & 8 deletions

File tree

packages/test/src/test/util/ExportTypesPairing.test.ts

Lines changed: 201 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,17 @@ interface BranchLocation {
4747
readonly manifest: string;
4848
readonly subpath: string;
4949
readonly condition: string;
50+
/**
51+
* Which of {@link IMPLEMENTATION_KEYS} named this implementation, or
52+
* `undefined` for the string-shorthand form where the condition's own value
53+
* IS the implementation and there is no separate key.
54+
*
55+
* Part of the identity because one object can now yield several branches —
56+
* `{types, require, default}` produces one per string-valued implementation
57+
* key — and they would otherwise share a label, colliding as the key for
58+
* `ALLOWED_MISMATCHES` and the staleness check.
59+
*/
60+
readonly implementationKey: string | undefined;
5061
readonly implementation: string;
5162
readonly expectedTypes: string;
5263
}
@@ -113,6 +124,7 @@ function collectBranches(
113124
manifest,
114125
subpath,
115126
condition,
127+
implementationKey: undefined,
116128
types: undefined,
117129
reason: "missing",
118130
implementation: node,
@@ -123,18 +135,24 @@ function collectBranches(
123135
}
124136
if (typeof node !== "object" || node === null || Array.isArray(node)) return;
125137
const entry = node as Record<string, unknown>;
126-
const implementationKey = IMPLEMENTATION_KEYS.find((key) => typeof entry[key] === "string");
127-
if (implementationKey !== undefined) {
138+
// EVERY string-valued implementation key, not just the first. A dual-package
139+
// object writes them flat — `{types, import: "./a.mjs", require: "./a.cjs"}` —
140+
// and taking only `import` left `require` unchecked, so a `.cjs` paired with a
141+
// `.d.ts` sailed through. The file's own `.cjs` fixtures use the nested form,
142+
// which is why the flat shape was never exercised.
143+
const implementationKeys = IMPLEMENTATION_KEYS.filter((key) => typeof entry[key] === "string");
144+
const keys = Object.keys(entry);
145+
for (const implementationKey of implementationKeys) {
128146
const implementation = entry[implementationKey] as string;
129147
const location: BranchLocation = {
130148
manifest,
131149
subpath,
132150
condition,
151+
implementationKey,
133152
implementation,
134153
expectedTypes: declarationFor(implementation),
135154
};
136155
const types = entry.types;
137-
const keys = Object.keys(entry);
138156
out.push(
139157
typeof types === "string"
140158
? {
@@ -162,7 +180,8 @@ function collectBranches(
162180
}
163181

164182
function label(branch: Branch): string {
165-
return `${branch.manifest} exports["${branch.subpath}"] [${branch.condition}]`;
183+
const key = branch.implementationKey === undefined ? "" : ` > ${branch.implementationKey}`;
184+
return `${branch.manifest} exports["${branch.subpath}"] [${branch.condition}${key}]`;
166185
}
167186

168187
function isViolation(branch: Branch): boolean {
@@ -199,6 +218,70 @@ function findViolations(manifest: string, exportsMap: Record<string, unknown>):
199218
.map(violationMessage);
200219
}
201220

221+
function isImplementationString(entry: Record<string, unknown>, key: string): boolean {
222+
return (IMPLEMENTATION_KEYS as readonly string[]).includes(key) && typeof entry[key] === "string";
223+
}
224+
225+
/**
226+
* Condition keys made unreachable by a SIBLING declared before them.
227+
*
228+
* The pairing rule and its `typesBeforeImplementation` flag both look inside a
229+
* single object, so they see nothing wrong with a map whose branches are
230+
* individually well formed but ordered so that one can never be selected.
231+
* Resolution stops at the first key that matches, so:
232+
*
233+
* - `{types, browser: {…}, import}` — TypeScript matches the outer `types` and
234+
* never looks at `browser`, which is exactly the browser-typed-as-node bug
235+
* this file exists to prevent, expressed through ordering instead of through
236+
* a wrong target; and
237+
* - `{import, browser: {…}}` — a runtime honoring `browser` still matches
238+
* `import` first, so the browser build never loads.
239+
*
240+
* Both shapes yield `violations: []` and `late: []` from the existing checks.
241+
*
242+
* The string-shorthand form (`browser: "./dist/x.js"`) is included: it is just
243+
* as dead as the object form, and this repo demonstrably writes it.
244+
*/
245+
function orderViolations(manifest: string, exportsMap: Record<string, unknown>): string[] {
246+
const out: string[] = [];
247+
const walk = (subpath: string, conditionPath: readonly string[], node: unknown): void => {
248+
if (typeof node !== "object" || node === null || Array.isArray(node)) return;
249+
const entry = node as Record<string, unknown>;
250+
const keys = Object.keys(entry);
251+
const typesIndex = keys.indexOf("types");
252+
const implementationIndex = keys.findIndex((key) => isImplementationString(entry, key));
253+
const where = conditionPath.join(" > ") || "(default)";
254+
const at = `${manifest} exports["${subpath}"] [${where}]`;
255+
256+
keys.forEach((key, index) => {
257+
// Only condition keys can be shadowed. `types` and the implementation
258+
// strings are the things that DO the shadowing.
259+
if (key === "types" || isImplementationString(entry, key)) return;
260+
if (typesIndex >= 0 && typesIndex < index) {
261+
out.push(
262+
`${at}: condition "${key}" is declared after "types", so TypeScript resolves ` +
263+
`"types" first and "${key}" is never reached`
264+
);
265+
}
266+
if (implementationIndex >= 0 && implementationIndex < index) {
267+
out.push(
268+
`${at}: condition "${key}" is declared after "${keys[implementationIndex]}", so ` +
269+
`resolution stops there and "${key}" is never reached`
270+
);
271+
}
272+
});
273+
274+
for (const [key, value] of Object.entries(entry)) {
275+
if (key === "types" || typeof value === "string") continue;
276+
walk(subpath, [...conditionPath, key], value);
277+
}
278+
};
279+
for (const [subpath, value] of Object.entries(exportsMap)) {
280+
walk(subpath, [], value);
281+
}
282+
return out;
283+
}
284+
202285
interface WorkspaceRoot {
203286
readonly dir: string;
204287
/** Directories holding workspace packages, from the root `workspaces` globs. */
@@ -316,6 +399,13 @@ describe("workspace exports maps", () => {
316399
expect(late).toEqual([]);
317400
});
318401

402+
it("declares every condition before the siblings that would shadow it", () => {
403+
const shadowed = manifests.flatMap((manifest) =>
404+
orderViolations(manifest.relative, manifest.exports)
405+
);
406+
expect(shadowed).toEqual([]);
407+
});
408+
319409
it("keeps the allowlist free of entries that no longer mismatch", () => {
320410
const stale = [...ALLOWED_MISMATCHES].filter(
321411
(entry) => !branches.some((branch) => label(branch) === entry && isViolation(branch))
@@ -413,11 +503,44 @@ describe("exports map violation detection", () => {
413503
},
414504
})
415505
).toEqual([
416-
'fixture/package.json exports["."] [browser]: implementation "./dist/ai.browser.js" ' +
417-
'declares no types (expected types="./dist/ai.browser.d.ts")',
506+
'fixture/package.json exports["."] [browser > import]: implementation ' +
507+
'"./dist/ai.browser.js" declares no types (expected types="./dist/ai.browser.d.ts")',
418508
]);
419509
});
420510

511+
/**
512+
* A flat dual-package object. `IMPLEMENTATION_KEYS.find(...)` stopped at
513+
* `import`, so `require`'s `.cjs` was never paired against anything and its
514+
* `.d.ts` mismatch went unreported. The nested `require: {types, default}`
515+
* form the fixtures below use is what hid this.
516+
*/
517+
it("checks every implementation key in a flat dual-package branch", () => {
518+
expect(
519+
findViolations("fixture/package.json", {
520+
".": {
521+
types: "./dist/a.d.ts",
522+
import: "./dist/a.js",
523+
require: "./dist/a.cjs",
524+
},
525+
})
526+
).toEqual([
527+
'fixture/package.json exports["."] [(default) > require]: types="./dist/a.d.ts" but ' +
528+
'implementation is "./dist/a.cjs" (expected types="./dist/a.d.cts")',
529+
]);
530+
});
531+
532+
it("does not turn extra agreeing implementation keys into noise", () => {
533+
expect(
534+
findViolations("fixture/package.json", {
535+
".": {
536+
types: "./dist/a.d.ts",
537+
import: "./dist/a.js",
538+
default: "./dist/a.js",
539+
},
540+
})
541+
).toEqual([]);
542+
});
543+
421544
it("accepts a `.cjs`/`.mjs` implementation declared by its own extension", () => {
422545
expect(
423546
findViolations("fixture/package.json", {
@@ -440,8 +563,8 @@ describe("exports map violation detection", () => {
440563
},
441564
})
442565
).toEqual([
443-
'fixture/package.json exports["."] [require]: types="./dist/ai.d.ts" but implementation ' +
444-
'is "./dist/ai.cjs" (expected types="./dist/ai.d.cts")',
566+
'fixture/package.json exports["."] [require > default]: types="./dist/ai.d.ts" but ' +
567+
'implementation is "./dist/ai.cjs" (expected types="./dist/ai.d.cts")',
445568
]);
446569
});
447570

@@ -451,6 +574,76 @@ describe("exports map violation detection", () => {
451574
);
452575
});
453576

577+
/**
578+
* Ordering hazards. Each shape is internally well formed — every `types`
579+
* names the right target beside the right implementation — so the pairing
580+
* rule and the `typesBeforeImplementation` flag both pass them, which is the
581+
* whole reason the order check has to exist separately.
582+
*/
583+
describe("condition ordering", () => {
584+
const outerTypesFirst = {
585+
".": {
586+
types: "./dist/node.d.ts",
587+
browser: { types: "./dist/browser.d.ts", import: "./dist/browser.js" },
588+
import: "./dist/node.js",
589+
},
590+
};
591+
592+
it("reports a nested condition shadowed by an outer `types`", () => {
593+
// Documenting WHY a second check is needed: the pairing rule is silent
594+
// here, so without this the dead `browser` branch ships unnoticed.
595+
expect(findViolations("fixture/package.json", outerTypesFirst)).toEqual([]);
596+
597+
expect(orderViolations("fixture/package.json", outerTypesFirst)).toEqual([
598+
'fixture/package.json exports["."] [(default)]: condition "browser" is declared after ' +
599+
'"types", so TypeScript resolves "types" first and "browser" is never reached',
600+
]);
601+
});
602+
603+
it("reports a nested condition shadowed by an implementation key", () => {
604+
const implementationFirst = {
605+
".": {
606+
import: "./dist/node.js",
607+
browser: { types: "./dist/browser.d.ts", import: "./dist/browser.js" },
608+
},
609+
};
610+
expect(orderViolations("fixture/package.json", implementationFirst)).toEqual([
611+
'fixture/package.json exports["."] [(default)]: condition "browser" is declared after ' +
612+
'"import", so resolution stops there and "browser" is never reached',
613+
]);
614+
});
615+
616+
// Guards against an "objects only" implementation: a string shorthand is
617+
// just as dead, and this repo writes that form.
618+
it("reports a string-shorthand condition declared after `types`", () => {
619+
const shorthandLate = {
620+
".": {
621+
types: "./dist/node.d.ts",
622+
import: "./dist/node.js",
623+
browser: "./dist/browser.js",
624+
},
625+
};
626+
expect(orderViolations("fixture/package.json", shorthandLate)).toEqual([
627+
'fixture/package.json exports["."] [(default)]: condition "browser" is declared after ' +
628+
'"types", so TypeScript resolves "types" first and "browser" is never reached',
629+
'fixture/package.json exports["."] [(default)]: condition "browser" is declared after ' +
630+
'"import", so resolution stops there and "browser" is never reached',
631+
]);
632+
});
633+
634+
it("accepts a condition declared before both", () => {
635+
expect(
636+
orderViolations("fixture/package.json", {
637+
".": {
638+
browser: { types: "./dist/browser.d.ts", import: "./dist/browser.js" },
639+
types: "./dist/node.d.ts",
640+
import: "./dist/node.js",
641+
},
642+
})
643+
).toEqual([]);
644+
});
645+
});
646+
454647
it("derives a source entry from a dist target", () => {
455648
expect(sourceCandidates("providers/openai/package.json", "./dist/ai.browser.d.ts")).toEqual([
456649
"providers/openai/src/ai.browser.ts",

0 commit comments

Comments
 (0)