feat: Pins the package manager as it's used for the first time #413
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
I'd prefer to avoid branching the behaviour too much between |
| - `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 |
There was a problem hiding this comment.
| field (which you can easily set through `corepack use [name]@[version]`), as | |
| field (which you can set through `corepack use [name]@[version]`), as |
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 |
|
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 |
|
It definitely should not happen when using |
|
In my case, it was just |
Latest Node 20 (20.13.1) ships with an updated corepack which seems to insist putting a package manager field in package.json (nodejs/corepack#413). Let it have its way, hoping that this doesn't break someone's workflow (depending on how they installed yarn without corepack or if they have a node version that doesn't have corepack).
Latest Node 20 (20.13.1) ships with an updated corepack which seems to insist putting a package manager field in package.json (nodejs/corepack#413). Let it have its way, hoping that this doesn't break someone's workflow (depending on how they installed yarn without corepack or if they have a node version that doesn't have corepack).
1688: Remove cross-fetch dependency r=brunoocasali a=flevi29 # Pull Request ## Related issue Fixes #1658 ## What does this PR do? - removes `cross-fetch` dependency - if anyone might need the polyfill they should run it themselves - there's an interesting discussion [here](w3ctag/polyfills#6 (comment)) about polyfills in libraries; I think, as a lot of people do, that the sane and clean path ahead is to make it the responsibility of the library user to polyfill, even if it's somewhat of a burden ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! 1698: Add `"packageManager"` entry to package.json r=brunoocasali a=flevi29 # Pull Request ## Why? - [`corepack`](https://nodejs.org/docs/latest-v20.x/api/corepack.html) [started adding this field automatically](nodejs/corepack#413), which can be disabled, but everyone has to do it separately, which is very annoying - anyhow there's no harm in it, `corepack` recommends it even in the "how to disable" doc https://github.com/nodejs/corepack/blob/main/README.md#environment-variables - > it ensures that your project installs are always deterministic (if you use `corepack`) - this does mean that this field will have to be updated periodically, but this is the recommended way ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: F. Levi <55688616+flevi29@users.noreply.github.com>
1698: Add `"packageManager"` entry to package.json r=brunoocasali a=flevi29 # Pull Request ## Why? - [`corepack`](https://nodejs.org/docs/latest-v20.x/api/corepack.html) [started adding this field automatically](nodejs/corepack#413), which can be disabled, but everyone has to do it separately, which is very annoying - anyhow there's no harm in it, `corepack` recommends it even in the "how to disable" doc https://github.com/nodejs/corepack/blob/main/README.md#environment-variables - > it ensures that your project installs are always deterministic (if you use `corepack`) - this does mean that this field will have to be updated periodically, but this is the recommended way ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: F. Levi <55688616+flevi29@users.noreply.github.com>
Once Corepack is enabled, someone restarting their container without having pinned the version of the package manager in
packageManagermay 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 whosepackage.jsondoesn't contain thepackageManagerfield (NoSpec). In this last case, we write in thepackage.jsonthe 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