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] Fix an issue where usage of custom fields in dependenciesMeta caused rush install to think that shrinkwrap file is outdated #4992

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"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",
Copy link
Member

Choose a reason for hiding this comment

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

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",

"type": "none"
}
],
"packageName": "@microsoft/rush"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@rushstack/node-core-library",
"comment": "Extend IDependenciesMetaTable interface to support custom properties in \"dependenciesMeta\" entries",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"comment": "Extend IDependenciesMetaTable interface to support custom properties in \"dependenciesMeta\" entries",
"comment": "Extend `IDependenciesMetaTable` interface to support custom properties in `"dependenciesMeta"` entries",

"type": "minor"
}
],
"packageName": "@rushstack/node-core-library"
}
1 change: 1 addition & 0 deletions common/reviews/api/node-core-library.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ export interface IDependenciesMetaTable {
// (undocumented)
[dependencyName: string]: {
injected?: boolean;
[key: string]: unknown;
};
}

Expand Down
12 changes: 11 additions & 1 deletion common/reviews/api/rush-lib.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -684,6 +684,14 @@ export interface _IOperationStateJson {
nonCachedDurationMs: number;
}

// @public (undocumented)
export interface IPackageJsonDependencyMetaSourceData {
// (undocumented)
[key: string]: unknown;
// (undocumented)
injected?: boolean;
}

// @public
export interface IPackageManagerOptionsJsonBase {
environmentVariables?: IConfigurationEnvironment;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change. Can this be reworked to maintain compatability?

// (undocumented)
get injected(): boolean;
// (undocumented)
readonly name: string;
// (undocumented)
get sourceData(): IPackageJsonDependencyMetaSourceData;
}

// @public (undocumented)
Expand Down
1 change: 1 addition & 0 deletions libraries/node-core-library/src/IPackageJson.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export interface IPeerDependenciesMetaTable {
export interface IDependenciesMetaTable {
[dependencyName: string]: {
injected?: boolean;
[key: string]: unknown;
Comment on lines 70 to +71
Copy link
Member

Choose a reason for hiding this comment

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

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?

};
}

Expand Down
27 changes: 20 additions & 7 deletions libraries/rush-lib/src/api/PackageJsonEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ export enum DependencyType {
YarnResolutions = 'resolutions'
}

/**
* @public
*/
export interface IPackageJsonDependencyMetaSourceData {
injected?: boolean;
[key: string]: unknown;
}

/**
* @public
*/
Expand Down Expand Up @@ -50,19 +58,23 @@ export class PackageJsonDependency {
* @public
*/
export class PackageJsonDependencyMeta {
private _injected: boolean;
private _sourceData: IPackageJsonDependencyMetaSourceData;
private _onChange: () => void;

public readonly name: string;

public constructor(name: string, injected: boolean, onChange: () => void) {
public constructor(name: string, sourceData: IPackageJsonDependencyMetaSourceData, onChange: () => void) {
this.name = name;
this._injected = injected;
this._sourceData = sourceData;
this._onChange = onChange;
}

public get sourceData(): IPackageJsonDependencyMetaSourceData {
return this._sourceData;
}

public get injected(): boolean {
return this._injected;
return this._sourceData.injected ?? false;
}
}

Expand Down Expand Up @@ -107,7 +119,8 @@ 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]: IPackageJsonDependencyMetaSourceData } =
data.dependenciesMeta || {};

const _onChange: () => void = this._onChange.bind(this);

Expand Down Expand Up @@ -180,10 +193,10 @@ export class PackageJsonEditor {
);
});

Object.keys(dependenciesMeta || {}).forEach((packageName: string) => {
Object.entries(dependenciesMeta || {}).forEach(([packageName, dependencyMeta]: [string, IPackageJsonDependencyMetaSourceData]) => {
this._dependenciesMeta.set(
packageName,
new PackageJsonDependencyMeta(packageName, dependenciesMeta[packageName].injected, _onChange)
new PackageJsonDependencyMeta(packageName, dependencyMeta, _onChange)
);
});

Expand Down
1 change: 1 addition & 0 deletions libraries/rush-lib/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export {
PackageJsonEditor,
PackageJsonDependency,
DependencyType,
type IPackageJsonDependencyMetaSourceData,
PackageJsonDependencyMeta
} from './api/PackageJsonEditor';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,9 +325,7 @@ export class WorkspaceInstallManager extends BaseInstallManager {
if (dependencyMetaList.length !== 0) {
const dependenciesMeta: IDependenciesMetaTable = {};
for (const dependencyMeta of dependencyMetaList) {
dependenciesMeta[dependencyMeta.name] = {
injected: dependencyMeta.injected
};
dependenciesMeta[dependencyMeta.name] = dependencyMeta.sourceData;
}

// get the relative path from common temp folder to package folder, to align with the value in pnpm-lock.yaml
Expand Down
16 changes: 10 additions & 6 deletions libraries/rush-lib/src/logic/pnpm/PnpmShrinkwrapFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,12 @@ import type { IShrinkwrapFilePolicyValidatorOptions } from '../policy/Shrinkwrap
import { PNPM_SHRINKWRAP_YAML_FORMAT } from './PnpmYamlCommon';
import { RushConstants } from '../RushConstants';
import type { IExperimentsJson } from '../../api/ExperimentsConfiguration';
import { DependencyType, type PackageJsonDependency, PackageJsonEditor } from '../../api/PackageJsonEditor';
import {
DependencyType,
type PackageJsonDependency,
type IPackageJsonDependencyMetaSourceData,
PackageJsonEditor
} from '../../api/PackageJsonEditor';
import type { RushConfigurationProject } from '../../api/RushConfigurationProject';
import { PnpmfileConfiguration } from './PnpmfileConfiguration';
import { PnpmProjectShrinkwrapFile } from './PnpmProjectShrinkwrapFile';
Expand All @@ -32,15 +37,14 @@ import { PnpmOptionsConfiguration } from './PnpmOptionsConfiguration';
import type { IPnpmfile, IPnpmfileContext } from './IPnpmfile';
import type { Subspace } from '../../api/Subspace';
import { CustomTipId, type CustomTipsConfiguration } from '../../api/CustomTipsConfiguration';
import { objectsAreDeepEqual } from '../../utilities/objectUtilities';

const yamlModule: typeof import('js-yaml') = Import.lazy('js-yaml', require);

export interface IPeerDependenciesMetaYaml {
optional?: boolean;
}
export interface IDependenciesMetaYaml {
injected?: boolean;
}
export type IDependenciesMetaYaml = IPackageJsonDependencyMetaSourceData;
Copy link
Member

Choose a reason for hiding this comment

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

Just get rid of this and have other things reference the other type.


export type IPnpmV7VersionSpecifier = string;
export interface IPnpmV8VersionSpecifier {
Expand Down Expand Up @@ -1040,8 +1044,8 @@ export class PnpmShrinkwrapFile extends BaseShrinkwrapFile {
}
}

for (const { name, injected } of dependencyMetaList) {
if (importer.dependenciesMeta?.[name]?.injected === injected) {
for (const { name, sourceData } of dependencyMetaList) {
if (objectsAreDeepEqual(importer.dependenciesMeta?.[name], sourceData)) {
importerDependenciesMeta.delete(name);
}
}
Expand Down
Loading