[rush] PNPM Overrides with more advanced syntax will cause Rush to always complain that shrinkwrap file need to be updated

[kenrick95]

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:

rushstack/libraries/rush-lib/src/logic/pnpm/PnpmShrinkwrapFile.ts

Line 948 in 0c41a82

const resolvedVersion: string = this.overrides.get(importerPackageName) ?? foundDependency.version;

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

[dmichon-msft]

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

	// TODO: Emit an error message when someone tries to override a version of something in one of their
	// local repo packages.
	let resolvedVersion: string = this.overrides.get(name) ?? version;

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

[kenrick95]

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

[chengcyber]

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

rushstack/libraries/rush-lib/src/logic/pnpm/PnpmfileConfiguration.ts

Line 128 in 4013c5a

public transform(packageJson: IPackageJson): IPackageJson {

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

[octogonz]

We discussed this and I think arrived at two separate decisions:

1. 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.

2. Don't rely on transforms at all for rush install. Today rush 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 successful rush 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.