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

feat: Pins the package manager as it's used for the first time #413

Merged
merged 3 commits into from Mar 3, 2024

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Feb 29, 2024

Once Corepack is enabled, someone restarting their container without having pinned the version of the package manager in packageManager may use different package manager versions pre-restart and post-restart, due to Corepack always defaulting on the highest release when the package manager isn't pinned (this was strongly requested in #93).

However, we want this "dynamic" selection to be slightly tuned down so it only affects projects as they are created - once a project is created or accessed for the first time, the package manager version should remain the same until the user decides otherwise. People in #399 seemed to agree this was the right behaviour, and I don't have very strong objections on that.

This diff implements this logic by checking whether the command is being run inside an arbitrary folder (NoProject), or a project folder whose package.json doesn't contain the packageManager field (NoSpec). In this last case, we write in the package.json the spec string for the package manager we'd have selected. No special handling is done to differentiate read commands (--version) from write commands (install), as I'm not certain this distinction is useful in practice.

Fixes #399; cc @mcollina

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.

This is a significant win.
Having a config or env variable to "skip" this behavior might be useful (the case where a dev uses a different package manager than the one listed in package.json)

As it might be slightly surprising to users, I think it might be better to write a message to stdout that a packageManager field was added to package.json, and to run corepack XX to update.

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.

lgtm

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can we, like the download prompt, make it default to 1 only when using the non-corepack binaries? For the same reason that, because it output stuff to stderr, it could break automation. (i.e. by default, yarn install would add packageManager field if absent, corepack yarn install would not)

sources/Engine.ts Outdated Show resolved Hide resolved
sources/Engine.ts Outdated Show resolved Hide resolved
@arcanis
Copy link
Contributor Author

arcanis commented Mar 1, 2024

Can we, like the download prompt, make it default to 1 only when using the non-corepack binaries?

I'd prefer to avoid branching the behaviour too much between corepack yarn and raw yarn. I don't expect corepack yarn to be a common case, so the inconsistency doesn't feel worth it (compared to people just having to set the env variable if they really wish to avoid this write).

- `COREPACK_ENABLE_AUTO_PIN` can be set to `0` to prevent Corepack from
updating the `packageManager` field when it detects that the local package
doesn't list it. In general we recommend to always list a `packageManager`
field (which you can easily set through `corepack use [name]@[version]`), as
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
field (which you can easily set through `corepack use [name]@[version]`), as
field (which you can set through `corepack use [name]@[version]`), as

@aduh95
Copy link
Contributor

aduh95 commented Mar 2, 2024

Can we, like the download prompt, make it default to 1 only when using the non-corepack binaries?

I'd prefer to avoid branching the behaviour too much between corepack yarn and raw yarn. I don't expect corepack yarn to be a common case, so the inconsistency doesn't feel worth it (compared to people just having to set the env variable if they really wish to avoid this write).

Fair enough.

Maybe it doesn't have to be in this PR, but I think it'd be valuable to have a way to ask Corepack to hard fail instead setting the file silently – I would like to enable that on my CI runs, so we can detect if a package.json is missing the field.

@arcanis arcanis merged commit 8b6c6d4 into main Mar 3, 2024
10 checks passed
@arcanis arcanis deleted the mael/pin-on-first-use branch March 3, 2024 12:50
@simonhaenisch
Copy link

@arcanis wondering if it makes sense that this feature gets similar behavior as COREPACK_ENABLE_DOWNLOAD_PROMPT (#360) to be disabled in non-interactive shells/CI by default? see

https://github.com/nodejs/corepack/pull/360/files#diff-f40bfea195fed9476186ef5bef59fff20a01e341513d8233ec399cfda410ebc0R19

@jakebailey
Copy link

I use corepack for everything, but this behavior is pretty annoying (now that I'm using Node 22 with this change) since I can't go into other people's projects without modifying their package.jsons. I see COREPACK_ENABLE_AUTO_PIN=0 should undo this, but it was super surprising when I just happened to be in someone else's repo, did npx ... and then got packageManager set.

@aduh95
Copy link
Contributor

aduh95 commented May 3, 2024

It definitely should not happen when using npx – but maybe what you call with npx is itself calling npm …? Could you open a new issue with a repro so we can investigate?

@jakebailey
Copy link

jakebailey commented May 3, 2024

In my case, it was just npx tsc (I believe). I will file an issue when I get a chance.

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.

Provide reproducible build by default
6 participants