fix(create-pds): Autocopy allowBuilds when using PNPM - #202
fix(create-pds): Autocopy allowBuilds when using PNPM#202helloimalastair wants to merge 1 commit into
allowBuilds when using PNPM#202Conversation
There was a problem hiding this comment.
Thanks — the "Ignored build scripts" warning on a fresh scaffold is annoying and I'd like to get rid of it. One real problem: a pnpm-workspace.yaml with no packages: key makes pnpm 9 abort the install (reproduced on 9.15.9, which is what corepack users get since the template has no packageManager). Also worth being accurate that this silences a warning rather than fixing a breakage — workerd and esbuild resolve their binaries via optionalDependencies. Details inline; needs a changeset too.
ascorbic
left a comment
ascorbic
left a comment
There was a problem hiding this comment.
Inline notes for the review above.
| @@ -0,0 +1,4 @@ | |||
| allowBuilds: | |||
There was a problem hiding this comment.
pnpm 9 treats any pnpm-workspace.yaml as a workspace manifest and, with no packages: key, aborts with ERROR packages field missing or empty — reproduced on 9.15.9. The template has no packageManager field so that's what corepack resolves to. pnpm 9 doesn't block build scripts at all, so for those users this file is pure downside: install fails and create-pds reports "Failed to install dependencies".
Two ways out: gate the copy on the pnpm major (it's in npm_config_user_agent), or spell it as onlyBuiltDependencies: [esbuild, workerd], which every pnpm 10 understands — allowBuilds only arrived partway through 10.x so early 10 users silently get nothing from this either way. I'd avoid adding packageManager to the template since that breaks npm/yarn/bun under corepack.
| @@ -0,0 +1,4 @@ | |||
| allowBuilds: | |||
| esbuild: true | |||
| sharp: false | |||
There was a problem hiding this comment.
sharp isn't in a scaffolded project's tree (it comes from docs/ in this monorepo), so this entry is noise.
| let destName = entry.name; | ||
|
|
||
| // Skip pnpm-specific files if not using pnpm | ||
| if (destName === "pnpm-workspace.yaml" && pm !== "pnpm") continue; |
There was a problem hiding this comment.
This branch isn't covered — test/e2e.test.ts asserts a fixed file list. Would be good to run both (--package-manager pnpm gets the file, npm doesn't) since the CLI already supports the flag.
2 participants
Hit this small papercut myself.
Per Block risky postinstall scripts,
pnpmnow doesn't runpostinstallscripts automatically. This change adds the config required to ensureworkerdandesbuildhave their postinstall scripts run, and only performs this work when applicable(i.e., when usingpnpm).