Open kenrick95 opened 5 months ago
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: https://github.com/microsoft/rushstack/blob/0c41a82312ebe4c4cf02950c7a000a8ea9de63b8/libraries/rush-lib/src/logic/pnpm/PnpmShrinkwrapFile.ts#L1007-L1009
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
https://github.com/microsoft/rushstack/blob/4013c5acb1d60dce24272b6b1155f9396a486759/libraries/rush-lib/src/logic/pnpm/PnpmfileConfiguration.ts#L128
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
. 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.
Summary
Using PNPM Overrides feature with a more advanced syntax like
<pkg-name>@^<ver>
in the keys will causerush install
to always failRepro steps
At
common/config/rush/pnpm-config.json
, I have:At
meow/package.json
, I have:Then I run:
Expected result:
rush install
runs successfullyActual result:
rush install
failed with:Repo: https://github.com/kenrick95/rush-repro-pnpm-overrides
Details
I found that this code:
https://github.com/microsoft/rushstack/blob/0c41a82312ebe4c4cf02950c7a000a8ea9de63b8/libraries/rush-lib/src/logic/pnpm/PnpmShrinkwrapFile.ts#L948
only handle case where
importerPackageName
exist exactly in theoverrides
map.However, from PNPM Overrides docs, we can specify overrides like:
or
but it seems like it is not handled when it is first implemented in the PR: https://github.com/microsoft/rushstack/pull/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.
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:
@microsoft/rush
globally installed version?rushVersion
from rush.json?useWorkspaces
from rush.json?node -v
)?