Add prepublishOnly guard to prevent publishing incomplete packages - #277
Merged
Conversation
`release:publish` prepares three things no checked-in manifest carries — concrete versions in place of `workspace:`/`catalog:`, the `development` condition stripped, a LICENSE copied into each package — and `bun run build` produces the `dist/` every entry point names. All four happen in the ephemeral publish job, so all four are skipped by an `npm publish` run anywhere else, and nothing caught that. `@amritk/mjst@0.7.15` and `@amritk/generate-parsers@0.12.3` are why this is worth a gate rather than a convention. Both shipped dead on arrival, and both are deprecated on npm rather than fixable, because a published version is forever. The sibling repo took the same class of miss further: `@amritk/mini-lynx-native@0.2.0` was hand-published with no `dist/` at all, and its surviving `development` condition pointed at the `src` the tarball also shipped, so anything honouring that condition resolved raw TypeScript and the package looked healthy. `scripts/check-publishable.mjs` runs from the package root as `prepublishOnly`. Two shapes in this workspace made the obvious version of it wrong, and both are covered: - `@amritk/mjst` is a CLI. Its only entry is `bin.mjst` and its exports map carries nothing but `./package.json`, so a check reading `exports` alone called it unreachable — and would have had nothing to say if the executable went missing. It reads `exports`, `bin`, `main`, `module` and `types`. - `@amritk/helpers` exports `./*` as `./dist/*.js`, so a target can be a pattern rather than a path. A pattern is satisfied by any one file of that extension, which is all the manifest itself promises. Verified both ways against the real pipeline: after `resolve-workspace-protocol`, `strip-development-exports` and `copy-license`, all twelve packages pass; with `packages/cli/dist` moved aside the CLI fails naming `./dist/cli.js`, and before the strip runs `@amritk/helpers` fails on the condition and the LICENSE. Nothing about the repo's own checks changes: `prepublishOnly` does not fire on `npm pack`, which is what dist-smoke and cli-e2e use.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
release:publishprepares three things no checked-in manifest carries — concrete versions in place ofworkspace:/catalog:, thedevelopmentcondition stripped, a LICENSE copied into each package — andbun run buildproduces thedist/every entry point names. All four happen in the ephemeral publish job, so all four are skipped by annpm publishrun anywhere else, and nothing caught that.@amritk/mjst@0.7.15and@amritk/generate-parsers@0.12.3are why this is worth a gate rather than a convention. Both shipped dead on arrival — tsc-alias corrupted a regex literal in the compiled JS, so the CLI threw aSyntaxErrorbefore doing any work — and both are deprecated on npm rather than fixable, because a published version is forever. The sibling repo took the same class of miss further:@amritk/mini-lynx-native@0.2.0was hand-published with nodist/at all, and its survivingdevelopmentcondition pointed at thesrcthe tarball also shipped, so anything honouring that condition resolved raw TypeScript and the package looked healthy.This adds the check at the one point that catches it regardless of who is publishing or from where.
Changes
scripts/check-publishable.mjs— fails a publish when the manifest points at adist/file that is not on disk, when adevelopmentcondition is still present, or when the package directory has no LICENSE. Plain node and node builtins only, so no environment can skip it.prepublishOnlyin all 12 publishable packages —prepublishOnlyrather thanprepackdeliberately: it does not fire onnpm pack, which is whatdist-smokeandcli-e2euse, so the repo's own checks are untouched.scripts/check-publishable.test.ts— 11 cases, including both workspace-specific shapes below and the vacuous-pass case (a manifest with no dist target anywhere, which is how a check like this goes quiet)..changeset/publish-guard.md— empty changeset; this touches no published surface.Two shapes in this workspace made the obvious version of this check wrong, and both are covered:
@amritk/mjstis a CLI. Its only entry isbin.mjstand its exports map carries nothing but./package.json, so a check readingexportsalone called it unreachable — and would have had nothing to say if the executable went missing. It readsexports,bin,main,moduleandtypes.@amritk/helpersexports./*as./dist/*.js. A target can be a pattern rather than a path, soexistsSyncon it always fails. A pattern is satisfied by any one file of that extension, which is all the manifest itself promises.Testing
Verified both directions against the real pipeline, not just the unit tests. After
resolve-workspace-protocol→strip-development-exports→copy-license, all twelve packages pass. Withpackages/cli/distmoved aside the CLI fails naming./dist/cli.js; before the strip runs,@amritk/helpersfails on both the surviving condition and the missing LICENSE.bun run testpasses (13 workspaces)bun run checkpasses (the 22 warnings are pre-existing — confirmed by stashing; these files add none)bun run buildpassesconfig.schema.jsonchanged (bun run generate-readmes) — n/a, unchangedbunx changeset) if this affects a published packageAlso run:
bun run versions:check,bun run types:check(12 workspaces), andbun run test:dist(96 tests across 9 files).Related issues
Companion to amritk/mini#34, which fixes the
mini-lynx-nativerelease described above and removes thedevelopmentcondition from that repo entirely. The condition is deliberately left in place here — see that PR for why the tradeoff differs.⚡ Benchmark delta vs main (
c3863a9→80cdcc7)mjst only, all suites (shared or bench-harness changes); each timed number is the better of two order-balanced (ABBA) runs, each the median of 21 isolated-process trials (±n% = coefficient of variation). ⚪ within ±5% · 🟢 improvement · 🔴 regression · ~ marks an unstable sample (CV > 10%) · ⚠parity marks a correctness disagreement. On shared CI runners, deltas within ±10% are usually noise — trust direction only when it persists across pushes.