[rush] PNPM Overrides with more advanced syntax will cause Rush to always complain that shrinkwrap file need to be updated
Opened this issue · 4 comments
Summary
Using PNPM Overrides feature with a more advanced syntax like <pkg-name>@^<ver>
in the keys will cause rush install
to always fail
Repro steps
At common/config/rush/pnpm-config.json
, I have:
"globalOverrides": {
"react-router@^5": "5.3.4"
},
At meow/package.json
, I have:
{
"dependencies": {
"react-router": "^5.0.0"
}
}
Then I run:
rush update
rush install
Expected result: rush install
runs successfully
Actual result: rush install
failed with:
Rush Multi-Project Build Tool 5.122.1 - https://rushjs.io
Node.js version is 18.20.2 (LTS)
Starting "rush install"
Validating package manager shrinkwrap file.
Trying to acquire lock for pnpm-8.15.8
Acquired lock for pnpm-8.15.8
Found pnpm version 8.15.8 in /user-path/.rush/node-v18.20.2/pnpm-8.15.8
Symlinking "/path-to-repo/common/temp/pnpm-local"
--> "/user-path/.rush/node-v18.20.2/pnpm-8.15.8"
Transforming /path-to-repo/common/config/rush/.npmrc
--> "/path-to-repo/common/temp/.npmrc"
Updating workspace files in /path-to-repo/common/temp
Copying "/path-to-repo/common/config/rush/pnpm-lock.yaml"
--> "/path-to-repo/common/temp/pnpm-lock.yaml"
Copying "/path-to-repo/common/config/rush/pnpm-lock.yaml"
--> "/path-to-repo/common/temp/pnpm-lock-preinstall.yaml"
The shrinkwrap file (pnpm-lock.yaml) contains the following issues:
Dependencies of project "@kenrick95/meow-1" do not match the current shrinkwrap.
The shrinkwrap file (pnpm-lock.yaml) is out of date. You need to run "rush update".
Repo: https://github.com/kenrick95/rush-repro-pnpm-overrides
Details
I found that this code:
only handle case where importerPackageName
exist exactly in the overrides
map.
However, from PNPM Overrides docs, we can specify overrides like:
"bar@^2.1.0": "3.0.0"
or
"qar@1>zoo": "2"
but it seems like it is not handled when it is first implemented in the PR: #4252 ( cc @chengcyber )
As for why pnpm overrides is used, for my case, it is from migrating from a repo that plainly uses PNPM Workspaces to Rush, so naturally we'll migrate those overrides config. For this case, the intention was to make sure everyone (whether projects within same workspace or external dependencies) are on the same react-router version without changing too many codes that isn't "owned" by the "platform-level" team.
TODO: Emit an error message when someone tries to override a version of something in one of their local repo packages.
Update 2024-05-06: I saw the TODO there and it actually makes sense to throw error too... So I'm not sure whether this should be fixed by adding override parsing, or fixed by throwing error when this case occurred.
Standard questions
Please answer these questions to help us investigate your issue more quickly:
Question | Answer |
---|---|
@microsoft/rush globally installed version? |
5.117.8 |
rushVersion from rush.json? |
5.122.1 |
useWorkspaces from rush.json? |
true |
Operating system? | Mac |
Would you consider contributing a PR? | Yes |
Node.js version (node -v )? |
18.20.2 |
There's a TODO in the code just above where you linked to consider using globalOverrides
to modify the evaluation of workspace-local package.json
files to be an error:
rushstack/libraries/rush-lib/src/logic/pnpm/PnpmShrinkwrapFile.ts
Lines 1007 to 1009 in 0c41a82
The basis for this is that globalOverrides
is intended to be a mechanism for compensating for incorrect content in package.json
in external dependencies, so if the contents of a local package.json
are wrong, that package.json
should be directly modified.
Enforcing the installed version dependency in workspace packages is the domain of common/config/common-versions.json
and the ensureConsistentVersions
setting: https://rushjs.io/pages/maintainer/recommended_settings/#ensureconsistentversions
I see... so this should throw an error I guess. At the moment, there's no "hint" of what went wrong and users will keep retrying to rush update
without avail
Hi @kenrick95
but it seems like it is not handled when it is first implemented in the PR
You are right. The initial implementation doesn't cover the advanced syntax, as it was a quick fix for the urgent issues in our company's monorepo.
I just checked this requirement again to see whether I can quickly support it. However, It seems to me a little bit complicated to support the advance syntax. Technically, PnpmfileConfiguration#transform
needs to know the overrides configuration and mimic the pnpm's fashion to patch hooks.readPackage
function if overrides exist
Further, to make it 100% accurate, there are other configurations should be supported as well. Related pnpm code is
https://github.com/pnpm/pnpm/blob/aa33269f9f9fc0c3505ae1c59264d1706923a971/hooks/read-package-hook/src/createReadPackageHook.ts
We discussed this and I think arrived at two separate decisions:
-
Create a library that performs accurate transforms.
PnpmfileConfiguration.ts
should be split into two separate APIs: One half is the Rush-specific API whose purpose is only to generate the.pnpmfile.cjs
shim. The other half will be a common library whose purpose is to accurately transform a package.json file the way that PNPM would do. This involves loading.pnpmfile.cjs
(ideally into an isolated web worker that can be cleaned up easily), as well as applying other transforms such as"overrides"
that do not involve any script code, probably leveraging the@pnpm/hooks.read-package-hook
package that @chengcyber mentioned. -
Don't rely on transforms at all for
rush install
. Todayrush install
is transforming package.json because it is trying to compare project dependency versions against the lockfile to determine whether the lockfile is up-to-date. This was a good idea in the old days, but PNPM is now so complicated and so reliable that such logic has become difficult to support. A better model would be to focus purely on inputs: (1) collect all the inputs such as package.json, pnpm-config.json, .pnpmfile.cjs, etc. (2) normalize the file contents by removing irrelevant fields then sorting, (3) hash the result, and then (4) compare those hashes against hashes saved from the previous successfulrush update
. This avoids the need for semantic analysis of lockfiles, and so is a much simpler algorithm, and will be much more robust for behavioral changes across PNPM releases.
Thus, the proposed common library for accurate package.json transforms is really only needed by rush deploy
and the Lockfile Explorer app. In both cases, although we will aim for 100% accurate transforms of package.json, it is not a big deal if there are some minor mistakes. (Lockfile Explorer is just a viewer, and rush deploy
's analysis is technically a heuristic that can be fixed up using manual configurations.)
These two actions are separate work items that can be implemented in any order. #2 is probably the quickest fix for #4675.