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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Lectem
Copy link

@Lectem Lectem commented Aug 23, 2024

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

sources/Engine.ts Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

auto-pinning solve the reproducibility problem that npm bundled with Node.js guarantees.

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

@Lectem
Copy link
Author

Lectem commented Aug 24, 2024

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
Copy link
Author

Lectem commented Aug 24, 2024

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)

@mcollina
Copy link
Member

mcollina commented Sep 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants