fix(create-cloudflare): generate valid pnpm-workspace.yaml for pnpm 9/10#13967
fix(create-cloudflare): generate valid pnpm-workspace.yaml for pnpm 9/10#13967matingathani wants to merge 4 commits into
Conversation
pnpm 10+ blocks build scripts by default; without an explicit allow-list, `pnpm install` exits with ERR_PNPM_IGNORED_BUILDS. Before running install, write a pnpm-workspace.yaml that opts in the packages that need build scripts (esbuild, workerd, sharp): - pnpm ≥ 10: allowBuilds map with boolean values - pnpm 9.x: onlyBuiltDependencies list Does nothing if the file already exists or the PM is not pnpm. Fixes cloudflare#13928
🦋 Changeset detectedLatest commit: f0851cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
workers-devprod
requested review from
a team and
emily-shen
and removed request for
a team
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
There was a problem hiding this comment.
Pull request overview
This PR adds a writePnpmWorkspaceYaml helper that is invoked before pnpm install in create-cloudflare's npmInstall() flow, intending to fix #13928 (pnpm 10+ aborting scaffolds with ERR_PNPM_IGNORED_BUILDS). Depending on the detected pnpm major version, it writes either an onlyBuiltDependencies list (pnpm 9) or an allowBuilds map of booleans (pnpm 10+) for esbuild, workerd, and sharp. A new unit-test file covers each branch.
Changes:
- Add
writePnpmWorkspaceYaml(projectPath)that detects the package manager and pnpm version (viasemver) and writes apnpm-workspace.yamlif one doesn't exist. - Call the helper from
npmInstall()before runningpnpm install. - Add
packages.test.tswith branch coverage for non-pnpm, pnpm <9, pnpm 9.x, pnpm 10+, and the "file exists" short-circuit.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/create-cloudflare/src/helpers/packages.ts | Adds writePnpmWorkspaceYaml and wires it into npmInstall. |
| packages/create-cloudflare/src/helpers/tests/packages.test.ts | New unit tests for the helper's branches. |
Comments suppressed due to low confidence (2)
packages/create-cloudflare/src/helpers/packages.ts:42
- Using
allowBuildsas the pnpm 10+ setting key is very likely incorrect. pnpm did not renameonlyBuiltDependenciesin 10.x — that key is still the documented mechanism for approving install scripts in bothpnpm-workspace.yamlandpackage.json, and its value is still a list of package names (not a map of booleans). The pnpm docs URL referenced in the comment (https://pnpm.io/settings#allowbuilds) does not correspond to a real setting. IfallowBuildsis not a recognized key, pnpm 10 will silently ignore it andpnpm installwill still fail withERR_PNPM_IGNORED_BUILDS, meaning this PR does not actually fix #13928 on pnpm 10/11. Please verify against the pnpm 10 release notes / settings docs and, unless there is concrete evidence such a key exists, writeonlyBuiltDependenciesas a list for both the pnpm 9 and pnpm 10+ branches (the format is the same across versions).
if (semver.gte(version, "10.0.0")) {
// pnpm 10+: allowBuilds uses a map of package -> boolean
const entries = PNPM_BUILT_DEPENDENCIES.map((pkg) => ` ${pkg}: true`).join(
"\n"
);
content = [
"# Approve build scripts for packages that require them.",
"# See: https://pnpm.io/settings#allowbuilds",
"allowBuilds:",
entries,
"",
].join("\n");
packages/create-cloudflare/src/helpers/packages.ts:43
semver.gte(version, "10.0.0")will throwTypeError: Invalid Versionifwhich-pm-runsreturns a non-semver-coercibleversion(orundefined, which can happen when pnpm cannot be parsed from the user agent). Existing code in this package generally treatsversionas opaque and doesn't compare it. Consider usingsemver.coerce(version)orsemver.gte(version, "10.0.0", { loose: true })and guarding for a missing/invalid version so a malformed PM version string does not crashnpmInstallfor all users.
if (semver.gte(version, "10.0.0")) {
// pnpm 10+: allowBuilds uses a map of package -> boolean
const entries = PNPM_BUILT_DEPENDENCIES.map((pkg) => ` ${pkg}: true`).join(
"\n"
);
content = [
"# Approve build scripts for packages that require them.",
"# See: https://pnpm.io/settings#allowbuilds",
"allowBuilds:",
entries,
"",
].join("\n");
} else if (semver.gte(version, "9.0.0")) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const { npm } = detectPackageManager(); | ||
|
|
||
| writePnpmWorkspaceYaml(ctx.project.path); |
| vi.mock("node:fs"); | ||
| vi.mock("which-pm-runs"); | ||
|
|
||
| describe("writePnpmWorkspaceYaml", () => { | ||
| beforeEach(() => { | ||
| mockPackageManager("npm"); | ||
| vi.mocked(existsSync).mockReturnValue(false); | ||
| vi.mocked(writeFileSync).mockImplementation(() => undefined); |
| import { existsSync, writeFileSync } from "node:fs"; | ||
| import { writePnpmWorkspaceYaml } from "helpers/packages"; | ||
| import { beforeEach, describe, expect, test, vi } from "vitest"; | ||
| import whichPMRuns from "which-pm-runs"; |
| const workspaceYamlPath = nodePath.join(projectPath, "pnpm-workspace.yaml"); | ||
| if (existsSync(workspaceYamlPath)) { | ||
| return; | ||
| } |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| @@ -0,0 +1,72 @@ | |||
| import { existsSync, writeFileSync } from "node:fs"; | |||
| import { writePnpmWorkspaceYaml } from "helpers/packages"; | |||
| import { beforeEach, describe, expect, test, vi } from "vitest"; | |||
There was a problem hiding this comment.
🔴 Test imports expect from vitest instead of using test context destructuring
The AGENTS.md rule explicitly states: "expect must come from test context — never import { expect } from "vitest". Use destructured test context: it("name", ({ expect }) => { ... })". This test file imports expect from vitest at line 3 and all six test callbacks use bare () => instead of ({ expect }) =>. Every other test file in the same directory (e.g., packages/create-cloudflare/src/helpers/__tests__/args.test.ts, packageManagers.test.ts, json.test.ts) correctly uses the ({ expect }) pattern.
Prompt for agents
The test file at packages/create-cloudflare/src/helpers/__tests__/packages.test.ts violates the mandatory AGENTS.md rule that expect must come from the test context, not from a vitest import.
What needs to change:
1. Remove `expect` from the vitest import on line 3: change to `import { beforeEach, describe, test, vi } from "vitest";`
2. Update every `test("...", () => {` callback to `test("...", ({ expect }) => {` — this affects lines 17, 23, 29, 36, 49, and 64.
See the existing test files in the same directory (args.test.ts, packageManagers.test.ts, json.test.ts) for the correct pattern.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed — removed expect from the vitest import and switched all test callbacks to use ({ expect }) destructuring pattern.
|
|
||
| const { npm } = detectPackageManager(); | ||
|
|
||
| writePnpmWorkspaceYaml(ctx.project.path); |
There was a problem hiding this comment.
🔴 rectifyPmMismatch runs pnpm install without creating pnpm-workspace.yaml
The PR adds writePnpmWorkspaceYaml(ctx.project.path) to npmInstall (packages/create-cloudflare/src/helpers/packages.ts:103) but not to rectifyPmMismatch (packages/create-cloudflare/src/helpers/packageManagers.ts:130). Both functions run pnpm install. When a framework template scaffolds and installs deps with npm (creating node_modules), npmInstall at packages/create-cloudflare/src/cli.ts:155 returns early without creating the workspace yaml, then rectifyPmMismatch at packages/create-cloudflare/src/cli.ts:156 detects the lockfile mismatch, deletes node_modules, and runs pnpm install without a pnpm-workspace.yaml. On pnpm 10+ this causes the exact ERR_PNPM_IGNORED_BUILDS error this PR aims to fix, as build scripts for esbuild/workerd/sharp are silently skipped.
Prompt for agents
The writePnpmWorkspaceYaml function is called in npmInstall (packages/create-cloudflare/src/helpers/packages.ts:103) but not in rectifyPmMismatch (packages/create-cloudflare/src/helpers/packageManagers.ts:99-136). Both code paths run pnpm install and are called sequentially from cli.ts:155-156.
When a framework template creates node_modules during scaffolding, npmInstall returns early (line 97-98) without creating pnpm-workspace.yaml. Then rectifyPmMismatch detects the npm-vs-pnpm lockfile mismatch, deletes node_modules, and runs pnpm install without the workspace yaml.
To fix: add a writePnpmWorkspaceYaml(ctx.project.path) call inside rectifyPmMismatch in packageManagers.ts, before the runCommand call at line 130. You'll need to import writePnpmWorkspaceYaml from helpers/packages. Alternatively, move the writePnpmWorkspaceYaml call earlier in the cli.ts flow (e.g. before npmInstall) so it's always executed regardless of which install path is taken.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in f0851cd — added writePnpmWorkspaceYaml(ctx.project.path) to rectifyPmMismatch before the pnpm install call when npm === "pnpm".
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
@cloudflare/wrangler-bundler
commit: |
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
I don't think this is the correct approach for that issue.
I am still to see a proper reproduction of the problem. The issue description talks about C3 generating a bad pnpm workspace file but as demonstrated by this PR we do not generate any such file. So that seems invalid.
The subsequent comments there also don't seem related to the original issue.
This PR is generating a file to "solve" the problem but hardcoding it like this would be fragile. If we were to try to resolve the original problem, I think it would be more likely to just run something like pnpm approve-builds as part of the setup. But I am yet to be convinced that is needed.
|
Thanks for the feedback, @petebacondarwin! I agree hardcoding package names is fragile. A few clarifying questions before revising:
Happy to revise once we align on direction. |
|
@matingathani - if you are able to create a reproduction that would help. I was not able to. |
|
Here is a minimal reproduction for @petebacondarwin. Prerequisite: pnpm 10 (which blocks build scripts by default) # Install pnpm 10 globally
npm install -g pnpm@10
pnpm --version # confirm: 10.x.x
# Simulate what create-cloudflare does: install a package that needs build scripts
mkdir /tmp/c3-repro && cd /tmp/c3-repro
cat > package.json <<'PKGJSON'
{
"name": "test-worker",
"private": true,
"dependencies": {
"esbuild": "^0.24.0"
}
}
PKGJSON
pnpm install
# Expected: ERR_PNPM_IGNORED_BUILDS — esbuild requires a build script
# which pnpm 10 blocks unless pnpm-workspace.yaml lists it under `allowBuilds`Why create-cloudflare triggers this: Fix in this PR: You can also verify the fix works end-to-end using the preview build from this PR: npm i https://pkg.pr.new/create-cloudflare@13967 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
Thanks for offering this reproduction, but it doesn't actually run C3. Can you actually reproduce the error by running C3? When I do so (with pnpm 10) it doesn't error. |
|
Thanks for testing. If running C3 with pnpm 10 doesn't trigger the error, the fix may not be needed — the issue could already be handled elsewhere. Happy to close this PR if you can't reproduce it. Let me know. |
matingathani
force-pushed
the
fix/create-cloudflare-pnpm-ignored-builds
branch
from
3629eb4 to
d366d44
Compare
Also fix unused import and test-context expect per AGENTS.md rules. Addresses Devin review comments.
|
Hi @matingathani — thanks for filing this first and for the persistence on the repro. You had the right shape of the fix two weeks before I did. For context: I dug into #13928 from another angle (#14193) and found that the reason this kept slipping through was a combination of two things — a pnpm 10→11 default flip ( #14193 supersedes this PR. It takes the same core approach (write
The PR description and issue comment both credit you. Once #14193 lands you can close this one. Apologies again for the delay, and thanks for sticking with it. |
|
Thank you Pete , i will have a look at it , but will take me 2-3 days to work on it because i just got my wisdom teeth removal surgery done today . |
Fixes #13928.
create-cloudflareranpnpm installwithout apnpm-workspace.yaml. pnpm 10+ blocks build scripts by default, so installs failed withERR_PNPM_IGNORED_BUILDSfor packages likeesbuild,workerd, andsharpthat require build scripts.Changes
writePnpmWorkspaceYaml(projectPath)is called beforepnpm installinnpmInstall()allowBuildswith proper YAML boolean values (not quoted strings)onlyBuiltDependencieslist