[rush] Fix edge cases where Rush does not update the lockfile #4910
Conversation
L-Qun
requested review from
iclanton,
octogonz,
apostolisms,
D4N14L and
dmichon-msft
as code owners
| @@ -1002,6 +1002,7 @@ export class PnpmShrinkwrapFile extends BaseShrinkwrapFile { | |||
| } | |||
| // If fall through, there is a chance the package declares an inconsistent version, ignore it. | |||
| isDevDepFallThrough = true; | |||
| break; | |||
| } | |||
|
|
|||
| // eslint-disable-next-line no-fallthrough | |||
There was a problem hiding this comment.
| // eslint-disable-next-line no-fallthrough |
There was a problem hiding this comment.
Given that this explicit rule suppression is here, it seems like there was a reason for this behavior initially. Are you sure unit tests adequately cover all scenarios here?
There was a problem hiding this comment.
I'm not sure. But I noticed that you added this rule suppression. Is there a specific case for that?
There was a problem hiding this comment.
Or maybe I guess it was just an oversight.
There was a problem hiding this comment.
@chengcyber and I reviewed this today. It is really a spec change, not a bug:
In the past, whether a package was a devDependency or regular dependency did not affect the node_modules folder. The distinction maybe was reflected in the lockfile, but the installation plan was equivalent.
But now, apparently rush install fails if PNPM finds that the lockfile is inconsistent with package.json with regards to devDependencies-vs-dependencies. We need to modify the spec, such that Rush's analysis now will treat devDependencies vs dependencies as a significant difference.
This PR technically fixes the behavior by inserting a break statement, however the change is a bit confusing, because the code comments and structure are unchanged. The /intent/ of the code appears to be the same, but in fact we have changed it.
Possible improvements:
- Add some code comments better explaining the expected behavior
- Possibly restructure the code to no longer rely on case statements that fallthrough
- Ideally add some unit tests with examples of the edge cases
| @@ -1002,6 +1002,7 @@ export class PnpmShrinkwrapFile extends BaseShrinkwrapFile { | |||
| } | |||
| // If fall through, there is a chance the package declares an inconsistent version, ignore it. | |||
There was a problem hiding this comment.
This should probably be an error case.
There was a problem hiding this comment.
Making this an error case would likely mitigate whatever issue the existing code was trying to account for.
There was a problem hiding this comment.
Making this an error case would likely mitigate whatever issue the existing code was trying to account for.
I'm not sure about that. If the inconsistency is in an indirect dependency, and PNPM tolerates it, then Rush probably should as well.
...on/changes/@microsoft/rush/fix-edge-case-rush-does-not-update-lockfile_2024-08-30-06-44.json
Outdated
Show resolved
Hide resolved
iclanton
changed the title
…ate-lockfile_2024-08-30-06-44.json Co-authored-by: Ian Clanton-Thuon <[email protected]>
Co-authored-by: Ian Clanton-Thuon <[email protected]>
Details
If the user moves a package from
dependenciestodevDependencies, and then runsrush update, Rush should update the lockfile correctly, but it does not. This issue occurs because it triggerscase DependencyType.Dev, but Rush cannot find this package inimporter.devDependencies. Consequently, Rush deletes it from theimporterDependenciesset.Impacted documentation
None.