Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🚀 Feature Request: Do we really need "configure pnpm to use the version specified in package.json engines field" #8093

Open
vicb opened this issue Feb 11, 2025 · 8 comments
Labels
enhancement New feature or request question A question about a feature rather than a bug quick win Potentially easy/straightforward issue to tackle regression Break in existing functionality as a result of a recent change workflows

Comments

@vicb
Copy link
Contributor

vicb commented Feb 11, 2025

Describe the solution

#7968 added manage-package-manager-versions = true

This breaks my dev workflow as the only way I was able to make this work is to corepack enable.

Without corepack enable it is not possible for me to use pnpm in this repo as it would complain that my version does not match the required version.

This is fixed by a corepack enable but on other repos, this cause issues:

unjs/unev throws with:

! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-10.2.1.tgz
? Do you want to continue? [Y/n] 

/Users/vberchet/.nvm/versions/node/v22.11.0/lib/node_modules/corepack/dist/lib/corepack.cjs:21535
  if (key == null || signature == null) throw new Error(`Cannot find matching keyid: ${JSON.stringify({ signatures, keys })}`);
                                              ^

Error: Cannot find matching keyid: {"signatures":[{"sig":"MEYCIQDkZyZZmBzkRcQowEEFiEcGp4/xV8GBLXxTEzz9QstrsAIhAPx6tvZixjTub6GPqJa82vcWFhUU39JCtoJvcoRK/K39","keyid":"SHA256:DhQ8wR5APBvFHLF/+Tc+AYvPOdTpcIDqOhxsBHRwC7U"}],"keys":[{"expires":null,"keyid":"SHA256:jl3bwswu80PjjokCgh0o2w5c2U4LhQAE57gj9cz1kzA","keytype":"ecdsa-sha2-nistp256","scheme":"ecdsa-sha2-nistp256","key":"MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE1Olb3zMAFFxXKHiIkQO5cJ3Yhl5i6UPp+IhuteBJbuHcA5UogKo0EWtlWwW6KSaKoTNEYL7JlCQiVnkhBktUgg=="}]}

On opennextjs, it would add an engine field in the package.json which I don't think is necessary.

Is this addition in #7968 solving an actual issue? @anonrig @petebacondarwin

If not maybe we can revert the PR (or document something that helps).

Thoughts?

@vicb vicb added the enhancement New feature or request label Feb 11, 2025
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Feb 11, 2025
@vicb vicb added quick win Potentially easy/straightforward issue to tackle question A question about a feature rather than a bug regression Break in existing functionality as a result of a recent change workflows labels Feb 11, 2025
@petebacondarwin
Copy link
Contributor

I just can't reproduce this problem. I don't ever remember running corepack enable and as I move from one repo to another it just makes sure that the correct pnpm is used. I even tried running corepack cache clean and corepack disable to see if that would trigger the issue, but no luck.

FWIW, pnpm is installed via homebrew (not npm) at version 9.15.4.

@vicb
Copy link
Contributor Author

vicb commented Feb 11, 2025

This is what I have:

Image

Image

@petebacondarwin
Copy link
Contributor

> which pnpm
/opt/homebrew/bin/pnpm

I don't use Volta, I use nvm...

@vicb
Copy link
Contributor Author

vicb commented Feb 11, 2025

Image

After deleting packageManager: ... in package.json:

Image

I deleted volta and it does not solve the issue:

Image

Back to the original question, does #7968 solve an actual issue?

@petebacondarwin
Copy link
Contributor

Back to the original question, does #7968 solve an actual issue?

For me it automatically runs the correct version of pnpm between workerd and workers-sdk.

@petebacondarwin
Copy link
Contributor

Interestingly in that directory (~/Library/pnpm) I have:

Image

which are the two versions needed by workerd and workers-sdk...

@anonrig
Copy link
Member

anonrig commented Feb 11, 2025

You need to install pnpm (without Volta) in order to make use of pnpm's auto install feature.

@jldec
Copy link

jldec commented Feb 14, 2025

+1 to reconsider the use of manage-package-manager-version
pnpm install no longer works on my local setup.
It may be because I have not enabled corepack or perhaps because I have pnpm v10.x or something else.
I installed pnpm using npm install -g pnpm.

~/cloudflare$ which node
/Users/jldec/n/bin/node

~/cloudflare$ node -v
v22.11.0

~/cloudflare$ which pnpm 
/Users/jldec/.local/bin/pnpm

~/cloudflare$ pnpm -v
10.4.0

~/cloudflare$ git clone https://github.com/cloudflare/workers-sdk
Cloning into 'workers-sdk'...
remote: Enumerating objects: 87607, done.
remote: Counting objects: 100% (6695/6695), done.
remote: Compressing objects: 100% (504/504), done.
remote: Total 87607 (delta 6389), reused 6208 (delta 6190), pack-reused 80912 (from 1)
Receiving objects: 100% (87607/87607), 73.90 MiB | 10.33 MiB/s, done.
Resolving deltas: 100% (61182/61182), done.

~/cloudflare$ cd workers-sdk/
[email protected] main ~/cloudflare/workers-sdk$ pnpm install
 ERROR  Unsupported engine for /Users/jldec/cloudflare/workers-sdk: wanted: {"pnpm":"^9.12.0"} (current: {"node":"v22.11.0","pnpm":"10.4.0"})
For help, run: pnpm help install

[email protected] main ~/cloudflare/workers-sdk$

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question A question about a feature rather than a bug quick win Potentially easy/straightforward issue to tackle regression Break in existing functionality as a result of a recent change workflows
Projects
Status: Untriaged
Development

No branches or pull requests

4 participants