Repository navigation
[BUG] corepack enable npm pnpm breaks 32 pnpm commands #419
Description
Context
pnpm shells out to npm for roughly 32 different commands.
Current Behavior
Enabling both pnpm & npm will break usage of numerous commands in pnpm projects.
Expected Behavior
pnpm commands would not break. Corepack should understand this nuance or remove npm.
Steps to Reproduce
corepack enable npm pnpmcorepack use pnpmpnpm loginget usage error (orpnpm access,pnpm dist-tag,pnpm org,pnpm team& many more)
Activity
aduh95 commentedon Mar 7, 2024
Hopefully this will fix itself when
devEnginesbecomes a thing.darcyclarke commentedon Mar 7, 2024
Corepack would need to migrate to that & the design of
devEngineshas only just kicked-off. Also, Corepack would still need to be aware of how these two package managers interact (ex. adding bothnpm&pnpmtopnpmproject'sdevEngines- which somewhat defeats the purpose of package manager lock-in since you would be able to callnpminside ofpnpmprojects again). I think, fundamentally, there should be a fix put in place for this scenario ifpnpm&npmare going to continue to be supported together. The alternative is forpnpmto drop its dependency onnpmbut that seems out of the scope of this project & the issue/errors I've run into are from Corepack.aduh95 commentedon Mar 7, 2024
On the contrary, I think that field would allow Corepack to defer to those tools rather than trying to guess – i.e.
COREPACK_ENABLE_STRICTwould default to 0.darcyclarke commentedon Mar 7, 2024
I'm definitely a big +1 on
COREPACK_ENABLE_STRICTbeing defaulted to0.arcanis commentedon Mar 7, 2024
Why should Corepack change the default when the maintainer of the tool you mention is on the side of fixing it on his side?
darcyclarke commentedon Mar 7, 2024
@arcanis I don't see any reference to resolving this problem in
pnpmitself from the link you referenced.Setting
COREPACK_ENABLE_STRICT=0loosens the requirements of Corepack. It is not a fix to this problem.The potential fixes I can see here are below:
pnpmcan removenpmas a dependency (out of the scope of this project)npmis being executed bypnpminpnpmprojects (& not warn/throw in those cases)npm(proposed here)The first is out of our hands & I haven't heard of support for it yet from that project. The second requires some work & investigation to determine if it's even possible (which I don't have time for & don't think is good to add more project-specific logic). The last is the easiest/cleanest & the work has already been done.
aduh95 commentedon Mar 7, 2024
I think it would make sense if the check is implemented by the underlying tool anyway: you get the same UX (an error is thrown if you use the wrong package manager), but the logic is no longer in Corepack. It would also give 2. for free, as pnpm and npm will have to work out those things regardless of Corepack.
Maybe we can add some nuance to that (maybe strict mode is disabled only when
devEnginesis defined, or something like that), but it seems to me we could avoid the redundant check.It seems that removing
npmor changingCOREPACK_ENABLE_STRICTshould equally be "not a fix", no? In both cases, a feature is removed from Corepack.styfle commentedon Mar 7, 2024
That sounds reasonable since
devEnginesallows an array of package managers 👍arcanis commentedon Mar 7, 2024
Then we have very different reading comprehension. I myself read "pnpm can run npm with the COREPACK_ENABLE_STRICT=0 env variable" as a very clear explanation that pnpm could set the variable in the env when it calls npm.
darcyclarke commentedon Mar 7, 2024
Loosing the constraints (ie.
COREPACK_ENABLE_STRICT=0) is tangential to ensuringpnpmdoes not break whenCOREPACK_ENABLE_STRICT=1.In the example I provided I'm specifically concerned about
pnpm&pnpmprojects. Hypotheticallynpmcould have been enabled at any point. The fact is that I should not be able to get into a state where executingpnpminside apnpmproject throws just becausenpmwas enabled (as it should not be possible to enablenpm- ref. https://nodejs.org/docs/latest/api/corepack.html#supported-package-managers).As you can see, even zoltan is surprised I brought this up because they did not think
npmwas included in Corepack (which is why I'm still recommending #418 lands)I don't think this is a necessary/helpful comment to make.
You've added conditions which don't meet the requirements. I want Corepack to strictly manage
pnpm.pnpmjust so happens to have a dependency onnpm. Whennpmis enabled - which shouldn't be possible - executingpnpmvia Corepack breaks the numerous commands I've mentioned. There should be no requirement to setCOREPACK_ENABLE_STRICT=0because a package manager is being enabled that shouldn't be possible.arcanis commentedon Mar 7, 2024
Then don't run
corepack enable npm? Why did you run this command if you didn't intend to use npm through Corepack?aduh95 commentedon Mar 7, 2024
As a workaround, you can either disable npm (by running
corepack disable npm), or setCOREPACK_ENABLE_STRICT=0in your env. If you haven't already, I can open try to open a PR to with a fix/workaround on pnpm side.Another solution that might unblock you is to install pnpm using their standalone script, which does not use Corepack, and you won't get that limitation – I think their standalone script also installs npm for you.
aduh95 commentedon Mar 8, 2024
The fix on pnpm has landed, I think this can be closed. Until the fix makes its way to a pnpm release, you can use any of the workarounds described in my previous comment. Thanks for the report and let me know if you need any more help!
darcyclarke commentedon Apr 1, 2024
Just want to note that the fix landed in
pnpm@>=8.15.5but all previous versions will still exhibit this issue (~1k versions). If you're running into this problem today, hopefully you land here & now know you should setCOREPACK_ENABLE_STRICT=0when usingpnpm@<8.15.5.