diff --git a/CHANGELOG.md b/CHANGELOG.md index ae4845e..e5e1b9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] + +### Fixed + +- **The install leg substitutes a sibling whose contract moved without its version moving + (usetheokit/theokit#659).** Check D skipped any sibling whose version number the registry already + had. That answers "does this version exist?", which is a different question from "is the published + copy the one this workspace is about to publish?". + + A release that widens a peer range across two workspace packages moves the contract and not the + version, because changesets bumps at version time and not at pull-request time. So the leg packed + tomorrow's package, resolved yesterday's sibling from npm, and reported two copies of a peer — a + pair publication never produces. It is a required check, so the release could not merge, and no + ordering of the old design avoided it: bumping first makes the packed tarball ask for a version npm + does not have, and the install answers ETARGET instead. + + A sibling now also earns its local tarball when the registry serves its version with different + `dependencies`, `peerDependencies` or `optionalDependencies`. The narrowness is deliberate — an + identical contract is still installed from the registry, which is what makes the check test what a + consumer resolves. Absent evidence answers "unchanged": a registry that returns no manifest must + not read as "the contract moved", or every sibling gets a local tarball and the leg tests nothing. + + Each substitution now says WHICH gap it filled. The single note claimed the registry did not have + the version, which is false for this case, and a note that names the wrong reason is worse than + none because it is read and believed. ### Added - **The back-merge says how long its pull request has been open (usetheokit/theokit-sdk#566).** diff --git a/packages/dep-check/index.mjs b/packages/dep-check/index.mjs index d952f89..52d6e91 100755 --- a/packages/dep-check/index.mjs +++ b/packages/dep-check/index.mjs @@ -23,7 +23,7 @@ import { parseArgs } from "node:util"; import { ceilingDrift, consumersLeftBehind, floorsInOwnWorkspace, groupUntestedFloors, installedDrift, isSibling, peerInstallSpecs, pinnableSiblings, rangeFloor, sharedFloor, unpublishedSiblings, unpublishedWorkspaceVersions, untestedFloors } from "./src/checks.mjs"; import { findPublishablePackages, resolveInstalledVersion, siblingReferences } from "./src/ecosystem.mjs"; -import { consumersOf, discoverEcosystemPackages, latestVersion, packument, publishedVersions } from "./src/registry.mjs"; +import { consumersOf, discoverEcosystemPackages, latestVersion, packument, publishedManifest, publishedVersions } from "./src/registry.mjs"; import { batchedWithDeps, detectBuildScript, detectPackageManager, pinOverrides } from "./src/package-manager.mjs"; import { installFromTarball } from "./src/tarball.mjs"; @@ -222,12 +222,23 @@ async function commandInstall(root) { dir: p.dir, // Carried so the substitution can follow a substituted tarball's own unpublished asks. references: siblingReferences(p.manifest, isSibling), + // Carried so the substitution can compare the contract this workspace declares against the one + // the registry serves at the same version number (usetheokit/theokit#659). + manifest: p.manifest, })); const substitutions = []; // Built once for every workspace package, not per reference: the substitution walks // transitively, so it needs an answer for siblings the package under test never names. const published = {}; for (const p of workspace) published[p.name] = await publishedVersions(p.name); + // What the registry serves AT the workspace's own version, so a contract that moved without the + // version moving is visible. Only that one version is fetched: it is the only one the substitution + // decision compares against (usetheokit/theokit#659). + const manifests = {}; + for (const p of workspace) { + const served = await publishedManifest(p.name, p.version); + if (served) manifests[p.name] = { [p.version]: served }; + } // What the registry SERVES, as opposed to everything it holds. `peerInstallSpecs` asks whether // that version satisfies a declared range, which is the only way to tell a real prerelease floor // (`>=4.63.4-next.0`, latest does not satisfy) from a prerelease SENTINEL (`>=0.1.0-alpha.0`, @@ -238,7 +249,7 @@ async function commandInstall(root) { const refs = siblingReferences(pkg.manifest, isSibling); // What the registry cannot answer yet, taken from the workspace instead. Only the gap — // see `unpublishedSiblings`. - const localSiblings = unpublishedSiblings({ references: refs, workspace, published }); + const localSiblings = unpublishedSiblings({ references: refs, workspace, published, manifests }); // Computed AFTER the substitution and from it: a peer this cut is about to publish must not // also be asked for as `@latest`, or the registry copy overrides the packed one on the same // command line — see `peerInstallSpecs`. @@ -265,7 +276,11 @@ async function commandInstall(root) { // consumer can install today — and a reader deciding what a green D means has to know which // one they got. Same reason `untestedFloors` prints (#6). for (const s of substitutions) { - console.log(` note: ${s.pkg} was installed against ${s.name}@${s.version} packed from this workspace — the registry does not have that version yet, so this cut is testing what it is about to publish`); + const why = + s.reason === 'contract-moved' + ? 'the registry serves that version with different dependency ranges, so this cut is testing the pair it is about to publish rather than one publication never produces' + : 'the registry does not have that version yet, so this cut is testing what it is about to publish'; + console.log(` note: ${s.pkg} was installed against ${s.name}@${s.version} packed from this workspace — ${why}`); } return findings.length === 0 ? 0 : 1; } diff --git a/packages/dep-check/src/checks.mjs b/packages/dep-check/src/checks.mjs index 15a5d66..b0d0499 100644 --- a/packages/dep-check/src/checks.mjs +++ b/packages/dep-check/src/checks.mjs @@ -314,7 +314,30 @@ export function peerInstallSpecs({ references, localSiblings = [], latest = new return [...specs]; } -export function unpublishedSiblings({ references, workspace, published }) { +/** The fields a consumer's installer resolves. `devDependencies` never reaches a consumer's tree. */ +const INSTALLED_FIELDS = ["dependencies", "peerDependencies", "optionalDependencies"]; + +/** + * Does the workspace copy declare a different installed contract than the registry serves at the + * SAME version number? + * + * Absent evidence answers NO, deliberately. A registry that did not return a manifest, or a caller + * that passed none, must not read as "the contract changed" — that would hand a local tarball to + * every sibling and stop the check testing what a consumer actually resolves, which is the reason + * the version skip exists (usetheokit/theokit#659). + */ +function contractMoved(local, publishedManifest) { + if (!publishedManifest || !local.manifest) return false; + return INSTALLED_FIELDS.some((field) => !sameRanges(local.manifest[field], publishedManifest[field])); +} + +function sameRanges(a = {}, b = {}) { + const keys = new Set([...Object.keys(a), ...Object.keys(b)]); + for (const k of keys) if (a[k] !== b[k]) return false; + return true; +} + +export function unpublishedSiblings({ references, workspace, published, manifests = {} }) { const byName = new Map(workspace.map((p) => [p.name, p])); const out = []; const seen = new Set(); @@ -333,8 +356,22 @@ export function unpublishedSiblings({ references, workspace, published }) { // Missing is not empty: an unreachable registry must not read as "nothing is published", // or every sibling gets a local tarball and the check tests nothing real. if (!Array.isArray(versions) || !versions.length) continue; - if (versions.includes(local.version)) continue; - out.push({ name: dep, version: local.version, dir: local.dir }); + // "The registry has this version number" is a DIFFERENT question from "the registry has the + // copy this workspace is about to publish". A release that changes a dependency contract across + // two workspace packages moves the contract without moving the version, because changesets bumps + // at version time and not at pull-request time. Skipping on the number alone installed tomorrow's + // package against yesterday's sibling and reported a duplicate peer — a pair publication never + // produces, and one no ordering of the old design could avoid (usetheokit/theokit#659). + if (versions.includes(local.version) && !contractMoved(local, manifests[dep]?.[local.version])) continue; + out.push({ + name: dep, + version: local.version, + dir: local.dir, + // Which of the two gaps this substitution fills. The report explains itself with this, + // and "the registry does not have that version" is FALSE for a contract that moved + // without the version moving (usetheokit/theokit#659). + reason: versions.includes(local.version) ? 'contract-moved' : 'unpublished', + }); for (const r of local.references ?? []) queue.push(r.dep); } return out; diff --git a/packages/dep-check/src/registry.mjs b/packages/dep-check/src/registry.mjs index 55df5a0..48dbe30 100644 --- a/packages/dep-check/src/registry.mjs +++ b/packages/dep-check/src/registry.mjs @@ -34,6 +34,19 @@ export async function latestVersion(name) { return doc?.["dist-tags"]?.latest ?? null; } +/** + * The manifest the registry SERVES for one exact version, or null when it serves none. + * + * A version number is not a contract: the registry can hold `0.8.0` while the workspace is about to + * republish a different `0.8.0` with a widened peer range, because changesets moves the version at + * version time and not at pull-request time. Reading the served manifest is what lets the install + * leg tell those two apart (usetheokit/theokit#659). + */ +export async function publishedManifest(name, version) { + const doc = await packument(name); + return doc?.versions?.[version] ?? null; +} + /** Every published version, in registry order. */ export async function publishedVersions(name) { const doc = await packument(name); diff --git a/packages/dep-check/test/checks.test.mjs b/packages/dep-check/test/checks.test.mjs index 3ecb2fd..5aee5ca 100644 --- a/packages/dep-check/test/checks.test.mjs +++ b/packages/dep-check/test/checks.test.mjs @@ -247,6 +247,7 @@ describe("unpublishedSiblings — what check D cannot get from the registry yet" const out = unpublishedSiblings({ references: [{ dep: "@theokit/agents" }], workspace, published }); expect(out.map((s) => s.name)).toEqual(["@theokit/agents"]); expect(out[0].version).toBe("12.1.0"); + expect(out[0].reason).toBe("unpublished"); }); it("test_leaves_a_sibling_the_registry_already_has_to_be_installed_from_the_registry", () => { @@ -261,6 +262,48 @@ describe("unpublishedSiblings — what check D cannot get from the registry yet" expect(unpublishedSiblings({ references: [{ dep: "@theokit/nowhere" }], workspace, published })).toEqual([]); }); + it("test_substitutes_a_published_version_whose_local_contract_no_longer_matches", () => { + // usetheokit/theokit#659. The skip above asks "is this version number published?", which is a + // different question from "is the published copy the one this workspace is about to publish?". + // A release that widens a peer range across two workspace packages moves the CONTRACT without + // moving the version number — changesets bumps at version time, not at pull-request time. So the + // gate installed tomorrow's package against yesterday's sibling and reported two copies of a + // peer, a pair publication never produces. Measured on usetheokit/theokit#658, which it blocked. + const ws = [ + { + name: "@theokit/presenter", + version: "0.8.0", + dir: "/w/packages/presenter", + manifest: { peerDependencies: { "@theokit/sdk": "^4.49.0 || ^5.0.0" } }, + }, + ]; + const pub = { "@theokit/presenter": ["0.7.0", "0.8.0"] }; + const manifests = { "@theokit/presenter": { "0.8.0": { peerDependencies: { "@theokit/sdk": "^4.49.0" } } } }; + const out = unpublishedSiblings({ references: [{ dep: "@theokit/presenter" }], workspace: ws, published: pub, manifests }); + expect(out.map((s) => s.name)).toEqual(["@theokit/presenter"]); + // The report explains every substitution, and the older explanation — "the registry does not + // have that version yet" — is FALSE here: it has it, with a different contract. A note that + // names the wrong reason is worse than none, because it is read and believed. + expect(out[0].reason).toBe("contract-moved"); + }); + + it("test_leaves_a_published_version_alone_when_the_local_contract_is_identical", () => { + // The narrowness is the point (#659). Substituting whenever a package is in the workspace would + // stop testing what a consumer resolves — the reason the skip exists at all. Only a contract + // that DIFFERS earns the local tarball. + const ws = [ + { + name: "@theokit/presenter", + version: "0.8.0", + dir: "/w/packages/presenter", + manifest: { peerDependencies: { "@theokit/sdk": "^4.49.0" }, dependencies: {} }, + }, + ]; + const pub = { "@theokit/presenter": ["0.8.0"] }; + const manifests = { "@theokit/presenter": { "0.8.0": { peerDependencies: { "@theokit/sdk": "^4.49.0" } } } }; + expect(unpublishedSiblings({ references: [{ dep: "@theokit/presenter" }], workspace: ws, published: pub, manifests })).toEqual([]); + }); + it("test_follows_a_substituted_tarballs_own_unpublished_asks", () => { // `@theokit/tauri` depends on `theokit@0.57.0`, which is substituted — and that tarball then // requests `@theokit/agents@^12.1.0`, still absent. Substituting only the direct reference