From 745ee1f8ab8f858dcf39eb0d639e5c7bee426b4b Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 27 Aug 2026 15:59:31 +0200 Subject: [PATCH 1/6] Test the published package, and fix two defects it found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everything in the suite tested the source, or `dist/` from inside the repository. Neither can see the questions a consumer actually hits, because the tarball's METADATA decides them: which file an `exports` condition resolves to, which declarations TypeScript reads beside it, whether `files` left something out, whether the bin runs when npm installs it as a shim. `scripts/compat-matrix.mjs` packs a real tarball and installs it into isolated consumer projects — ESM JavaScript, CommonJS JavaScript, ESM TypeScript, CommonJS TypeScript, CommonJS TypeScript without `@types/node`, the installed bin, and export encapsulation. It runs identically locally (`npm run test:consumers`) and in CI across npm, pnpm, Yarn and Bun, plus a Windows smoke test for the generated `.cmd` launcher. The existing Node 18/20/22/24 matrix is untouched. It found two shipped defects immediately. ## CommonJS TypeScript could not use this package at all `exports` carried a single top-level `types` pointing at the ESM declarations. A CommonJS TypeScript consumer therefore resolved `require` for the runtime and ESM types for the shape, so TypeScript concluded the target was an ES module and refused the `require` outright — TS1479, at the import line, with no way around it. CommonJS JavaScript worked perfectly, which is why nothing noticed. Types are per-condition now, `import` to `.d.ts` and `require` to `.d.cts`, with `default` last. The root's `.d.cts` was already being built and simply never referenced; `./protect` had no `.d.cts` at all despite advertising a `require` condition, so the build now emits one — a copy, because tsup emits the root's two declaration files byte-identical and there is nothing per-format to express. Edge conditions stay ahead of `import`, so a Worker still gets the edge bundle. TypeScript does not apply them and falls through to `import`/`require`, where it now finds the right declarations. ## The published types required a dependency the package does not declare Two exported functions took `NodeJS.ProcessEnv`, which comes from `@types/node`. A consumer without it could not compile against the public surface at all — the declarations referenced a namespace their project had never heard of. An `EnvLike` alias replaces it: `process.env` satisfies it, and so does a plain object. `cjs-ts-no-node-types` exists for exactly this and is not redundant with `cjs-ts`, since every other fixture installs `@types/node` and would keep passing. Both regressions confirmed to fail the matrix: reverting `exports` fails 2 of 7 shapes, restoring `NodeJS.ProcessEnv` fails 1. Dropping the `protect.d.cts` copy fails the packaging test, which also now checks that every file the export map names is inside `files` AND actually built — a map can name a path the build never produces, and that surfaces three layers away as "cannot find module" in someone else's project. `module` is kept. Removing it needs evidence no bundler still reads it, and the bundled-consumer test that would provide that is not written yet; keeping a redundant field costs nothing, guessing costs a consumer. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 60 +++++++++++ CONTRIBUTING.md | 6 ++ package.json | 25 +++-- scripts/compat-matrix.mjs | 165 +++++++++++++++++++++++++++++ scripts/copy-protect-templates.mjs | 10 +- src/stack.ts | 14 ++- tests/protect/edge-bundle.test.ts | 28 ++++- 7 files changed, 298 insertions(+), 10 deletions(-) create mode 100644 scripts/compat-matrix.mjs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f9e70d70..62d4d0f4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,6 +42,66 @@ jobs: # unable to tell it is behind — the same silent drift the manifest exists to remove. run: node scripts/check-capability-version.mjs --base "origin/${{ github.base_ref || github.event.repository.default_branch }}" + # Does the PUBLISHED package work when a real consumer installs it? + # + # Everything in `validate` tests the source, or `dist/` from inside the repository. Neither can see the + # questions a consumer actually hits, because the tarball's METADATA decides them: which file an `exports` + # condition resolves to, which declarations TypeScript reads beside it, whether `files` left something + # out. Two such defects were already shipped and were found by writing this check. + # + # Four managers because their resolution genuinely differs, and npm's answer does not generalise — that + # is the whole reason to run more than one. + consumers: + name: 📦 Consumers on ${{ matrix.manager }} + runs-on: ubuntu-latest + timeout-minutes: 15 + strategy: + fail-fast: false + matrix: + manager: [npm, pnpm, yarn, bun] + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: 22.x + cache: npm + + - if: matrix.manager == 'pnpm' + uses: pnpm/action-setup@v4 + with: + version: 9 + + - if: matrix.manager == 'bun' + uses: oven-sh/setup-bun@v2 + + - name: Install dependencies + run: npm ci + + - name: Consumer shapes against a packed tarball + run: node scripts/compat-matrix.mjs --manager ${{ matrix.manager }} + + # The generated `.cmd` launcher and path handling are Windows-only code paths in npm's shim, not ours, + # and they are exactly what breaks a bin that works everywhere else. One smoke test rather than the whole + # matrix: the question is whether the launcher runs and resolves, not whether four managers agree. + windows-smoke: + name: 🪟 Windows bin and consumer smoke + runs-on: windows-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: 22.x + cache: npm + + - name: Install dependencies + run: npm ci + + - name: Consumer shapes against a packed tarball + run: node scripts/compat-matrix.mjs --manager npm + validate: name: Validate on Node ${{ matrix.node-version }} runs-on: ubuntu-latest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3d3bedd9..667207f0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -49,6 +49,12 @@ reads exactly like passing. - `npm run typecheck`, `npm test`, `npm run build` all clean. - `npm pack --dry-run` if you touched anything about packaging. `files` in `package.json` is the authoritative allowlist of what ships. +- `npm run test:consumers` if you touched `exports`, `files`, the build config or anything about types. It + packs a real tarball, installs it into isolated consumer projects and checks every shape a consumer can + use — ESM and CommonJS, JavaScript and TypeScript, the installed bin, and that nothing outside `exports` + is reachable. Nothing else can see those: they are decided by the tarball's metadata rather than by the + code, so the source suite passes while a consumer cannot import the package at all. CI runs it across + npm, pnpm, Yarn and Bun, plus a Windows smoke test. - Read your own diff, commit messages and pull-request description against the boundary above. The pull-request template asks you to confirm this; it is the only control that covers prose, because secret scanning finds credentials and nothing finds a paragraph of architecture. diff --git a/package.json b/package.json index c9cb4c7d..6686cfb0 100644 --- a/package.json +++ b/package.json @@ -27,19 +27,31 @@ "types": "./dist/index.d.ts", "exports": { ".": { - "types": "./dist/index.d.ts", - "import": "./dist/index.js", - "require": "./dist/index.cjs" + "import": { + "types": "./dist/index.d.ts", + "default": "./dist/index.js" + }, + "require": { + "types": "./dist/index.d.cts", + "default": "./dist/index.cjs" + }, + "default": "./dist/index.js" }, "./protect": { - "types": "./dist/protect.d.ts", "workerd": "./dist/protect.edge.js", "worker": "./dist/protect.edge.js", "edge-light": "./dist/protect.edge.js", "deno": "./dist/protect.edge.js", "browser": "./dist/protect.edge.js", - "import": "./dist/protect.js", - "require": "./dist/protect.cjs" + "import": { + "types": "./dist/protect.d.ts", + "default": "./dist/protect.js" + }, + "require": { + "types": "./dist/protect.d.cts", + "default": "./dist/protect.cjs" + }, + "default": "./dist/protect.js" } }, "bin": { @@ -56,6 +68,7 @@ "verify:edge": "node scripts/verify-edge-platform.mjs", "dev": "tsup --watch", "test": "vitest run", + "test:consumers": "node scripts/compat-matrix.mjs", "test:manifest": "bun scripts/test-manifest.ts", "test:watch": "vitest", "typecheck": "tsc --noEmit && node scripts/typecheck-templates.mjs", diff --git a/scripts/compat-matrix.mjs b/scripts/compat-matrix.mjs new file mode 100644 index 00000000..9e8647ad --- /dev/null +++ b/scripts/compat-matrix.mjs @@ -0,0 +1,165 @@ +#!/usr/bin/env node +// Does the PUBLISHED package work when a real consumer installs it? +// +// Everything else in this repository tests the source, or at best `dist/` from inside the repository. That +// cannot see the questions a consumer actually hits, because they are all decided by the tarball's +// metadata rather than by the code: which file an `exports` condition resolves to, which declarations +// TypeScript reads beside it, whether `files` left something out, whether the bin is executable. +// +// Those failures are invisible from here and total for the consumer. Two were shipped and found by writing +// this: +// +// - `exports` had a single top-level `types` pointing at the ESM declarations, so a CommonJS TypeScript +// consumer resolved `require` for the runtime and ESM types for the shape. TypeScript concluded the +// target was an ES module and refused the `require` (TS1479) — while CommonJS JavaScript worked. The +// package was unusable from CommonJS TypeScript. +// - the published declarations referenced `NodeJS.ProcessEnv`, from `@types/node`, which this package +// does not depend on. A consumer without it could not compile against the types at all. +// +// Run: node scripts/compat-matrix.mjs [--manager npm|pnpm|yarn|bun] +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync, existsSync, readdirSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +const manager = (() => { + const i = process.argv.indexOf('--manager'); + return i === -1 ? 'npm' : process.argv[i + 1]; +})(); + +const ROOT = process.cwd(); +const run = (cmd, args, cwd, extraEnv = {}) => + execFileSync(cmd, args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env, ...extraEnv } }); + +/** Install a local tarball. Each manager spells it differently, and their resolution differs — which is + * the point of running all four rather than assuming npm's answer generalises. */ +const INSTALL = { + npm: (tgz, deps) => ['npm', ['install', '--no-audit', '--no-fund', '--silent', tgz, ...deps]], + pnpm: (tgz, deps) => ['pnpm', ['add', '--silent', tgz, ...deps]], + yarn: (tgz, deps) => ['yarn', ['add', tgz, ...deps]], + bun: (tgz, deps) => ['bun', ['add', tgz, ...deps]], +}; + +if (!INSTALL[manager]) { + console.error(`Unknown manager: ${manager}. Known: ${Object.keys(INSTALL).join(', ')}`); + process.exit(2); +} + +const PROBE_TS = ` +import { buildWirePayload, collectHostingEnvKeys } from '@patchstack/connect'; +import { createProtection } from '@patchstack/connect/protect'; +void buildWirePayload({ ecosystem: 'npm', packages: [] }); +void collectHostingEnvKeys({ VERCEL: '1' }); +void createProtection; +`; + +const TSCONFIG = (module_, resolution, types) => JSON.stringify({ + compilerOptions: { + module: module_, moduleResolution: resolution, target: 'es2022', + strict: true, noEmit: true, ...(types ? { types } : {}), + }, +}, null, 2); + +/** + * Each consumer shape, and what it is here to prove. + * + * `cjs-ts-no-node-types` is not redundant with `cjs-ts`: it is the only one that catches published + * declarations depending on `@types/node` without declaring it, because every other fixture installs it. + */ +const SHAPES = [ + { + name: 'esm-js', why: 'ESM JavaScript import', + pkg: { type: 'module' }, deps: [], + files: { 'probe.mjs': "import * as r from '@patchstack/connect';\nimport * as p from '@patchstack/connect/protect';\nif (typeof r.buildWirePayload !== 'function') throw new Error('root export missing');\nif (typeof p.createProtection !== 'function') throw new Error('protect export missing');\n" }, + check: (dir) => run('node', ['probe.mjs'], dir), + }, + { + name: 'cjs-js', why: 'CommonJS require', + pkg: {}, deps: [], + files: { 'probe.cjs': "const r = require('@patchstack/connect');\nconst p = require('@patchstack/connect/protect');\nif (typeof r.buildWirePayload !== 'function') throw new Error('root export missing');\nif (typeof p.createProtection !== 'function') throw new Error('protect export missing');\n" }, + check: (dir) => run('node', ['probe.cjs'], dir), + }, + { + name: 'esm-ts', why: 'ESM TypeScript compilation', + pkg: { type: 'module' }, deps: ['typescript@5', '@types/node'], + files: { 'probe.ts': PROBE_TS, 'tsconfig.json': TSCONFIG('nodenext', 'nodenext') }, + check: (dir) => run(tsc(dir), ['-p', 'tsconfig.json'], dir), + }, + { + name: 'cjs-ts', why: 'CommonJS TypeScript compilation', + pkg: {}, deps: ['typescript@5', '@types/node'], + files: { 'probe.ts': PROBE_TS, 'tsconfig.json': TSCONFIG('node16', 'node16') }, + check: (dir) => run(tsc(dir), ['-p', 'tsconfig.json'], dir), + }, + { + name: 'cjs-ts-no-node-types', why: 'published types must not require an undeclared @types/node', + pkg: {}, deps: ['typescript@5'], + files: { 'probe.ts': PROBE_TS, 'tsconfig.json': TSCONFIG('node16', 'node16', []) }, + check: (dir) => run(tsc(dir), ['-p', 'tsconfig.json'], dir), + }, + { + name: 'cli', why: 'the installed bin runs', + pkg: { type: 'module' }, deps: [], + check: (dir) => { + const out = run(bin(dir), ['--help'], dir); + if (!out.includes('patchstack-connect')) throw new Error(`bin produced no recognisable help:\n${out}`); + + return out; + }, + }, + { + name: 'encapsulation', why: 'nothing outside exports is reachable', + pkg: { type: 'module' }, deps: [], + files: { + 'probe.mjs': "let leaked = [];\nfor (const p of ['@patchstack/connect/dist/cli.js', '@patchstack/connect/package.json', '@patchstack/connect/src/index.ts']) {\n try { await import(p); leaked.push(p); } catch {}\n}\nif (leaked.length) throw new Error('reachable outside exports: ' + leaked.join(', '));\n", + }, + check: (dir) => run('node', ['probe.mjs'], dir), + }, +]; + +const tsc = (dir) => path.join(dir, 'node_modules', '.bin', process.platform === 'win32' ? 'tsc.cmd' : 'tsc'); +const bin = (dir) => path.join(dir, 'node_modules', '.bin', process.platform === 'win32' ? 'patchstack-connect.cmd' : 'patchstack-connect'); + +/** Pack the real artifact. `--ignore-scripts` is deliberately NOT passed: `prepare` builds `dist/`. */ +function packTarball(into) { + const out = run('npm', ['pack', '--pack-destination', into, '--silent'], ROOT).trim().split('\n').pop().trim(); + // npm's stdout has carried extra lines across versions, so trust the directory rather than the parse. + const found = readdirSync(into).filter((f) => f.endsWith('.tgz')); + if (found.length !== 1) throw new Error(`expected exactly one tarball in ${into}, found: ${found.join(', ') || 'none'} (npm said "${out}")`); + + return path.join(into, found[0]); +} + +const work = mkdtempSync(path.join(tmpdir(), 'ps-compat-')); +let failures = 0; + +try { + const tarball = packTarball(work); + console.log(`packed ${path.basename(tarball)}; installing with ${manager}\n`); + + for (const shape of SHAPES) { + const dir = path.join(work, shape.name); + mkdirSync(dir, { recursive: true }); + writeFileSync(path.join(dir, 'package.json'), JSON.stringify({ name: `consumer-${shape.name}`, private: true, ...shape.pkg }, null, 2)); + for (const [file, body] of Object.entries(shape.files ?? {})) writeFileSync(path.join(dir, file), body); + + try { + const [cmd, args] = INSTALL[manager](tarball, shape.deps); + run(cmd, args, dir); + shape.check(dir); + console.log(` ok ${shape.name.padEnd(22)} ${shape.why}`); + } catch (error) { + failures++; + const detail = `${error.stdout ?? ''}${error.stderr ?? ''}`.trim() || error.message; + console.log(` FAIL ${shape.name.padEnd(22)} ${shape.why}`); + console.log(detail.split('\n').slice(0, 8).map((l) => ` ${l}`).join('\n')); + } + } +} finally { + rmSync(work, { recursive: true, force: true }); +} + +console.log(failures === 0 + ? `\nall ${SHAPES.length} consumer shapes work with ${manager}` + : `\n${failures} of ${SHAPES.length} consumer shapes FAILED with ${manager}`); +process.exit(failures === 0 ? 0 : 1); diff --git a/scripts/copy-protect-templates.mjs b/scripts/copy-protect-templates.mjs index ca1ea7c7..b1ca860f 100644 --- a/scripts/copy-protect-templates.mjs +++ b/scripts/copy-protect-templates.mjs @@ -10,4 +10,12 @@ console.log('copied protect templates -> dist/protect/templates'); // Ship the hand-authored declarations for the `@patchstack/connect/protect` subpath (the runtime // is plain JS, so tsup doesn't emit these). Referenced by the "./protect" export's `types`. copyFileSync('src/protect/protect.d.ts', 'dist/protect.d.ts'); -console.log('copied protect types -> dist/protect.d.ts'); +// The same declarations under the CommonJS extension. A CJS TypeScript consumer resolves the `require` +// condition, and TypeScript then reads the types beside it: given an ESM `.d.ts` it concludes the target is +// an ES module and refuses the `require` outright (TS1479) — while the CJS runtime works perfectly. So the +// package was unusable from CommonJS TypeScript and fine from CommonJS JavaScript. +// +// A copy rather than a second generated file, because tsup emits `index.d.ts` and `index.d.cts` +// byte-identical for the root entry; there is nothing per-format to express. +copyFileSync('src/protect/protect.d.ts', 'dist/protect.d.cts'); +console.log('copied protect types -> dist/protect.d.ts, dist/protect.d.cts'); diff --git a/src/stack.ts b/src/stack.ts index 4966b216..04e90381 100644 --- a/src/stack.ts +++ b/src/stack.ts @@ -112,7 +112,17 @@ const HOSTING_ENV_PATTERNS: readonly RegExp[] = [ * distinguish "deployed on Cloudflare" from "deployed on Vercel" without ever * disclosing a secret. */ -export function collectHostingEnvKeys(env: NodeJS.ProcessEnv = process.env): string[] { +/** + * The shape of an environment, structurally. + * + * Not `NodeJS.ProcessEnv`: that type comes from `@types/node`, which this package does not declare as a + * dependency, so a consumer without it could not compile against these signatures at all — the published + * declarations referenced a namespace their project had never heard of. `process.env` satisfies this, and + * so does a plain object, which is what the tests pass. + */ +export type EnvLike = Record; + +export function collectHostingEnvKeys(env: EnvLike = process.env): string[] { return Object.keys(env) .filter((key) => HOSTING_ENV_PATTERNS.some((pattern) => pattern.test(key))) .sort(); @@ -124,7 +134,7 @@ export function collectHostingEnvKeys(env: NodeJS.ProcessEnv = process.env): str */ export function detectStack( packages: readonly WirePackage[], - env: NodeJS.ProcessEnv = process.env, + env: EnvLike = process.env, ): StackDescriptor { const present = new Set(packages.map((pkg) => pkg.name)); diff --git a/tests/protect/edge-bundle.test.ts b/tests/protect/edge-bundle.test.ts index 5b0018a6..065ea75e 100644 --- a/tests/protect/edge-bundle.test.ts +++ b/tests/protect/edge-bundle.test.ts @@ -63,9 +63,35 @@ describe('edge bundle', () => { for (const cond of ['workerd', 'worker', 'edge-light', 'deno', 'browser']) { expect(protect[cond]).toBe('./dist/protect.edge.js'); } - expect(protect.import).toBe('./dist/protect.js'); // Node still gets the full build + // Node still gets the full build. Nested now, because the types differ per format: a CommonJS + // consumer resolving `require` must be handed the `.d.cts` declarations, or TypeScript reads the ESM + // ones, concludes the target is an ES module and refuses the `require` outright. + expect(protect.import).toEqual({ types: './dist/protect.d.ts', default: './dist/protect.js' }); + expect(protect.require).toEqual({ types: './dist/protect.d.cts', default: './dist/protect.cjs' }); + // Condition order matters: an edge condition must be matched before the generic `import`. const keys = Object.keys(protect); expect(keys.indexOf('workerd')).toBeLessThan(keys.indexOf('import')); + // And `default` must be last, or it shadows everything after it. + expect(keys.indexOf('default')).toBe(keys.length - 1); + }); + + it('ships the declarations both format conditions point at', () => { + // The map can name a file that the build never produces, and the failure surfaces three layers away as + // "cannot find module" in a consumer's project. `protect.d.cts` did not exist at all while `require` + // was already being advertised. + const pkg = JSON.parse(readFileSync(root + 'package.json', 'utf8')); + const shipped = pkg.files as string[]; + + for (const entry of [pkg.exports['.'], pkg.exports['./protect']]) { + for (const condition of ['import', 'require'] as const) { + const target = entry[condition] as { types: string; default: string }; + for (const file of [target.types, target.default]) { + // `files` is the allowlist: a path outside it resolves locally and is absent from the tarball. + expect(shipped.some((allowed) => file.startsWith(`./${allowed}`))).toBe(true); + expect(existsSync(root + file.replace(/^\.\//, '')), `${file} is exported but not built`).toBe(true); + } + } + } }); }); From 96d6afa324e294a5ff3fd15eb65fa6b897cbdff7 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 27 Aug 2026 16:19:44 +0200 Subject: [PATCH 2/6] Make the consumer matrix run on Windows, and hold its own controls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six corrections to the compatibility check and the code it exercises. **Windows.** `execFileSync` creates a process directly, and the package managers and installed binaries are `.cmd` shims — a batch file is not an executable image, so it cannot be launched that way. Windows now goes through the command interpreter with every token quoted, and every other platform keeps direct execution: no shell, nothing to escape wrongly. Quoting wraps each token because a Windows temp path contains spaces, and doubles `"`, which is cmd's own escape. **The encapsulation case had no positive control.** Every assertion in it expects an import to fail, so it passed whether or not the package was installed. It now resolves the public root first and fails if that does not work — verified by removing the install for that shape, which turns the case red. **Yarn Berry needs the fixture declared self-contained.** Berry walks up from the working directory looking for a project root and adopts any `package.json` above it, which a temp directory often has. An empty `yarn.lock` in the fixture is Berry's own documented answer, and it is harmless under Classic. Per-manager preparation now exists for that. **Manager versions are pinned and printed.** An unpinned manager silently changes what was exercised, and Yarn Classic and Berry resolve differently enough that a pass under one is not a pass under the other — so both are in the matrix, and the script reports the version it actually saw alongside Node and the platform. **`EnvLike` had displaced the documentation of the function below it**, leaving two adjacent doc blocks and a function with none. The type carries its own comment and sits above the function's. **Comments state the invariant, not the history.** Several of the new comments narrated what used to happen and how it was found, which belongs in the private engineering record rather than in a public repository. They now say what holds and why it matters to a consumer. Verified: full suite green, and all seven consumer shapes pass under npm, Yarn Classic and Bun locally. Berry and Windows are exercised by CI — the Berry blocker above was reproduced and fixed from its own error message, but the complete Berry run has not been observed outside CI. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 37 ++++++++---- scripts/compat-matrix.mjs | 95 ++++++++++++++++++++++++------ scripts/copy-protect-templates.mjs | 11 ++-- src/stack.ts | 19 +++--- tests/protect/edge-bundle.test.ts | 11 ++-- 5 files changed, 123 insertions(+), 50 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 62d4d0f4..0f0ae4e8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -47,18 +47,25 @@ jobs: # Everything in `validate` tests the source, or `dist/` from inside the repository. Neither can see the # questions a consumer actually hits, because the tarball's METADATA decides them: which file an `exports` # condition resolves to, which declarations TypeScript reads beside it, whether `files` left something - # out. Two such defects were already shipped and were found by writing this check. + # out. A defect there leaves the source suite green while nothing can import the package. # - # Four managers because their resolution genuinely differs, and npm's answer does not generalise — that - # is the whole reason to run more than one. + # Managers resolve differently, so npm's answer does not generalise — that is the whole reason to run + # more than one. Versions are PINNED: an unpinned manager silently changes what was exercised, and Yarn + # Classic and Berry differ enough that a pass under one is not a pass under the other, so both run. + # The script prints the version it saw, so the label and the artifact cannot disagree. consumers: - name: 📦 Consumers on ${{ matrix.manager }} + name: 📦 Consumers on ${{ matrix.label }} runs-on: ubuntu-latest timeout-minutes: 15 strategy: fail-fast: false matrix: - manager: [npm, pnpm, yarn, bun] + include: + - { label: npm 10, manager: npm, setup: npm@10 } + - { label: pnpm 9, manager: pnpm, setup: pnpm@9 } + - { label: Yarn Classic 1.22, manager: yarn, setup: yarn@1.22.22 } + - { label: Yarn Berry 4, manager: yarn, setup: yarn@4 } + - { label: Bun 1, manager: bun, setup: bun } steps: - uses: actions/checkout@v4 @@ -67,13 +74,23 @@ jobs: node-version: 22.x cache: npm - - if: matrix.manager == 'pnpm' - uses: pnpm/action-setup@v4 - with: - version: 9 - - if: matrix.manager == 'bun' uses: oven-sh/setup-bun@v2 + with: + bun-version: latest + + - name: Pin the package manager + if: matrix.manager != 'bun' + run: npm install --global "${{ matrix.setup }}" + + # Berry defaults to its own linker and refuses a bare `yarn add` outside a project it initialised. + # `node-modules` is the layout the other managers produce and the one a consumer of this package will + # have, so it is what the fixtures must exercise. + - name: Configure Yarn Berry for a node_modules layout + if: startsWith(matrix.label, 'Yarn Berry') + run: | + yarn config set nodeLinker node-modules + yarn config set enableImmutableInstalls false - name: Install dependencies run: npm ci diff --git a/scripts/compat-matrix.mjs b/scripts/compat-matrix.mjs index 9e8647ad..89eb0554 100644 --- a/scripts/compat-matrix.mjs +++ b/scripts/compat-matrix.mjs @@ -6,18 +6,11 @@ // metadata rather than by the code: which file an `exports` condition resolves to, which declarations // TypeScript reads beside it, whether `files` left something out, whether the bin is executable. // -// Those failures are invisible from here and total for the consumer. Two were shipped and found by writing -// this: -// -// - `exports` had a single top-level `types` pointing at the ESM declarations, so a CommonJS TypeScript -// consumer resolved `require` for the runtime and ESM types for the shape. TypeScript concluded the -// target was an ES module and refused the `require` (TS1479) — while CommonJS JavaScript worked. The -// package was unusable from CommonJS TypeScript. -// - the published declarations referenced `NodeJS.ProcessEnv`, from `@types/node`, which this package -// does not depend on. A consumer without it could not compile against the types at all. +// Such failures are invisible from inside the repository and total for the consumer: the source suite is +// green while nothing can import the package. Each shape below states the consumption path it holds open. // // Run: node scripts/compat-matrix.mjs [--manager npm|pnpm|yarn|bun] -import { execFileSync } from 'node:child_process'; +import { execFileSync, execSync } from 'node:child_process'; import { mkdirSync, mkdtempSync, rmSync, writeFileSync, existsSync, readdirSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; @@ -28,8 +21,33 @@ const manager = (() => { })(); const ROOT = process.cwd(); -const run = (cmd, args, cwd, extraEnv = {}) => - execFileSync(cmd, args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env, ...extraEnv } }); +const WINDOWS = process.platform === 'win32'; + +/** + * Quote one token for the Windows command interpreter. + * + * Every token is wrapped, because a Windows temp path contains spaces and an unquoted one is split into + * two arguments. `"` doubles, which is cmd's own escape. + */ +const quoteForCmd = (token) => `"${String(token).replace(/"/g, '""')}"`; + +/** + * Run a tool and return its stdout. + * + * On Windows the package managers and the installed binaries are `.cmd` shims, and `execFileSync` cannot + * launch one: it creates a process directly, and a batch file is not an executable image. So Windows goes + * through the command interpreter with every token quoted, and every other platform keeps direct + * execution — no shell, nothing to quote, nothing to escape wrongly. + * + * See https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows. + */ +function run(cmd, args, cwd, extraEnv = {}) { + const options = { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], env: { ...process.env, ...extraEnv } }; + + if (!WINDOWS) return execFileSync(cmd, args, options); + + return execSync([cmd, ...args].map(quoteForCmd).join(' '), options); +} /** Install a local tarball. Each manager spells it differently, and their resolution differs — which is * the point of running all four rather than assuming npm's answer generalises. */ @@ -40,6 +58,18 @@ const INSTALL = { bun: (tgz, deps) => ['bun', ['add', tgz, ...deps]], }; +/** + * What a fixture needs before its manager will treat it as its own project. + * + * Yarn Berry walks up from the working directory looking for a project root, and adopts any `package.json` + * it finds above — a temp directory is often inside one. An empty `yarn.lock` declares the fixture + * self-contained, which is Berry's own documented answer. Harmless under Classic, which would write one + * anyway. + */ +const PREPARE = { + yarn: (dir) => writeFileSync(path.join(dir, 'yarn.lock'), ''), +}; + if (!INSTALL[manager]) { console.error(`Unknown manager: ${manager}. Known: ${Object.keys(INSTALL).join(', ')}`); process.exit(2); @@ -63,8 +93,8 @@ const TSCONFIG = (module_, resolution, types) => JSON.stringify({ /** * Each consumer shape, and what it is here to prove. * - * `cjs-ts-no-node-types` is not redundant with `cjs-ts`: it is the only one that catches published - * declarations depending on `@types/node` without declaring it, because every other fixture installs it. + * `cjs-ts-no-node-types` is not redundant with `cjs-ts`: every other TypeScript fixture installs + * `@types/node`, so it is the only one that holds the public declarations free of it. */ const SHAPES = [ { @@ -111,14 +141,33 @@ const SHAPES = [ name: 'encapsulation', why: 'nothing outside exports is reachable', pkg: { type: 'module' }, deps: [], files: { - 'probe.mjs': "let leaked = [];\nfor (const p of ['@patchstack/connect/dist/cli.js', '@patchstack/connect/package.json', '@patchstack/connect/src/index.ts']) {\n try { await import(p); leaked.push(p); } catch {}\n}\nif (leaked.length) throw new Error('reachable outside exports: ' + leaked.join(', '));\n", + // The positive control comes first and is not optional: every other assertion here expects an import + // to FAIL, so without it the case passes when the package is absent entirely. + 'probe.mjs': [ + "const root = await import('@patchstack/connect');", + "if (typeof root.buildWirePayload !== 'function') {", + " throw new Error('control failed: the public root does not resolve, so nothing below proves encapsulation');", + '}', + '', + 'const leaked = [];', + "for (const subpath of ['@patchstack/connect/dist/cli.js', '@patchstack/connect/package.json', '@patchstack/connect/src/index.ts']) {", + ' try {', + ' await import(subpath);', + ' leaked.push(subpath);', + " } catch { /* expected: not named in `exports` */ }", + '}', + "if (leaked.length) throw new Error('reachable outside exports: ' + leaked.join(', '));", + '', + ].join('\n'), }, check: (dir) => run('node', ['probe.mjs'], dir), }, ]; -const tsc = (dir) => path.join(dir, 'node_modules', '.bin', process.platform === 'win32' ? 'tsc.cmd' : 'tsc'); -const bin = (dir) => path.join(dir, 'node_modules', '.bin', process.platform === 'win32' ? 'patchstack-connect.cmd' : 'patchstack-connect'); +// A local binary is addressed by path, so on Windows it needs the shim's extension spelled out. +const localBin = (dir, name) => path.join(dir, 'node_modules', '.bin', WINDOWS ? `${name}.cmd` : name); +const tsc = (dir) => localBin(dir, 'tsc'); +const bin = (dir) => localBin(dir, 'patchstack-connect'); /** Pack the real artifact. `--ignore-scripts` is deliberately NOT passed: `prepare` builds `dist/`. */ function packTarball(into) { @@ -135,7 +184,16 @@ let failures = 0; try { const tarball = packTarball(work); - console.log(`packed ${path.basename(tarball)}; installing with ${manager}\n`); + + // Which manager, at which version. The label alone says "Yarn", and Classic and Berry resolve + // differently enough that a pass under one is not a pass under the other. + let managerVersion = 'unknown'; + try { + managerVersion = run(manager, ['--version'], ROOT).trim().split('\n').pop(); + } catch { /* the install below fails loudly if the manager is missing */ } + + console.log(`packed ${path.basename(tarball)}`); + console.log(`node ${process.version} · ${manager} ${managerVersion} · ${process.platform}\n`); for (const shape of SHAPES) { const dir = path.join(work, shape.name); @@ -144,6 +202,7 @@ try { for (const [file, body] of Object.entries(shape.files ?? {})) writeFileSync(path.join(dir, file), body); try { + PREPARE[manager]?.(dir); const [cmd, args] = INSTALL[manager](tarball, shape.deps); run(cmd, args, dir); shape.check(dir); diff --git a/scripts/copy-protect-templates.mjs b/scripts/copy-protect-templates.mjs index b1ca860f..adc5c7ca 100644 --- a/scripts/copy-protect-templates.mjs +++ b/scripts/copy-protect-templates.mjs @@ -10,12 +10,11 @@ console.log('copied protect templates -> dist/protect/templates'); // Ship the hand-authored declarations for the `@patchstack/connect/protect` subpath (the runtime // is plain JS, so tsup doesn't emit these). Referenced by the "./protect" export's `types`. copyFileSync('src/protect/protect.d.ts', 'dist/protect.d.ts'); -// The same declarations under the CommonJS extension. A CJS TypeScript consumer resolves the `require` -// condition, and TypeScript then reads the types beside it: given an ESM `.d.ts` it concludes the target is -// an ES module and refuses the `require` outright (TS1479) — while the CJS runtime works perfectly. So the -// package was unusable from CommonJS TypeScript and fine from CommonJS JavaScript. +// The same declarations under the CommonJS extension, because `exports` routes the `require` condition to +// them. TypeScript reads the declarations beside the condition it resolved, and an ESM `.d.ts` there tells +// it the target is an ES module — so it refuses the `require` (TS1479) even though the runtime works. // -// A copy rather than a second generated file, because tsup emits `index.d.ts` and `index.d.cts` -// byte-identical for the root entry; there is nothing per-format to express. +// A copy rather than a second generated file: tsup emits the root's `.d.ts` and `.d.cts` byte-identical, +// so there is nothing per-format to express. copyFileSync('src/protect/protect.d.ts', 'dist/protect.d.cts'); console.log('copied protect types -> dist/protect.d.ts, dist/protect.d.cts'); diff --git a/src/stack.ts b/src/stack.ts index 04e90381..f7cbcfba 100644 --- a/src/stack.ts +++ b/src/stack.ts @@ -106,22 +106,21 @@ const HOSTING_ENV_PATTERNS: readonly RegExp[] = [ /^FUNCTION_/, ]; +/** + * An environment, described structurally so it needs no ambient types. + * + * `@types/node` is not a dependency of this package, so nothing in its public surface may name + * `NodeJS.ProcessEnv`: a consumer without those types cannot compile against a signature that does. + * `process.env` satisfies this, and so does a plain object. + */ +export type EnvLike = Record; + /** * Return the sorted NAMES of hosting-related environment variables present in * `env`. Only names are surfaced — never values — so a build fingerprint can * distinguish "deployed on Cloudflare" from "deployed on Vercel" without ever * disclosing a secret. */ -/** - * The shape of an environment, structurally. - * - * Not `NodeJS.ProcessEnv`: that type comes from `@types/node`, which this package does not declare as a - * dependency, so a consumer without it could not compile against these signatures at all — the published - * declarations referenced a namespace their project had never heard of. `process.env` satisfies this, and - * so does a plain object, which is what the tests pass. - */ -export type EnvLike = Record; - export function collectHostingEnvKeys(env: EnvLike = process.env): string[] { return Object.keys(env) .filter((key) => HOSTING_ENV_PATTERNS.some((pattern) => pattern.test(key))) diff --git a/tests/protect/edge-bundle.test.ts b/tests/protect/edge-bundle.test.ts index 065ea75e..2c0905be 100644 --- a/tests/protect/edge-bundle.test.ts +++ b/tests/protect/edge-bundle.test.ts @@ -63,9 +63,9 @@ describe('edge bundle', () => { for (const cond of ['workerd', 'worker', 'edge-light', 'deno', 'browser']) { expect(protect[cond]).toBe('./dist/protect.edge.js'); } - // Node still gets the full build. Nested now, because the types differ per format: a CommonJS - // consumer resolving `require` must be handed the `.d.cts` declarations, or TypeScript reads the ESM - // ones, concludes the target is an ES module and refuses the `require` outright. + // Node gets the full build, and the types are nested per format: a CommonJS consumer resolving + // `require` must be handed the `.d.cts` declarations, or TypeScript reads the ESM ones, concludes the + // target is an ES module and refuses the `require`. expect(protect.import).toEqual({ types: './dist/protect.d.ts', default: './dist/protect.js' }); expect(protect.require).toEqual({ types: './dist/protect.d.cts', default: './dist/protect.cjs' }); @@ -77,9 +77,8 @@ describe('edge bundle', () => { }); it('ships the declarations both format conditions point at', () => { - // The map can name a file that the build never produces, and the failure surfaces three layers away as - // "cannot find module" in a consumer's project. `protect.d.cts` did not exist at all while `require` - // was already being advertised. + // The map can name a file the build never produces, and the failure surfaces three layers away as + // "cannot find module" in a consumer's project. const pkg = JSON.parse(readFileSync(root + 'package.json', 'utf8')); const shipped = pkg.files as string[]; From b3d4f00af2c37d3c3bb77ce4eab95a51ab5d762f Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 27 Aug 2026 16:34:59 +0200 Subject: [PATCH 3/6] Install Berry from its own distribution, build before testing, pin exactly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Yarn Berry is installed from `@yarnpkg/cli-dist`, which is the distribution carrying the `yarn` binary at 4.x. The `yarn` package on npm stops at 2.x, so a `yarn@4` install cannot resolve and the job ends before any fixture runs. Windows quotes the command word only when it is a path. A bare name has to stay unquoted: `npm` there is `npm.cmd`, found through `PATH` and `PATHEXT`, and the shim locates its own installation with `%~dp0` — the directory of the batch file. Quoting the name makes that the working directory instead, so npm looks for its CLI under whatever project is current. Absolute paths are quoted, since the binaries in `node_modules/.bin` are addressed by path and a Windows temp or workspace path contains spaces. The interpreter is invoked explicitly as `cmd /d /s /c ""` with `windowsVerbatimArguments`: `/s` strips exactly the outermost quote pair and takes the rest verbatim, and the verbatim flag stops Node re-escaping a line that is already correct. The build runs before the tests. Several tests only assert against `dist/`, and one of them returns early and green when it is absent — so the check that the built edge bundle carries no static Node-builtin import was not being made. Two comments claiming CI tests before building are corrected; they now describe what happens. Every manager version is exact — npm 11.6.0, pnpm 9.15.4, Yarn 1.22.22, Yarn 4.18.0, Bun 1.2.21 — and the step prints the version on PATH before any fixture runs, so an install that resolved to something else is visible rather than inferred from a later failure. A floating major means a passing run happened for a reason nobody chose. Berry needed no linker configuration: 4.x uses a `node_modules` layout for a project with no `.yarnrc.yml`, so the two config steps asserting otherwise are removed. The empty `yarn.lock` the script writes is what Berry actually requires. Verified locally: all seven consumer shapes pass under npm 11.6.0, Yarn Classic 1.22.22, Yarn Berry 4.18.0 and Bun. Windows remains CI-only. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 51 +++++++++++--------- scripts/compat-matrix.mjs | 38 +++++++++++---- tests/protect/edge-export-resolution.test.ts | 3 +- tests/protect/edge-safe.test.ts | 4 +- 4 files changed, 61 insertions(+), 35 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0f0ae4e8..cbed1c10 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,10 +49,16 @@ jobs: # condition resolves to, which declarations TypeScript reads beside it, whether `files` left something # out. A defect there leaves the source suite green while nothing can import the package. # - # Managers resolve differently, so npm's answer does not generalise — that is the whole reason to run - # more than one. Versions are PINNED: an unpinned manager silently changes what was exercised, and Yarn - # Classic and Berry differ enough that a pass under one is not a pass under the other, so both run. - # The script prints the version it saw, so the label and the artifact cannot disagree. + # Managers resolve differently, so npm's answer does not generalise — that is the whole reason to run more + # than one. Yarn Classic and Berry differ enough that a pass under one is not a pass under the other, so + # both are here. + # + # Every version is EXACT. A floating major silently changes what was exercised, and a run that passes for + # a reason nobody chose is not evidence. The script also prints the version it resolved, so the label and + # the artifact cannot disagree. + # + # Berry does not come from the `yarn` package on npm — that line stops at 2.x — so it is installed from + # `@yarnpkg/cli-dist`, which is the distribution that carries the `yarn` binary. consumers: name: 📦 Consumers on ${{ matrix.label }} runs-on: ubuntu-latest @@ -61,11 +67,11 @@ jobs: fail-fast: false matrix: include: - - { label: npm 10, manager: npm, setup: npm@10 } - - { label: pnpm 9, manager: pnpm, setup: pnpm@9 } - - { label: Yarn Classic 1.22, manager: yarn, setup: yarn@1.22.22 } - - { label: Yarn Berry 4, manager: yarn, setup: yarn@4 } - - { label: Bun 1, manager: bun, setup: bun } + - { label: npm 11.6.0, manager: npm, setup: 'npm@11.6.0' } + - { label: pnpm 9.15.4, manager: pnpm, setup: 'pnpm@9.15.4' } + - { label: Yarn Classic 1.22.22, manager: yarn, setup: 'yarn@1.22.22' } + - { label: Yarn Berry 4.18.0, manager: yarn, setup: '@yarnpkg/cli-dist@4.18.0' } + - { label: Bun 1.2.21, manager: bun, setup: '' } steps: - uses: actions/checkout@v4 @@ -77,20 +83,15 @@ jobs: - if: matrix.manager == 'bun' uses: oven-sh/setup-bun@v2 with: - bun-version: latest + bun-version: 1.2.21 - - name: Pin the package manager + - name: Install the package manager at an exact version if: matrix.manager != 'bun' - run: npm install --global "${{ matrix.setup }}" - - # Berry defaults to its own linker and refuses a bare `yarn add` outside a project it initialised. - # `node-modules` is the layout the other managers produce and the one a consumer of this package will - # have, so it is what the fixtures must exercise. - - name: Configure Yarn Berry for a node_modules layout - if: startsWith(matrix.label, 'Yarn Berry') run: | - yarn config set nodeLinker node-modules - yarn config set enableImmutableInstalls false + npm install --global "${{ matrix.setup }}" + # The version actually on PATH, before any fixture runs. An install that resolved to something + # else — or to nothing — must be visible here rather than inferred from a later failure. + ${{ matrix.manager }} --version - name: Install dependencies run: npm ci @@ -148,12 +149,16 @@ jobs: - name: Typecheck run: npm run typecheck - - name: Test - run: npm test - + # Before the tests. Several of them only run once `dist/` exists — the edge export-resolution and + # edge-safe checks among them — and in the other order they skip, which is indistinguishable from + # passing. Two dist-dependent checks are rerun explicitly below with a flag that refuses to skip; + # the rest rely on this ordering, so it is the ordering that has to be right. - name: Build run: npm run build + - name: Test + run: npm test + # After the build, because it drives the built bin. npm installs a bin as a SYMLINK, so this is the # only invocation shape that catches an entry point which runs when called directly and does nothing # when installed — silently, with exit 0. diff --git a/scripts/compat-matrix.mjs b/scripts/compat-matrix.mjs index 89eb0554..3184eabb 100644 --- a/scripts/compat-matrix.mjs +++ b/scripts/compat-matrix.mjs @@ -10,7 +10,7 @@ // green while nothing can import the package. Each shape below states the consumption path it holds open. // // Run: node scripts/compat-matrix.mjs [--manager npm|pnpm|yarn|bun] -import { execFileSync, execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import { mkdirSync, mkdtempSync, rmSync, writeFileSync, existsSync, readdirSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; @@ -23,21 +23,34 @@ const manager = (() => { const ROOT = process.cwd(); const WINDOWS = process.platform === 'win32'; +/** Quote one argument for the Windows interpreter: wrap it, and double any `"`, which is cmd's escape. */ +const quoteArg = (token) => `"${String(token).replace(/"/g, '""')}"`; + /** - * Quote one token for the Windows command interpreter. + * The command word, quoted only when it is a path. + * + * A BARE name must stay unquoted. `npm` on Windows is `npm.cmd`, found through `PATH` and `PATHEXT`, and + * the shim resolves its own installation with `%~dp0` — the directory of the batch file it is running. + * Quoting the name changes how cmd resolves it, `%~dp0` becomes the working directory, and npm then looks + * for its own CLI under whatever project happens to be current. * - * Every token is wrapped, because a Windows temp path contains spaces and an unquoted one is split into - * two arguments. `"` doubles, which is cmd's own escape. + * An ABSOLUTE path must be quoted: the binaries in `node_modules/.bin` are addressed by path, and a + * Windows temp or workspace path contains spaces. */ -const quoteForCmd = (token) => `"${String(token).replace(/"/g, '""')}"`; +const quoteCommand = (cmd) => (path.isAbsolute(cmd) ? quoteArg(cmd) : cmd); /** * Run a tool and return its stdout. * - * On Windows the package managers and the installed binaries are `.cmd` shims, and `execFileSync` cannot - * launch one: it creates a process directly, and a batch file is not an executable image. So Windows goes - * through the command interpreter with every token quoted, and every other platform keeps direct - * execution — no shell, nothing to quote, nothing to escape wrongly. + * On Windows the package managers and installed binaries are `.cmd` shims. `execFileSync` creates a process + * directly and a batch file is not an executable image, so it cannot launch one — the interpreter has to be + * invoked explicitly. Every other platform executes directly: no shell, nothing to quote, nothing to escape + * wrongly. + * + * The Windows form is `cmd /d /s /c ""` with `windowsVerbatimArguments`, which is the only + * combination that behaves predictably: `/s` makes cmd strip exactly the outermost quote pair and take the + * rest verbatim, and `windowsVerbatimArguments` stops Node re-escaping the line first. Without both, the + * quotes around individual arguments are rewritten and a path with a space breaks. * * See https://nodejs.org/api/child_process.html#spawning-bat-and-cmd-files-on-windows. */ @@ -46,7 +59,12 @@ function run(cmd, args, cwd, extraEnv = {}) { if (!WINDOWS) return execFileSync(cmd, args, options); - return execSync([cmd, ...args].map(quoteForCmd).join(' '), options); + const line = [quoteCommand(cmd), ...args.map(quoteArg)].join(' '); + + return execFileSync(process.env.ComSpec || 'cmd.exe', ['/d', '/s', '/c', `"${line}"`], { + ...options, + windowsVerbatimArguments: true, + }); } /** Install a local tarball. Each manager spells it differently, and their resolution differs — which is diff --git a/tests/protect/edge-export-resolution.test.ts b/tests/protect/edge-export-resolution.test.ts index eb9df560..b0646cfc 100644 --- a/tests/protect/edge-export-resolution.test.ts +++ b/tests/protect/edge-export-resolution.test.ts @@ -41,7 +41,8 @@ async function bundleWith(conditions: string[]): Promise<{ ok: boolean; text: st describe('edge conditional-export resolution', () => { beforeAll(() => { - // The exports map points at dist/, so the artifacts must exist. CI runs tests before the build. + // The exports map points at dist/, so the artifacts must exist. CI builds first; this covers a local + // run in a fresh checkout, where building here is cheaper than skipping and reporting green. if (!existsSync(join(repo, 'dist', 'protect.edge.js')) || !existsSync(join(repo, 'dist', 'protect.js'))) { execFileSync('npm', ['run', 'build'], { cwd: repo, stdio: 'ignore' }); } diff --git a/tests/protect/edge-safe.test.ts b/tests/protect/edge-safe.test.ts index dfdc1337..8e03217a 100644 --- a/tests/protect/edge-safe.test.ts +++ b/tests/protect/edge-safe.test.ts @@ -44,7 +44,9 @@ describe('protect runtime stays edge-safe', () => { it('the built protect bundle has no static Node-builtin import (when dist is fresh)', () => { const dist = fileURLToPath(new URL('../../dist/protect.js', import.meta.url)); - if (!existsSync(dist)) return; // dist is gitignored and CI tests before building + // `dist/` is gitignored, so a fresh local checkout has nothing to assert against. CI builds before it + // tests, which is what makes this run there rather than returning green having checked nothing. + if (!existsSync(dist)) return; // Only assert against a build that reflects the current sources: a STALE dist (left by a build on // another branch) would otherwise fail this spuriously. The source-graph assertion above is the // real invariant and always runs. From 19927c9b3a5818b944177b9ea8c70ff2abbca84e Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 27 Aug 2026 16:38:28 +0200 Subject: [PATCH 4/6] Pin the Yarn layout in the fixture instead of inheriting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Berry resolved nothing on Linux: the fixtures installed but no shape could import the package. Its linker depends on the Yarn version and on any `.yarnrc.yml` above the fixture, so the same script produced a `node_modules` layout on one machine and Plug'n'Play on another — and under Plug'n'Play nothing is in `node_modules`, so `node probe.mjs` cannot resolve the package at all. The fixture now states `nodeLinker: node-modules` alongside the empty `yarn.lock`, in the script rather than in the workflow, so it holds wherever the script runs. Confirmed to be the deciding factor: with `nodeLinker: pnp` all seven shapes fail, with `node-modules` all seven pass. Plug'n'Play is a real Berry configuration and this matrix does not cover it. The comment says so, because a matrix that quietly exercises one layout while reading as "Yarn works" is the more expensive outcome. Verified locally under npm, Yarn Classic 1.22.22, Yarn Berry 4.18.0 and Bun. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/compat-matrix.mjs | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/scripts/compat-matrix.mjs b/scripts/compat-matrix.mjs index 3184eabb..58b9eec6 100644 --- a/scripts/compat-matrix.mjs +++ b/scripts/compat-matrix.mjs @@ -77,15 +77,26 @@ const INSTALL = { }; /** - * What a fixture needs before its manager will treat it as its own project. + * What a fixture needs before its manager will treat it as its own project, and resolve the way a + * consumer of this package does. * - * Yarn Berry walks up from the working directory looking for a project root, and adopts any `package.json` - * it finds above — a temp directory is often inside one. An empty `yarn.lock` declares the fixture - * self-contained, which is Berry's own documented answer. Harmless under Classic, which would write one - * anyway. + * Two things for Yarn, and both are stated rather than left to a default: + * + * `yarn.lock` — Berry walks up from the working directory looking for a project root and adopts any + * `package.json` above it, which a temp directory usually has. An empty lockfile declares the fixture + * self-contained, which is Berry's own documented answer. Harmless under Classic, which writes one anyway. + * + * `nodeLinker: node-modules` — this is the layout being tested, so it is pinned instead of inherited. + * Berry's linker otherwise depends on its version and on any `.yarnrc.yml` above the fixture, so the same + * script resolves differently on two machines, and a pass says nothing about which layout was exercised. + * Under Plug'n'Play nothing is in `node_modules` at all and a plain `node probe.mjs` cannot resolve the + * package — a real Berry configuration, and a separate shape this matrix does not cover. */ const PREPARE = { - yarn: (dir) => writeFileSync(path.join(dir, 'yarn.lock'), ''), + yarn: (dir) => { + writeFileSync(path.join(dir, 'yarn.lock'), ''); + writeFileSync(path.join(dir, '.yarnrc.yml'), 'nodeLinker: node-modules\n'); + }, }; if (!INSTALL[manager]) { From 16f46c367ba2d8c81a122c090e0420bf256fa34a Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Thu, 27 Aug 2026 16:47:42 +0200 Subject: [PATCH 5/6] Name the layout the Berry job exercises The check read "Yarn Berry 4.18.0" while testing Berry with a `node_modules` layout only. Plug'n'Play is a real Berry configuration and is not covered, so the label claimed more than the job holds. It now says `Yarn Berry 4.18.0 (node-modules)`. Renaming is safe here: the consumer jobs are not required status checks, so no protection rule references the old name. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cbed1c10..41dee9cc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -70,7 +70,10 @@ jobs: - { label: npm 11.6.0, manager: npm, setup: 'npm@11.6.0' } - { label: pnpm 9.15.4, manager: pnpm, setup: 'pnpm@9.15.4' } - { label: Yarn Classic 1.22.22, manager: yarn, setup: 'yarn@1.22.22' } - - { label: Yarn Berry 4.18.0, manager: yarn, setup: '@yarnpkg/cli-dist@4.18.0' } + # The layout is in the label: this exercises Berry resolving the export map through + # `node_modules`, which is what the fixture pins. Plug'n'Play is a separate shape and is not + # covered, so the check must not read as "Berry works". + - { label: Yarn Berry 4.18.0 (node-modules), manager: yarn, setup: '@yarnpkg/cli-dist@4.18.0' } - { label: Bun 1.2.21, manager: bun, setup: '' } steps: - uses: actions/checkout@v4 From 387b6110924641cd38026bc7b58418781a8b0871 Mon Sep 17 00:00:00 2001 From: Dave Jong Date: Fri, 28 Aug 2026 09:48:25 +0200 Subject: [PATCH 6/6] List the verification commands in the order they must run The pre-pull-request checklist named typecheck, test, build, while the section above it establishes that the build has to come first: several tests only assert against `dist/`, and one returns early and green without it. The checklist now matches, and points at that reason rather than restating it. Co-Authored-By: Claude Opus 5 (1M context) --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 667207f0..4d1ef69f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,7 +46,7 @@ reads exactly like passing. ## Before you open a pull request -- `npm run typecheck`, `npm test`, `npm run build` all clean. +- `npm run typecheck`, `npm run build`, `npm test` all clean — in that order, for the reason above. - `npm pack --dry-run` if you touched anything about packaging. `files` in `package.json` is the authoritative allowlist of what ships. - `npm run test:consumers` if you touched `exports`, `files`, the build config or anything about types. It