infra: declare pnpm via devEngines.packageManager - #4001
Conversation
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #4001 +/- ##
==========================================
- Coverage 98.98% 98.92% -0.07%
==========================================
Files 926 926
Lines 3246 3246
Branches 569 569
==========================================
- Hits 3213 3211 -2
- Misses 29 31 +2
Partials 4 4 🚀 New features to boost your workflow:
|
bc44947 to
1b90106
Compare
There was a problem hiding this comment.
Note sure, whether we need exact pnpm version matches, but according to your comment pnpm requires it.
Renovate's npm manager tracks packageManager and knows nothing about devEngines.
So will every renovate pnpm PR explode due to it only ever updating only one of them?
If yes, that may be a reason to NACK this PR.
Can we test that in a test repo or something?
EDIT: It appears like this is the case
|
Does this PR provide any benefit other than blocking npm as package manager? |
I would even add a question on top of that: Why is blocking npm as the package manager a benefit to begin with? If a contributor does not want to install a completely new package manager to contribute, why force them? Is there any downside to using npm, other than slower dependency resolution for the user that choose to use npm? |
|
Ok, again many questions about a new thing I'm personally already used to. So lets start with https://github.com/faker-js/faker/pull/4002/changes#diff-dfa415899a467bf9067cd5ebdd0b6d359d1b25500bc76de5855631f459b3188bR45 example: this isn't really needed tbh, because we can utilize guarded tooling to prevent invalid usage. Like we use linter to guard coding, we can configure keys to guide with hard instead of soft fails. And this is not only helpful for humans, but especially for AI usage. e.g. when using claude, it tries very often in a fresh session to call So two questions from your side specifically:
pnpm is more than just a faster dependency manager, it also has MUCH higher safety net by not allowing to install dependency I hope the reasons above are already enough, but if you even want to force me harder, I will go into rabbit hole and find more. |
|
Nobody is arguing whether you should use I personally don't have anything against a somewhat strict pnpm@^v11 or [email protected] (as corepack basically handles it for me), but I would like to avoid getting into situations that get us stuck or "somebody else can clean up that warning/issue later". Maybe someone should open an issue with renovate so that both versions get updated in tandem. |
1b90106 to
4390d66
Compare
|
I merged #4002 for now, since currently you have to look the package.json yourself. This can potentially be simplified when this PR lands and thus this PR can change the respective docs as well. |
|
blocked by pnpm/pnpm#14009 |
Adds
devEngines.packageManagerto the rootpackage.jsonso that npm actually refuses to install this repo.The problem
packageManageronly does something when Corepack is enabled. npm itself never looks at it. So today this just works:npm resolves our whole tree, writes a
package-lock.json(which is in.gitignore, so nobody ever sees it in a diff) and leaves behind anode_modulesthat has nothing to do withpnpm-lock.yaml.npm run buildand the rest are equally happy. The first sign that something is off is usually a weird bug report.devEnginesis the field npm does read, and it checks it beforeinstall,ciandrun.The change
Only
packageManagerfor now. Noruntimeentry, Node is already covered byenginesand.nvmrc, and I did not want two topics in one PR.npm 11 and npm 12, before and after
Before, on both npm 11.19.0 and npm 12.0.2:
After:
Identical output for
npm run <script>, not only forinstall.This sits next to
packageManager, it does not replace itI did check whether we can drop the old field. We can't, at least not yet:
packageManager. WithdevEnginesalone it installs without a word and writes ayarn.lock.packageManager.devEngines.packageManagerlanded in pnpm 11.0.0, so pnpm 10 ignores it and keeps running itself instead of switching to 11.21.0.packageManager, and it chokes on a range indevEngines:Invalid package manager specification in package.json (pnpm@^11.21.0); expected a semver version.packageManagerand knows nothing aboutdevEngines.The two fields do different jobs.
packageManagerkeeps pinning the version for Corepack, pnpm andpnpm/action-setup,devEnginesis the part that makes npm say no.Why an exact version and why
onFail: downloadI got both of these wrong on the first attempt and only noticed because of the Docker runs below.
A range like
^11.21.0next to our exactpackageManagermakes pnpm complain on every single command, and it then resolves the range to the newest match (11.22.0 right now), quietly overriding the version we pinned for CI.onFail: "error"looks like the obvious choice, but it is a regression. Today someone on pnpm 11.22.0 gets silently switched down to 11.21.0 and never notices. Witherrorthey hit a wall instead.downloadkeeps the behaviour we have today: pnpm 10.34.5, 11.0.0 and 11.22.0 all end withDone in ...ms using pnpm v11.21.0and no warning.npm never validates the
onFailvalue here because the name mismatch short circuits first, so pnpm's non standarddownload(npm documents onlywarn,errorandignore) causes no trouble on the npm side.What a range and what onFail: error look like
Range
^11.21.0plus"packageManager": "[email protected]", printed before every command:[WARN] "packageManager" and "devEngines.packageManager" specify different versions of pnpm in package.json. "packageManager" will be ignoredExact version with
onFail: "error", running pnpm 11.22.0:That error also fires for anyone on a pnpm 11 patch we did not pin, which is not what I want to hand a first time contributor.
Lockfile
pnpm-lock.yamlgrows at the top by some lines. pnpm writes a new leading YAML document withpackageManagerDependenciesin it (pnpm,@pnpm/exeand the seven platform binaries with their integrity hashes). It is a single hunk at the top of the file, nothing else moves, no removals.I made sure this cannot break CI.
pnpm install --frozen-lockfilepasses in all three cases I could think of: against a lockfile from before this PR, against the regenerated one, and against one where onlypackageManagerwas bumped.How I checked all of this
The docs disagree with each other on a few points, so instead of guessing I built a throwaway Docker matrix. Seven images:
npm installtodaynpm install/pnpm install/yarn installwith this PREBADDEVENGINES, exit 1EBADDEVENGINES, exit 1Each image runs against 8 stripped down
package.jsonfixtures (with and withoutpackageManager, with and withoutdevEngines, exact version vs range,onFail: errorvsdownload) and executes both<pm> installand<pm> run <script>in each of them, since npm validates beforeruntoo. Then I replayed the winning fixture against the realpackage.jsonandpnpm-lock.yamlin a container to get the actual lockfile diff.The setup is not part of this PR, it lives in my local scratch folder. I can push it as a gist if anyone wants to re-run it.
yarn, for completeness (unchanged by this PR)
yarn 1 classic is already blocked by
packageManager, though the message is a bit of a mess:Note the
"yarn@[email protected]", yarn 1 blindly prefixesyarn@to whatever it finds. Ugly, but it exits 1.yarn 4 through Corepack is clean:
Both of these come from
packageManager, not fromdevEngines. That is the main reason the old field stays.Follow up
Renovate will bump
packageManagerand leavedevEngines.packageManageron the old version, and from that moment pnpm prints the "specify different versions" warning on every command until someone fixes it by hand. Either we add acustomManagersregex torenovate.json5, or we just keep an eye on it during review. I leftrenovate.json5untouched here, say the word if you want it in this PR.