[rush] Fix an issue where usage of custom fields in dependenciesMeta caused rush install to think that shrinkwrap file is outdated #4992
Conversation
witcher112
requested review from
iclanton,
octogonz,
apostolisms,
D4N14L and
dmichon-msft
as code owners
|
@microsoft-github-policy-service agree |
| @@ -107,7 +111,7 @@ export class PackageJsonEditor { | |||
| const devDependencies: { [key: string]: string } = data.devDependencies || {}; | |||
| const resolutions: { [key: string]: string } = data.resolutions || {}; | |||
|
|
|||
| const dependenciesMeta: { [key: string]: { [key: string]: boolean } } = data.dependenciesMeta || {}; | |||
| const dependenciesMeta: { [key: string]: object } = data.dependenciesMeta || {}; | |||
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) => { | |||
There was a problem hiding this comment.
Can we update this to use Object.entries?
| @@ -50,19 +50,23 @@ export class PackageJsonDependency { | |||
| * @public | |||
| */ | |||
| export class PackageJsonDependencyMeta { | |||
| private _injected: boolean; | |||
| private _sourceData: object; | |||
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 <[email protected]>
|
@iclanton thanks for the review, I've pushed new changes according to your requests :) |
| "changes": [ | ||
| { | ||
| "packageName": "@rushstack/node-core-library", | ||
| "comment": "Extend IDependenciesMetaTable interface to support custom properties in \"dependenciesMeta\" entries", |
There was a problem hiding this comment.
| "comment": "Extend IDependenciesMetaTable interface to support custom properties in \"dependenciesMeta\" entries", | |
| "comment": "Extend `IDependenciesMetaTable` interface to support custom properties in `"dependenciesMeta"` entries", |
| @@ -1005,11 +1013,13 @@ export class PackageJsonDependency { | |||
|
|
|||
| // @public (undocumented) | |||
| export class PackageJsonDependencyMeta { | |||
| constructor(name: string, injected: boolean, onChange: () => void); | |||
| constructor(name: string, sourceData: IPackageJsonDependencyMetaSourceData, onChange: () => void); | |||
There was a problem hiding this comment.
This is a breaking change. Can this be reworked to maintain compatability?
| injected?: boolean; | ||
| [key: string]: unknown; |
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; |
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", |
There was a problem hiding this comment.
| "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", |
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