Repository navigation
chore(deps): move to pnpm, and rename the release script out of the line of fire - #306
Merged
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
aadhar-sh | 003e20a | Aug 10 2026, 04:33 PM |
oddharsh
enabled auto-merge (squash)
August 10, 2026 16:05
oddharsh
force-pushed
the
chore/pnpm
branch
2 times, most recently
from
August 10, 2026 16:10
f481e9c to
ef82f4b
Compare
…ine of fire npm, pnpm, bun and all three yarn 4 linkers were measured against this tree. Every one built identically and passed the full suite, so the choice came down to cost: a second repo's dependencies cost pnpm 4 MB of real disk against npm's 459 MB (APFS clones the store, so `du` reports 340 MB and is wrong), and warm installs run 1.50s against 3.71s. centaur already runs pnpm, so this leaves one package manager rather than two. Yarn was ruled out on a hard failure rather than a preference: the 4.9.1 that Workers Builds ships cannot install this repo at all, because its builtin TypeScript compat patch expects `lib/_tsc.js` and TypeScript 7 has no such file. Controls: TypeScript 5.9.3 installs fine on 4.9.1, and 7.0.2 installs fine on 4.18.0. Bun ties pnpm as an installer and is ruled out as a RUNTIME, which is the only thing that would have made it interesting here (gotcha 29). The rename is the sharp edge. `pnpm <script>` runs the script, so `pnpm deploy` does not hit pnpm's builtin the way `pnpm deploy --help` in an empty directory implies: it starts `wrangler deploy` against production at 100%. Under npm that path required the word `run`. The root script is `deploy:direct` now so the release path has no two-word shorthand. lens-reader keeps a plain `deploy`, since it publishes a demo Worker rather than aadhar.sh. Three things measured rather than assumed: - `npx wrangler` resolves pnpm's node_modules/.bin, so the Workers Builds deploy commands in infra.json are untouched and NO dashboard edit is needed. - pnpm forwards script args natively and passes a literal `--` THROUGH to the script, where npm strips it. `pnpm run photos -- "$dir"` would have handed add-photos.sh a `--` as its first path, so all 41 stray `--` are gone. - Every native dependency ships its platform binary via optionalDependencies and passes its probe with postinstalls blocked, which is what makes the pnpm 11 vs 10 `allowBuilds`/`onlyBuiltDependencies` split survivable today. check-wrangler.mjs now reads the installed tree instead of parsing the npm lockfile, which is stronger: a lockfile records what should be there and node_modules records what is. Its two lockfile-echo assertions are dropped because `pnpm install --frozen-lockfile` already fails when the lockfile and package.json disagree, and CI runs that first. Verified it still has teeth against all three failure modes it was written for. minimumReleaseAge is pinned at 1440 (pnpm 11's default, written down so it stays a decision). It refuses packages published in the last 24h, including from a committed lockfile, so expect dependabot PRs to fail install until the package ages out or lands in minimumReleaseAgeExclude. Gates on a clean tree: build, test 206/206, cal 59/59, typecheck, routes:check, pages:check, perf-budget, photos:check, check-wrangler.
The blanket npm->pnpm sweep rewrote the bootstrap line itself into `pnpm install -g pnpm`, which cannot run before pnpm exists.
`npm install --no-audit --no-fund` became `pnpm install --no-audit --no-fund`, and pnpm rejects both flags outright: [ERROR] Unknown options: 'audit', 'fund'. Also documents the minimumReleaseAge case that reddened perf-diff: it checks out the MERGE BASE and builds it with this branch's workflow, so a commit predating minimumReleaseAgeExclude gets the policy without the list. Self-resolving once this lands on main, and perf-diff gates nothing.
Three fixes and one adoption, all found by CI rather than by reading. `pnpm install --no-audit --no-fund`: pnpm rejects both flags outright, which is what reddened validate. The blanket npm->pnpm sweep had rewritten an `npm install` that carried them. lens-reader needs --ignore-workspace. It deliberately stays out of the workspace (defuddle + linkedom are ~22 MB only that Worker bundles) and under pnpm a bare `pnpm install` inside it walks UP to the root pnpm-workspace.yaml, installs the five workspace projects, and never creates lens-reader/node_modules. Its tests then die on ERR_MODULE_NOT_FOUND with a `node_modules missing` warning as the only clue. Its package-lock.json is replaced by its own pnpm-lock.yaml. npx -> pnpm exec, 33 call sites. The reason is the registry rather than tidiness: npx FETCHES what it cannot resolve locally, reaching registry.npmjs.org even under --no-install, while pnpm exec fails locally with no network call. A repo that pins Wrangler exactly and runs check-wrangler to enforce one version everywhere should not keep a command that will silently download some other one. THREE npx strings deliberately survive: both Workers Builds commands in infra.json and the mirrored copies in wrangler.jsonc and check-infra.mjs. Those RECORD what is typed into the Cloudflare dashboard, and infra:check compares them against the live values, so editing them here alone fails on invented drift. Changing them for real means the dashboard first, the strings second. Gates: build, test, cal 59/59, lens-reader 5/5, typecheck, routes:check, pages:check, perf-budget, photos:check, check-wrangler, and infra:check green against the live dashboard.
check-bun.mjs landed on main while this branch was in flight. Its usage strings say `npm run bun:check -- --bun <path>`, and under pnpm the `--` is passed THROUGH to the script rather than stripped, so the documented invocation would hand check-bun.mjs a literal `--` as its first argument.
oddharsh
added a commit
that referenced
this pull request
Aug 10, 2026
#306 pinned `packageManager` and converted this file's comments, but the one actual npm invocation survived it: the newest-stable-bun lookup at the end of the verdict. It was the only thing in the repo still asking for the other client. The registry is unchanged, so the printed label still reads "on npm". Also drops stderr on that call. It is wrapped in a try/catch because being offline is fine, but a warning printed before the catch would land in the middle of a verdict, and a control script's output is the whole product. Verified end to end against bun 1.4.0-canary.1: all three questions green, 1975 files byte-identical, and the last line resolves 1.3.14 through pnpm. Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
Moves the repo from npm to pnpm, renames the root release script
deploy→deploy:direct, and replacesnpxwithpnpm exec.Why pnpm
npm, pnpm, bun and all three yarn 4 linkers were measured against this tree. All six produced an identical build and passed the full suite, so the decision came down to cost.
node-modulespnpmlinkerThe disk column is the one
dulies about: it reports 340 MB for pnpm, but APFS clones the store, so a second repo's dependencies genuinely cost 4 MB. Measured by free space.centauralready runs pnpm, so this leaves one package manager rather than two.Yarn failed rather than lost. The 4.9.1 that Workers Builds ships cannot install this repo: its builtin TypeScript compat patch expects
lib/_tsc.js, and TypeScript 7 has no such file. Controls both ways — TS 5.9.3 installs on 4.9.1, TS 7.0.2 installs on 4.18.0. Its PnP linker is the fastest thing measured, and its disk win evaporates here because this dependency set is native-heavy (337 MB of.yarn/unplugged).Bun is covered by gotcha 28 from #307, which landed mid-flight and supersedes what this branch originally said. My independent measurement agreed with it: bun 1.3.14 (current stable) silently ignores
zstdCompressSync'sdictionaryandbuild.mjs's existing probe kills the build. #307 established that 1.4 canary fixes it.Two behaviour changes worth reading
pnpm <script>runs the script.pnpm deploydoes not hit pnpm's builtin the waypnpm deploy --helpin an empty directory implies — it startswrangler deployagainst production at 100%. Under npm that path required the wordrun. The root script isdeploy:directnow.lens-readerkeeps a plaindeploy, since it publishes a demo Worker.minimumReleaseAge: 1440refuses packages published in the last 24h, including from a committed lockfile. Expect dependabot PRs to fail install until the package ages out or lands inminimumReleaseAgeExclude.npx → pnpm exec
The reason is the registry rather than tidiness:
npxFETCHES what it cannot resolve locally, reachingregistry.npmjs.orgeven under--no-install, whilepnpm execfails locally with no network call. A repo that pins Wrangler exactly and runscheck-wranglerto enforce one version should not keep a command that will silently download another. 30 call sites moved.pnpm dlxis npx's true twin and is the wrong tool here: every call site wasnpx wrangler, wrangler is a pinned devDependency, anddlxwould fetch from the registry and ignore the pin.Three
npxstrings deliberately survive — both Workers Builds commands ininfra.jsonand the mirrored copies inwrangler.jsoncandcheck-infra.mjs. Those record what is typed into the Cloudflare dashboard, andinfra:checkcompares them against the live values, so editing them here alone fails on invented drift. No dashboard edit is required, verified:npx wranglerresolves pnpm'snode_modules/.binand dry-runs clean.Other fixes CI found
pnpm install --no-audit --no-fund— pnpm rejects both flags.lens-readerneeds--ignore-workspace. A barepnpm installinside it walks UP to the root workspace and never createslens-reader/node_modules, so its tests die onERR_MODULE_NOT_FOUND. npm installed the localpackage.jsoninstead, so this is a behaviour change.--through to scripts where npm strips it.pnpm run photos -- "$dir"would have handedadd-photos.sha--as its first path. All 41 stray--removed, proven with a script that echoes argv.check-wrangler.mjsnow reads the installed tree instead of parsing the npm lockfile. Two assertions dropped because they only checked the lockfile echoedpackage.json, which--frozen-lockfilealready enforces. Verified it still catches all three failure modes.Verification
validategreen. Locally on a clean tree: build, test, cal 59/59, lens-reader 5/5, typecheck, routes:check, pages:check, perf-budget, photos:check, check-wrangler, andinfra:checkagainst the live dashboard.wire size vs merge baseis red and expected to be. It checks out the merge base and builds it with this branch's workflow, so a base predatingminimumReleaseAgeExcludegets the policy without the list (ERR_PNPM_MINIMUM_RELEASE_AGE_VIOLATION). It self-resolves once this is on main, and it gates nothing.Documented in CLAUDE.md gotcha 29.
🤖 Generated with Claude Code