[rush] Fix an issue where usage of custom fields in dependenciesMeta caused rush install to think that shrinkwrap file is outdated#4992
Open
witcher112 wants to merge 7 commits into
Conversation
witcher112
requested review from
D4N14L,
apostolisms,
dmichon-msft,
iclanton and
octogonz
as code owners
November 6, 2024 16:18
Contributor
Author
|
@microsoft-github-policy-service agree |
iclanton
reviewed
Nov 11, 2024
| const resolutions: { [key: string]: string } = data.resolutions || {}; | ||
|
|
||
| const dependenciesMeta: { [key: string]: { [key: string]: boolean } } = data.dependenciesMeta || {}; | ||
| const dependenciesMeta: { [key: string]: object } = data.dependenciesMeta || {}; |
Member
There was a problem hiding this comment.
Can this just reference IDependenciesMetaYaml? Or it probably makes more sense to define that type here and point IDependenciesMetaYaml to that (assuming that the types truly are the same).
| @@ -183,7 +187,7 @@ export class PackageJsonEditor { | |||
| Object.keys(dependenciesMeta || {}).forEach((packageName: string) => { | |||
Member
There was a problem hiding this comment.
Can we update this to use Object.entries?
| */ | ||
| export class PackageJsonDependencyMeta { | ||
| private _injected: boolean; | ||
| private _sourceData: object; |
Member
There was a problem hiding this comment.
And then use the more complete type here and in other places.
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Contributor
Author
|
@iclanton thanks for the review, I've pushed new changes according to your requests :) |
iclanton
approved these changes
Nov 18, 2024
| "changes": [ | ||
| { | ||
| "packageName": "@rushstack/node-core-library", | ||
| "comment": "Extend IDependenciesMetaTable interface to support custom properties in \"dependenciesMeta\" entries", |
Member
There was a problem hiding this comment.
Suggested change
| "comment": "Extend IDependenciesMetaTable interface to support custom properties in \"dependenciesMeta\" entries", | |
| "comment": "Extend `IDependenciesMetaTable` interface to support custom properties in `"dependenciesMeta"` entries", |
| // @public (undocumented) | ||
| export class PackageJsonDependencyMeta { | ||
| constructor(name: string, injected: boolean, onChange: () => void); | ||
| constructor(name: string, sourceData: IPackageJsonDependencyMetaSourceData, onChange: () => void); |
Member
There was a problem hiding this comment.
This is a breaking change. Can this be reworked to maintain compatability?
Comment on lines
70
to
+71
| injected?: boolean; | ||
| [key: string]: unknown; |
Member
There was a problem hiding this comment.
Instead of duplicating this in libraries/rush-lib/src/api/PackageJsonEditor.ts, can you just extract this object to its own interface and use that in rush-lib?
| export interface IDependenciesMetaYaml { | ||
| injected?: boolean; | ||
| } | ||
| export type IDependenciesMetaYaml = IPackageJsonDependencyMetaSourceData; |
Member
There was a problem hiding this comment.
Just get rid of this and have other things reference the other type.
| "changes": [ | ||
| { | ||
| "packageName": "@microsoft/rush", | ||
| "comment": "Fix an issue where usage of custom fields in dependenciesMeta caused rush install to think that shrinkwrap file is outdated", |
Member
There was a problem hiding this comment.
Suggested change
| "comment": "Fix an issue where usage of custom fields in dependenciesMeta caused rush install to think that shrinkwrap file is outdated", | |
| "comment": "Fix an issue where usage of custom fields in `dependenciesMeta` caused `rush install` to think that the lockfile is outdated", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In our project, we're using custom fields in "dependenciesMeta" to supply additional features in our custom build chain.
As it turns out, pnpm stores those in pnpm-lock.yaml file and current Rush mechanism fails to properly compare them as it's limited to retrieving "injected" property.
Details
I've extended the mechanism to retrieve an entire object from "dependenciesMeta" and compare it instead of hardcoding "injected" field.
Aside from covering our use case, this would prepare Rush for future fields that might be introduced in "dependenciesMeta" by pnpm.
How it was tested
I've verified the fix in our repo where we use custom dependenciesMeta.
Impacted documentation