Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rush] Improve accuracy of detecting pnpm shrinkwrap is modified #4252

Merged

Conversation

chengcyber
Copy link
Contributor

@chengcyber chengcyber commented Jul 25, 2023

Summary

Improve accuracy of detecting pnpm shrinkwrap modified

Details

Case 1: pnpm.overrides

With pnpm.overrides, the version in package.json is different from the specifier in pnpm lockfile. This MR changes the logic to compare the overrided version with specifier in lockfile when necessary.

Case 2: duplicated inconsistent version in dep and devDep.

There is a chance that one package declared both in dependencies and devDependencies with inconsistent versions (absurd). This MR ignores the version in devDependencies when detecting for pnpm lockfile v6

NOTE: when detecting pnpm lockfile v5, the implementation iterates specifiers in lockfile where dev dependencies' versions never show up.

How it was tested

Test cases are added

Impacted documentation

No

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenario of using pnpm.overrides to override the installed versions of dependencies declared in your own workspace package.json files is something that I would honestly consider an error case. You shouldn't be specifying one value in package.json and then using pnpm.overrides to install an incompatible version.

The case of having two different versions between dependencies and devDependencies is something I've considered doing for the purpose of having dependencies specify a range and devDependencies specify an exact version, but haven't seen particularly consistent behavior when that is attempted. Some tools error, some pick one or the other, etc.

As currently written I'm not sure exactly what this is solving, other than at least making the behavior in the aforementioned edge case of overriding the dependencies declared in the very same repository be consistent with the pnpm installation.

@chengcyber
Copy link
Contributor Author

chengcyber commented Jul 26, 2023

HI @dmichon-msft . Thank you for reviewing this!

For case 1, when the pnpm.overrides is specified, those projects depends on the overrided packages will be always marked as "out of date". which blocks rush install forever.

I created a repro repo at https://github.com/chengcyber/rush-pnpm-overrides-repro

For case 2, even though the case is specifying a range in dependencies and a fixed version in devDependencies, the project will be marked as "out of date". which blocks rush install forever.

I also created a repro repo at https://github.com/chengcyber/rush-pnpm-dep-devdep-repro

…d_2023-07-25-12-11.json

Co-authored-by: David Michon <dmichon@microsoft.com>
@iclanton iclanton enabled auto-merge August 2, 2023 18:22
@iclanton iclanton added this to In Progress in Bug Triage Aug 2, 2023
@iclanton iclanton merged commit 30e8a72 into microsoft:main Aug 2, 2023
3 checks passed
Bug Triage automation moved this from In Progress to Closed Aug 2, 2023
@chengcyber chengcyber deleted the improve-pnpm-shrinkwrap-modified branch August 14, 2023 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Bug Triage
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants