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
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
@@ -244,6 +244,7 @@ export interface IDependenciesMetaTable {
// (undocumented)
[dependencyName: string]: {
injected?: boolean;
[key: string]: unknown;
};
}

12 changes: 11 additions & 1 deletion common/reviews/api/rush-lib.api.md
Original file line number Diff line number Diff line change
@@ -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;
@@ -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)
1 change: 1 addition & 0 deletions libraries/node-core-library/src/IPackageJson.ts
Original file line number Diff line number Diff line change
@@ -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?

};
}

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

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

/**
* @public
*/
@@ -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;
}
}

@@ -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);

@@ -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)
);
});

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

Original file line number Diff line number Diff line change
@@ -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
16 changes: 10 additions & 6 deletions libraries/rush-lib/src/logic/pnpm/PnpmShrinkwrapFile.ts
Original file line number Diff line number Diff line change
@@ -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';
@@ -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 {
@@ -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);
}
}
Loading
Oops, something went wrong.