diff --git a/package.json b/package.json index 25ec657e76..d800cc60a3 100644 --- a/package.json +++ b/package.json @@ -83,6 +83,7 @@ "@changesets/parse": "^0.4.3", "@commitlint/cli": "^20.1.0", "@commitlint/config-conventional": "^20.0.0", + "@manypkg/get-packages": "^1.1.3", "@testing-library/jest-dom": "^6.9.1", "@testing-library/react": "^16.3.0", "@testing-library/user-event": "^14.6.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4c85d34175..5fd20bbc9b 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -84,6 +84,9 @@ importers: '@commitlint/config-conventional': specifier: ^20.0.0 version: 20.0.0 + '@manypkg/get-packages': + specifier: ^1.1.3 + version: 1.1.3 '@testing-library/jest-dom': specifier: ^6.9.1 version: 6.9.1 @@ -1933,10 +1936,6 @@ packages: peerDependencies: '@babel/core': ^7.29.6 - '@babel/runtime@7.28.4': - resolution: {integrity: sha512-Q/N6JNWvIvPnLDvjlE1OUBLPQHH6l3CltCEsHIujp45zQUSSh8K+gHnaEX45yAT1nyngnINhvWtzN+Nb9D8RAQ==} - engines: {node: '>=6.9.0'} - '@babel/runtime@7.29.7': resolution: {integrity: sha512-Nq8OhGWiZIZGV6hLHoyAKLLcJihP/xFeBMGJoUrxTX2psI8dCifzLhZISFb+VWS3wFMRDmCGw5R+dOySCqPLhw==} engines: {node: '>=6.9.0'} @@ -9124,8 +9123,6 @@ snapshots: '@babel/core': 7.29.7 '@babel/helper-plugin-utils': 7.27.1 - '@babel/runtime@7.28.4': {} - '@babel/runtime@7.29.7': {} '@babel/template@7.29.7': @@ -10581,14 +10578,14 @@ snapshots: '@manypkg/find-root@1.1.0': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@types/node': 12.20.55 find-up: 4.1.0 fs-extra: 8.1.0 '@manypkg/get-packages@1.1.3': dependencies: - '@babel/runtime': 7.28.4 + '@babel/runtime': 7.29.7 '@changesets/types': 4.1.0 '@manypkg/find-root': 1.1.0 fs-extra: 8.1.0 diff --git a/scripts/release/check-changesets.mjs b/scripts/release/check-changesets.mjs index b21fd6d510..0c18b79304 100644 --- a/scripts/release/check-changesets.mjs +++ b/scripts/release/check-changesets.mjs @@ -31,7 +31,7 @@ import { fileURLToPath } from "node:url"; import parseChangeset from "@changesets/parse"; import micromatch from "micromatch"; -import { getWorkspacePackageNames } from "./lib.mjs"; +import { getWorkspacePackages } from "./lib.mjs"; const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); @@ -58,11 +58,13 @@ const ALPHA_BUMP = "patch"; * `[["a", "b"]]` and is refused by the release tooling, and an entry that is not * a non-empty string names nothing. * - * Checked here rather than by handing the file to `@changesets/config`, whose - * `parse` needs a resolved workspace from `@manypkg/get-packages` — a second - * dependency, for a rule that is three sentences long. That is the opposite trade - * to the YAML reader, and for the opposite reason: this rule cannot drift, and a - * frontmatter grammar can. + * Checked here rather than by handing the file to `@changesets/config`, and NOT + * because that would cost a dependency — the workspace its `parse` needs is + * resolved a few lines above. Because it would answer a different question. + * Changesets accepts any number of disjoint groups and objects only to a package + * appearing in two of them; "exactly one group" is this repository's rule, which + * upstream will never enforce on its behalf. Delegating the check would silently + * drop the part that matters most here. */ export function fixedGroupShape(config) { const groups = config.fixed; @@ -140,13 +142,21 @@ export function lockstepPackages(configText, workspaceNames) { * config packages. Measuring against "what we publish" would report those four as * errors on every run. * - * Checked in BOTH directions. A name in the group that no package answers to is - * a rename or a deletion that the config still believes in, and Changesets fails - * a release on an unknown package in `fixed`. + * Checked in BOTH directions, and the second direction has two causes that are + * NOT the same fault. A name no workspace package answers to is a rename or a + * deletion the config still believes in, and Changesets refuses the release + * outright. A name that DOES resolve but sits outside `packages/` is the + * opposite: the release proceeds, quietly widened. It matters because the group + * moves every member to the highest version any of them carries, so one package + * versioned on its own line — an app, a harness — decides the number the whole + * train takes. Reporting both under one sentence would tell an author the release + * will fail when it is instead going to succeed at the wrong version. */ -export function groupMatchesWorkspace(packages, workspaceNames) { +export function groupMatchesWorkspace(packages, workspace) { const problems = []; - const absent = workspaceNames.filter(name => !packages.includes(name)); + const absent = workspace.underPackagesDir.filter( + name => !packages.includes(name) + ); if (absent.length > 0) { problems.push( `.changeset/config.json: the \`fixed\` group is missing ${absent.join(", ")}. ` + @@ -154,11 +164,23 @@ export function groupMatchesWorkspace(packages, workspaceNames) { `stranded at an older version by the next release.` ); } - const unknown = packages.filter(name => !workspaceNames.includes(name)); + const outside = packages.filter( + name => + !workspace.underPackagesDir.includes(name) && workspace.all.includes(name) + ); + if (outside.length > 0) { + problems.push( + `.changeset/config.json: the \`fixed\` group reaches ${outside.join(", ")}, ` + + `which is in the workspace but not under packages/. The group takes the ` + + `highest version any member carries, so a package versioned on its own ` + + `line decides the version of the entire release train.` + ); + } + const unknown = packages.filter(name => !workspace.all.includes(name)); if (unknown.length > 0) { problems.push( `.changeset/config.json: the \`fixed\` group names ${unknown.join(", ")}, ` + - `which no package under packages/ answers to. Changesets refuses a release ` + + `which no package in the workspace answers to. Changesets refuses a release ` + `on an unknown package in \`fixed\`.` ); } @@ -195,7 +217,8 @@ export function declaredReleases(fileText) { return undefined; } const releases = new Map(); - for (const release of parsed.releases) releases.set(release.name, release.type); + for (const release of parsed.releases) + releases.set(release.name, release.type); return releases; } @@ -259,8 +282,11 @@ export function problemsWith(path, fileText, packages) { * because the PR that adds a package is often exactly that one — and a stale * group makes every later changeset wrong while each of them passes. */ -export function checkChangesets(paths, readFile, configText, workspaceNames) { - const packages = lockstepPackages(configText, workspaceNames); +export function checkChangesets(paths, readFile, configText, workspace) { + // Expanded against the WHOLE workspace, which is what Changesets expands + // against. Narrowing it here to the packages the group is supposed to contain + // would make a pattern reaching outside `packages/` look like exact coverage. + const packages = lockstepPackages(configText, workspace.all); if (packages.length === 0) { // A config with no fixed group would make every check below vacuous, and a // guard that passes because it found nothing to check is worse than none. @@ -275,7 +301,7 @@ export function checkChangesets(paths, readFile, configText, workspaceNames) { // reading the release tooling does not share. if (shape.length > 0) return shape; return [ - ...groupMatchesWorkspace(packages, workspaceNames), + ...groupMatchesWorkspace(packages, workspace), ...paths.flatMap(path => problemsWith(path, readFile(path), packages)), ]; } @@ -349,7 +375,7 @@ async function main(argv) { paths, path => readFileSync(resolve(REPO_ROOT, path), "utf8"), readFileSync(resolve(REPO_ROOT, ".changeset", "config.json"), "utf8"), - getWorkspacePackageNames() + getWorkspacePackages() ); if (problems.length === 0) { console.log( diff --git a/scripts/release/check-changesets.test.mjs b/scripts/release/check-changesets.test.mjs index 58c135e6f2..6595eb1744 100644 --- a/scripts/release/check-changesets.test.mjs +++ b/scripts/release/check-changesets.test.mjs @@ -14,8 +14,21 @@ import { const CONFIG = JSON.stringify({ fixed: [["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]], }); -const WORKSPACE = ["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]; -const PACKAGES = lockstepPackages(CONFIG, WORKSPACE); +const MEMBERS = ["nextly", "@nextlyhq/ui", "@nextlyhq/builder"]; + +/** A workspace whose every package is one the group is supposed to contain. */ +const WORKSPACE = { all: MEMBERS, underPackagesDir: MEMBERS }; + +/** + * The same workspace plus an app, which is the shape the real repository has: + * in the workspace, versioned on its own line, deliberately NOT on the train. + */ +const WITH_AN_APP = { + all: [...MEMBERS, "playground"], + underPackagesDir: MEMBERS, +}; + +const PACKAGES = lockstepPackages(CONFIG, WORKSPACE.all); /** A changeset naming the given `package: bump` pairs. */ function changeset(pairs, body = "Something changed.") { @@ -34,7 +47,7 @@ describe("reading the group", () => { // A guard that passes because it found nothing to check is worse than no // guard: it reports success on every changeset for as long as the config is // broken. - const problems = checkChangesets(["a.md"], () => COMPLETE, "{}", PACKAGES); + const problems = checkChangesets(["a.md"], () => COMPLETE, "{}", WORKSPACE); expect(problems).toHaveLength(1); expect(problems[0]).toContain("no `fixed` group"); }); @@ -127,17 +140,24 @@ describe("what else a frontmatter can get wrong", () => { describe("the group against the workspace", () => { it("accepts a group that matches", () => { // The control, and the state the repository is in today. - expect(groupMatchesWorkspace(PACKAGES, [...PACKAGES])).toEqual([]); + expect(groupMatchesWorkspace(PACKAGES, WORKSPACE)).toEqual([]); + }); + + it("accepts a group that leaves an app out of the train", () => { + // The other control, and the one the real config relies on: a workspace + // package outside packages/ is not a gap in the group. A check measuring the + // group against the whole workspace would report `playground` on every run. + expect(groupMatchesWorkspace(PACKAGES, WITH_AN_APP)).toEqual([]); }); it("catches a package the config was never told about", () => { // The drift the config cannot see about itself: a PR adds a package under // packages/ and every changeset in it names the old members, so all of them // pass while the new package is stranded on the next train. - const problems = groupMatchesWorkspace(PACKAGES, [ - ...PACKAGES, - "@nextlyhq/newcomer", - ]); + const problems = groupMatchesWorkspace(PACKAGES, { + all: [...MEMBERS, "@nextlyhq/newcomer"], + underPackagesDir: [...MEMBERS, "@nextlyhq/newcomer"], + }); expect(problems).toHaveLength(1); expect(problems[0]).toContain("@nextlyhq/newcomer"); @@ -149,21 +169,43 @@ describe("the group against the workspace", () => { // release on an unknown package in `fixed`. const problems = groupMatchesWorkspace( [...PACKAGES, "@nextlyhq/departed"], - [...PACKAGES] + WORKSPACE ); expect(problems).toHaveLength(1); expect(problems[0]).toContain("@nextlyhq/departed"); + expect(problems[0]).toContain("refuses a release"); + }); + + it("separates a group reaching outside packages/ from a name that resolves to nothing", () => { + // Two different outcomes, so they cannot share a sentence. A name nothing + // answers to fails the release; a name answering to an app succeeds it, at + // whatever version that app happens to carry. + const problems = groupMatchesWorkspace( + [...PACKAGES, "playground", "@nextlyhq/departed"], + WITH_AN_APP + ); + + expect(problems).toHaveLength(2); + const reaches = problems.find(problem => problem.includes("playground")); + expect(reaches).toContain("not under packages/"); + expect(reaches).not.toContain("@nextlyhq/departed"); + + const unknown = problems.find(problem => + problem.includes("@nextlyhq/departed") + ); + expect(unknown).toContain("refuses a release"); + expect(unknown).not.toContain("playground"); }); it("is asked even when the pull request touches no changeset", () => { // The PR that adds a package is often exactly the one with no changeset of // its own, so an early return on an empty list would skip the check that // matters most. - const problems = checkChangesets([], () => "", CONFIG, [ - ...PACKAGES, - "@nextlyhq/newcomer", - ]); + const problems = checkChangesets([], () => "", CONFIG, { + all: [...MEMBERS, "@nextlyhq/newcomer"], + underPackagesDir: [...MEMBERS, "@nextlyhq/newcomer"], + }); expect(problems).toHaveLength(1); expect(problems[0]).toContain("@nextlyhq/newcomer"); @@ -295,7 +337,7 @@ describe("the shape of the fixed group itself", () => { [], () => "", JSON.stringify({ fixed: ["nextly", "@nextlyhq/ui"] }), - ["nextly", "@nextlyhq/ui", "@nextlyhq/builder"] + WORKSPACE ); expect(problems).toHaveLength(1); expect(problems[0]).toContain("array of ARRAYS"); @@ -309,8 +351,8 @@ describe("a group written with globs", () => { // report every matching package as missing and the pattern as unknown, which // on a config written this way rejects every pull request. const globbed = JSON.stringify({ fixed: [["nextly", "@nextlyhq/*"]] }); - expect(lockstepPackages(globbed, WORKSPACE).sort()).toEqual( - [...WORKSPACE].sort() + expect(lockstepPackages(globbed, WORKSPACE.all).sort()).toEqual( + [...MEMBERS].sort() ); }); @@ -319,7 +361,7 @@ describe("a group written with globs", () => { // `@changesets/config` does it. Evaluating each pattern alone and unioning // the results would let `**` put back what `!…` excluded. const negated = JSON.stringify({ fixed: [["**", "!@nextlyhq/builder"]] }); - expect(lockstepPackages(negated, WORKSPACE).sort()).toEqual([ + expect(lockstepPackages(negated, WORKSPACE.all).sort()).toEqual([ "@nextlyhq/ui", "nextly", ]); @@ -327,14 +369,36 @@ describe("a group written with globs", () => { it("reports a pattern that matches nothing by the name it was written as", () => { const globbed = JSON.stringify({ fixed: [["@nowhere/*"]] }); - expect(lockstepPackages(globbed, WORKSPACE)).toEqual(["@nowhere/*"]); + expect(lockstepPackages(globbed, WORKSPACE.all)).toEqual(["@nowhere/*"]); }); it("does not report the workspace as missing when a glob covers it", () => { const globbed = JSON.stringify({ fixed: [["nextly", "@nextlyhq/*"]] }); - expect( - checkChangesets([], () => "", globbed, WORKSPACE) - ).toEqual([]); + expect(checkChangesets([], () => "", globbed, WORKSPACE)).toEqual([]); + }); + + it("expands a broad pattern over the packages a release resolves, not one directory", () => { + // The gap this closes. `**` reaches every package Changesets enumerates, + // which includes the ones outside packages/. Expanding it against the + // packages/ listing alone answers with exact coverage, so a changeset naming + // only those packages passes here and the release widens underneath it. + const globbed = JSON.stringify({ fixed: [["**"]] }); + + expect(lockstepPackages(globbed, WITH_AN_APP.all).sort()).toEqual( + [...WITH_AN_APP.all].sort() + ); + + const problems = checkChangesets([], () => "", globbed, WITH_AN_APP); + expect(problems).toHaveLength(1); + expect(problems[0]).toContain("playground"); + expect(problems[0]).toContain("not under packages/"); + }); + + it("still accepts a broad pattern in a workspace that is only packages/", () => { + // The other direction, so the case above cannot pass by rejecting `**` on + // sight: with nothing outside packages/ to reach, the same group is correct. + const globbed = JSON.stringify({ fixed: [["**"]] }); + expect(checkChangesets([], () => "", globbed, WORKSPACE)).toEqual([]); }); }); @@ -403,7 +467,7 @@ describe("what the check is pointed at", () => { ["good.md", "bad.md"], path => files[path], CONFIG, - PACKAGES + WORKSPACE ); expect(problems).toHaveLength(1); @@ -413,6 +477,6 @@ describe("what the check is pointed at", () => { it("passes when it is given nothing", () => { // The ordinary PR touches no changeset — a test-only or docs-only one gets // none at all — and that must not be a failure. - expect(checkChangesets([], () => "", CONFIG, PACKAGES)).toEqual([]); + expect(checkChangesets([], () => "", CONFIG, WORKSPACE)).toEqual([]); }); }); diff --git a/scripts/release/lib.mjs b/scripts/release/lib.mjs index 30607c749a..4fa6fec6cb 100644 --- a/scripts/release/lib.mjs +++ b/scripts/release/lib.mjs @@ -9,6 +9,8 @@ import { existsSync, readdirSync, readFileSync } from "node:fs"; import { dirname, join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import { getPackagesSync } from "@manypkg/get-packages"; + const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), "..", ".."); const PACKAGES_DIR = join(REPO_ROOT, "packages"); const PRE_STATE_PATH = join(REPO_ROOT, ".changeset", "pre.json"); @@ -78,6 +80,53 @@ export function readPreState() { return state.mode === "pre" ? state : null; } +/** + * The workspace as the release tooling sees it, in two lists that are NOT the + * same and must not be swapped: + * + * - `all` — every package Changesets resolves, private ones included. This is + * what a `fixed` entry is expanded against, so a pattern here has to match + * against exactly this list or the group this repository checks is not the + * group the release builds. + * - `underPackagesDir` — those sitting directly in `packages/`. This is what the + * group is supposed to CONTAIN, which is a different question: `apps/*` is in + * the workspace and deliberately outside the release train. + * + * Both come from one walk rather than two, because they only mean anything + * relative to each other: enumerated separately they could disagree about which + * packages exist, and every comparison below would then be reading two different + * workspaces. + * + * Resolved by `@manypkg/get-packages` rather than by reading a directory or a + * workspace file, because that is the library Changesets itself enumerates + * through — so what expands here and what expands at release time agree by + * construction. The distinction is not academic in this repository: the root + * manifest carries a `workspaces` field, which that library prefers over + * `pnpm-workspace.yaml`, so the two files describe different sets and only one + * of them is the one Changesets acts on. + * + * A package whose manifest has no `name` throws rather than being skipped: it is + * a member of the workspace that no list here can report on, and dropping it + * would make every check below quietly answer a smaller question. + */ +export function getWorkspacePackages() { + const { packages } = getPackagesSync(REPO_ROOT); + const all = []; + const underPackagesDir = []; + + for (const entry of packages) { + const name = entry.packageJson.name; + all.push(name); + if (dirname(entry.dir) === PACKAGES_DIR) underPackagesDir.push(name); + } + + const byName = (a, b) => a.localeCompare(b); + return { + all: all.sort(byName), + underPackagesDir: underPackagesDir.sort(byName), + }; +} + /** * Every workspace package that `changeset publish` is expected to push to npm: * the contents of `packages/*` minus anything marked private. Private packages @@ -88,28 +137,6 @@ export function readPreState() { * A manifest that exists but cannot be parsed is an error rather than a silent * skip: dropping it here would also drop it from every check below. */ -/** - * Every package name under `packages/`, private ones included. - * - * Deliberately wider than {@link getReleaseManifest}, which answers "what do we - * publish". Changesets versions private workspace packages too unless told - * otherwise, so the set it has to be told about is every package in the - * directory rather than only the publishable ones — and a checker comparing the - * `fixed` group against the narrower list would report the config-only packages - * as errors on every run. - */ -export function getWorkspacePackageNames() { - const names = []; - for (const entry of readdirSync(PACKAGES_DIR, { withFileTypes: true })) { - if (!entry.isDirectory()) continue; - const manifestPath = join(PACKAGES_DIR, entry.name, "package.json"); - if (!existsSync(manifestPath)) continue; - const pkg = readJson(manifestPath); - if (typeof pkg.name === "string") names.push(pkg.name); - } - return names.sort((a, b) => a.localeCompare(b)); -} - export function getReleaseManifest() { const manifest = [];