corepack icon indicating copy to clipboard operation
corepack copied to clipboard

Do not auto pin by default COREPACK_ENABLE_AUTO_PIN=0 is now the default

Open Lectem opened this issue 1 year ago • 3 comments

This fixes all issues related to auto-pin mentioned in #485.

Lectem avatar Aug 23 '24 11:08 Lectem

Github tells me that you requested changes but I'm not sure what changes you asked for ? (maybe the comments were not commited, I've seen this happen in the past if you don't submit the review)

Without auto-pinning, corepack is the source of significant bugs and failed builds, as the version of the package manager is essentially free floating.

While I do agree, this is very invasive as mentioned in the issue. To sum them up again here:

  • Any contributor using corepack will need to either ignore the package.json changes or try to submit their own version of the packagemanager field, which may not be well tested by the maintainers.
  • Corepack should not attempt to write read-only files, this defeats the whole purpose of some command parameters of the package managers such as npm ci npm install -save=false and pnpm --frozen-lockfile. I should be able to build without triggering "failed to write" issues.

That is why I made it so that a warning will be logged as long as the user does not provide the packageManager field. If it needs more discoverability, we could output it to stderr instead of the logs ?

corepack is the source of significant bugs and failed builds

I mean, that's not any different from people installing their own version of npm, pnpm or yarn. If the package.json does not restrict the package manager version properly, it's an issue whether or not you use corepack.

Lectem avatar Aug 24 '24 17:08 Lectem

An alternative to changing COREPACK_ENABLE_AUTO_PIN default, would be to actually prompt the user if they actually want you to do the change. In general, modifying (especially source) files without user consent is a bad idea IMHO.

So what one could do:

  • if user has a non redirected console (basicaly noTTY() && !CI)) => prompt them to apply the change
  • if no input or CI or file readonly => don't change the package.json, or fail, except if COREPACK_ENABLE_AUTO_PIN is set (to either 0 or 1 explicitly)

Lectem avatar Aug 24 '24 17:08 Lectem

I mean, that's not any different from people installing their own version of npm, pnpm or yarn. If the package.json does not restrict the package manager version properly, it's an issue whether or not you use corepack.

Indeed it's the same. The change was requested when it was asked to enable corepack by default with Node.js.

In that specific instance, there is no user will to install a package manager.

If the user does not know that they are actually installing a package manager (and what version that is), I think the only sensible thing to do is to add the packageManager field.


My -1 is that I don't think this should land at all in this form.

mcollina avatar Sep 01 '24 16:09 mcollina