10000 Merge pull request #3314 from iclanton/fix-rush-add · psy-repos-typescript/rushstack@6ec531c · GitHub
[go: up one dir, main page]
Skip to content

Commit 6ec531c

Browse files
iclantoniclanton
authored
Merge pull request microsoft#3314 from iclanton/fix-rush-add
[rush] Fix a few issues with the way "rush add" selects versions.
2 parents 9635919 + 9d92170 commit 6ec531c

File tree

6 files changed

+277
-201
lines changed

6 files changed

+277
-201
lines changed

‎apps/rush-lib/src/api/RushConfiguration.ts

Lines changed: 78 additions & 61 deletions
A3E2
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ import { ExperimentsConfiguration } from './ExperimentsConfiguration';
3333
import { PackageNameParsers } from './PackageNameParsers';
3434
import { RepoStateFile } from '../logic/RepoStateFile';
3535
import { LookupByPath } from '../logic/LookupByPath';
36-
import { PackageJsonDependency } from './PackageJsonEditor';
36+
import { DependencyType, PackageJsonDependency } from './PackageJsonEditor';
3737
import { RushPluginsConfiguration } from './RushPluginsConfiguration';
3838

3939
const MINIMUM_SUPPORTED_RUSH_JSON_VERSION: string = '0.0.0';
@@ -502,6 +502,8 @@ export class RushConfiguration {
502502
private _commonVersionsConfigurations: Map<string, CommonVersionsConfiguration> | undefined;
503503
// variant -> map of package name -> implicitly preferred version
504504
private _implicitlyPreferredVersions: Map<string, Map<string, string>> | undefined;
505+
// variant -> map of package name to a set of specified dependency versions
506+
private _allDependencyVersions: Map<string, Map<string, Set<string>>> | undefined;
505507

506508
private _versionPolicyConfiguration: VersionPolicyConfiguration;
507509
private _versionPolicyConfigurationFilePath: string;
@@ -1586,15 +1588,11 @@ export class RushConfiguration {
15861588
}
15871589

15881590
// Use an empty string as the key when no variant provided. Anything else would possibly conflict
1589-
// with a varient created by the user
1591+
// with a variant created by the user
15901592
const variantKey: string = variant || '';
15911593
let implicitlyPreferredVersions: Map<string, string> | undefined =
15921594
this._implicitlyPreferredVersions.get(variantKey);
15931595
if (!implicitlyPreferredVersions) {
1594-
// First, collect all the direct dependencies of all local projects, and their versions:
1595-
// direct dependency name --> set of version specifiers
1596-
const versionsForDependencies: Map<string, Set<string>> = new Map<string, Set<string>>();
1597-
15981596
// Only generate implicitly preferred versions for variants that request it
15991597
const commonVersionsConfiguration: CommonVersionsConfiguration = this.getCommonVersions(variant);
16001598
const useImplicitlyPreferredVersions: boolean =
@@ -1603,14 +1601,9 @@ export class RushConfiguration {
16031601
: true;
16041602

16051603
if (useImplicitlyPreferredVersions) {
1606-
for (const project of this.projects) {
1607-
this._collectVersionsForDependencies(
1608-
versionsForDependencies,
1609-
[...project.packageJsonEditor.dependencyList, ...project.packageJsonEditor.devDependencyList],
1610-
project.cyclicDependencyProjects,
1611-
variant
1612-
);
1613-
}
1604+
// First, collect all the direct dependencies of all local projects, and their versions:
1605+
// direct dependency name --> set of version specifiers
1606+
const versionsForDependencies: Map<string, Set<string>> = this._getAllDependencyVersions(variant);
16141607

16151608
// If any dependency has more than one version, then filter it out (since we don't know which version
16161609
// should be preferred). What remains will be the list of preferred dependencies.
@@ -1803,59 +1796,83 @@ export class RushConfiguration {
18031796
return undefined;
18041797
}
18051798

1806-
private _collectVersionsForDependencies(
1807-
versionsForDependencies: Map<string, Set<string>>,
1808-
dependencies: ReadonlyArray<PackageJsonDependency>,
1809-
cyclicDependencies: Set<string>,
1810-
variant: string | undefined
1811-
): void {
1812-
const commonVersions: CommonVersionsConfiguration = this.getCommonVersions(variant);
1813-
const allowedAlternativeVersions: Map<
1814-
string,
1815-
ReadonlyArray<string>
1816-
> = commonVersions.allowedAlternativeVersions;
1817-
1818-
for (const dependency of dependencies) {
1819-
const alternativesForThisDependency: ReadonlyArray<string> =
1820-
allowedAlternativeVersions.get(dependency.name) || [];
1821-
1822-
// For each dependency, collectImplicitlyPreferredVersions() is collecting the set of all version specifiers
1823-
// that appear across the repo. If there is only one version specifier, then that's the "preferred" one.
1824-
// However, there are a few cases where additional version specifiers can be safely ignored.
1825-
let ignoreVersion: boolean = false;
1826-
1827-
// 1. If the version specifier was listed in "allowedAlternativeVersions", then it's never a candidate.
1828-
// (Even if it's the only version specifier anywhere in the repo, we still ignore it, because
1829-
// otherwise the rule would be difficult to explain.)
1830-
if (alternativesForThisDependency.indexOf(dependency.version) > 0) {
1831-
ignoreVersion = true;
1832-
} else {
1833-
// Is it a local project?
1834-
const localProject: RushConfigurationProject | undefined = this.getProjectByName(dependency.name);
1835-
if (localProject) {
1836-
// 2. If it's a symlinked local project, then it's not a candidate, because the package manager will
1837-
// never even see it.
1838-
// However there are two ways that a local project can NOT be symlinked:
1839-
// - if the local project doesn't satisfy the referenced semver specifier; OR
1840-
// - if the local project was specified in "cyclicDependencyProjects" in rush.json
1841-
if (
1842-
!cyclicDependencies.has(dependency.name) &&
1843-
semver.satisfies(localProject.packageJsonEditor.version, dependency.version)
1844-
) {
1845-
ignoreVersion = true;
1799+
/**
1800+
* @internal
1801+
*/
1802+
public _getAllDependencyVersions(variant?: string | undefined): Map<string, Set<string>> {
1803+
if (!this._allDependencyVersions) {
1804+
this._allDependencyVersions = new Map();
1805+
}
1806+
1807+
const variantKey: string = variant || '';
1808+
let allDependencyVersions: Map<string, Set<string>> | undefined =
1809+
this._allDependencyVersions.get(variantKey);
1810+
if (!allDependencyVersions) {
1811+
allDependencyVersions = new Map();
1812+
1813+
const commonVersions: CommonVersionsConfiguration = this.getCommonVersions(variant);
1814+
const allowedAlternativeVersions: Map<
1815+
string,
1816+
ReadonlyArray<string>
1817+
> = commonVersions.allowedAlternativeVersions;
1818+
for (const project of this.projects) {
1819+
const dependencies: PackageJsonDependency[] = [
1820+
...project.packageJsonEditor.dependencyList,
1821+
...project.packageJsonEditor.devDependencyList
1822+
];
1823+
for (const { name: dependencyName, version: dependencyVersion, dependencyType } of dependencies) {
1824+
if (dependencyType === DependencyType.Peer) {
1825+
// If this is a peer dependency, it isn't a real dependency in this context, so it shouldn't
1826+
// be included in the list of dependency versions.
1827+
continue;
18461828
}
1847-
}
18481829

1849-
if (!ignoreVersion) {
1850-
let versionForDependency: Set<string> | undefined = versionsForDependencies.get(dependency.name);
1851-
if (!versionForDependency) {
1852-
versionForDependency = new Set<string>();
1853-
versionsForDependencies.set(dependency.name, versionForDependency);
1830+
const alternativesForThisDependency: ReadonlyArray<string> =
1831+
allowedAlternativeVersions.get(dependencyName) || [];
1832+
1833+
// For each dependency, collectImplicitlyPreferredVersions() is collecting the set of all version specifiers
1834+
// that appear across the repo. If there is only one version specifier, then that's the "preferred" one.
1835+
// However, there are a few cases where additional version specifiers can be safely ignored.
1836+
let ignoreVersion: boolean = false;
1837+
1838+
// 1. If the version specifier was listed in "allowedAlternativeVersions", then it's never a candidate.
1839+
// (Even if it's the only version specifier anywhere in the repo, we still ignore it, because
1840+
// otherwise the rule would be difficult to explain.)
1841+
if (alternativesForThisDependency.indexOf(dependencyVersion) > 0) {
1842+
ignoreVersion = true;
1843+
} else {
1844+
// Is it a local project?
1845+
const localProject: RushConfigurationProject | undefined = this.getProjectByName(dependencyName);
1846+
if (localProject) {
1847+
// 2. If it's a symlinked local project, then it's not a candidate, because the package manager will
1848+
// never even see it.
1849+
// However there are two ways that a local project can NOT be symlinked:
1850+
// - if the local project doesn't satisfy the referenced semver specifier; OR
1851+
// - if the local project was specified in "cyclicDependencyProjects" in rush.json
1852+
if (
1853+
!project.cyclicDependencyProjects.has(dependencyName) &&
1854+
semver.satisfies(localProject.packageJsonEditor.version, dependencyVersion)
1855+
) {
1856+
ignoreVersion = true;
1857+
}
1858+
}
1859+
1860+
if (!ignoreVersion) {
1861+
let versionForDependency: Set<string> | undefined = allDependencyVersions.get(dependencyName);
1862+
if (!versionForDependency) {
1863+
versionForDependency = new Set<string>();
1864+
allDependencyVersions.set(dependencyName, versionForDependency);
1865+
}
1866+
versionForDependency.add(dependencyVersion);
1867+
}
18541868
}
1855-
versionForDependency.add(dependency.version);
18561869
}
18571870
}
1871+
1872+
this._allDependencyVersions.set(variantKey, allDependencyVersions);
18581873
}
1874+
1875+
return allDependencyVersions;
18591876
}
18601877

18611878
private _getVariantConfigFolderPath(variant?: string | undefined): string {

‎apps/rush-lib/src/cli/actions/AddAction.ts

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,14 @@
33

44
import * as os from 'os';
55
import * as semver from 'semver';
6-
import { Import } from '@rushstack/node-core-library';
76
import { CommandLineFlagParameter, CommandLineStringListParameter } from '@rushstack/ts-command-line';
87

98
import { RushConfigurationProject } from '../../api/RushConfigurationProject';
109
import { BaseRushAction } from './BaseRushAction';
1110
import { RushCommandLineParser } from '../RushCommandLineParser';
1211
import { DependencySpecifier } from '../../logic/DependencySpecifier';
1312

14-
import type * as PackageJsonUpdaterTypes from '../../logic/PackageJsonUpdater';
15-
const packageJsonUpdaterModule: typeof PackageJsonUpdaterTypes = Import.lazy(
16-
'../../logic/PackageJsonUpdater',
17-
require
18-
);
13+
import type * as PackageJsonUpdaterType from '../../logic/PackageJsonUpdater';
1914

2015
export class AddAction extends BaseRushAction {
2116
private _allFlag!: CommandLineFlagParameter;
@@ -117,10 +112,10 @@ export class AddAction extends BaseRushAction {
117112
);
118113
}
119114

115+
const packageJsonUpdater: typeof PackageJsonUpdaterType = await import('../../logic/PackageJsonUpdater');
116+
120117
const specifiedPackageNameList: ReadonlyArray<string> = this._packageNameList.values!;
121-
const packageNames: string[] = [];
122-
const initialVersions: Map<string, string | undefined> = new Map();
123-
const rangeStyles: Map<string, PackageJsonUpdaterTypes.SemVerStyle> = new Map();
118+
const packagesToAdd: PackageJsonUpdaterType.IPackageForRushAdd[] = [];
124119

125120
for (const specifiedPackageName of specifiedPackageNameList) {
126121
/**
@@ -149,13 +144,11 @@ export class AddAction extends BaseRushAction {
149144
throw new Error(`The SemVer specifier "${version}" is not valid.`);
150145
}
151146
}
152-
packageNames.push(packageName);
153-
initialVersions.set(packageName, version);
154147

155148
/**
156149
* RangeStyle
157150
*/
158-
let rangeStyle: PackageJsonUpdaterTypes.SemVerStyle;
151+
let rangeStyle: PackageJsonUpdaterType.SemVerStyle;
159152
if (version && version !== 'latest') {
160153
if (this._exactFlag.value || this._caretFlag.value) {
161154
throw new Error(
@@ -164,29 +157,30 @@ export class AddAction extends BaseRushAction {
164157
);
165158
}
166159

167-
rangeStyle = packageJsonUpdaterModule.SemVerStyle.Passthrough;
160+
rangeStyle = packageJsonUpdater.SemVerStyle.Passthrough;
168161
} else {
169162
rangeStyle = this._caretFlag.value
170-
? packageJsonUpdaterModule.SemVerStyle.Caret
163+
? packageJsonUpdater.SemVerStyle.Caret
171164
: this._exactFlag.value
172-
? packageJsonUpdaterModule.SemVerStyle.Exact
173-
: packageJsonUpdaterModule.SemVerStyle.Tilde;
165+
? packageJsonUpdater.SemVerStyle.Exact
166+
: packageJsonUpdater.SemVerStyle.Tilde;
174167
}
175-
rangeStyles.set(packageName, rangeStyle);
168+
169+
packagesToAdd.push({ packageName, version, rangeStyle });
176170
}
177171

178-
const updater: PackageJsonUpdaterTypes.PackageJsonUpdater =
179-
new packageJsonUpdaterModule.PackageJsonUpdater(this.rushConfiguration, this.rushGlobalFolder);
172+
const updater: PackageJsonUpdaterType.PackageJsonUpdater = new packageJsonUpdater.PackageJsonUpdater(
173+
this.rushConfiguration,
174+
this.rushGlobalFolder
175+
);
180176

181177
await updater.doRushAdd({
182178
projects: projects,
183-
packageNames,
184-
initialVersions,
179+
packagesToAdd,
185180
devDependency: this._devDependencyFlag.value,
186181
updateOtherPackages: this._makeConsistentFlag.value,
187182
skipUpdate: this._skipUpdateFlag.value,
188-
debugInstall: this.parser.isDebug,
189-
rangeStyles
183+
debugInstall: this.parser.isDebug
190184
});
191185
}
192186
}

‎apps/rush-lib/src/cli/actions/test/AddAction.test.ts

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
import '../../test/mockRushCommandLineParser';
55

6-
import { PackageJsonUpdater } from '../../../logic/PackageJsonUpdater';
6+
import { IPackageJsonUpdaterRushAddOptions, PackageJsonUpdater } from '../../../logic/PackageJsonUpdater';
77
import { RushCommandLineParser } from '../../RushCommandLineParser';
88
import { AddAction } from '../AddAction';
99

@@ -47,9 +47,18 @@ describe(AddAction.name, () => {
4747

4848
await expect(parser.execute()).resolves.toEqual(true);
4949
expect(doRushAddMock).toHaveBeenCalledTimes(1);
50-
expect(doRushAddMock.mock.calls[0][0].projects).toHaveLength(1);
51-
expect(doRushAddMock.mock.calls[0][0].projects[0].packageName).toEqual('a');
52-
expect(doRushAddMock.mock.calls[0][0].packageNames).toContain('assert');
50+
const doRushAddOptions: IPackageJsonUpdaterRushAddOptions = doRushAddMock.mock.calls[0][0];
51+
expect(doRushAddOptions.projects).toHaveLength(1);
52+
expect(doRushAddOptions.projects[0].packageName).toEqual('a');
53+
expect(doRushAddOptions.packagesToAdd).toMatchInlineSnapshot(`
54+
Array [
55+
Object {
56+
"packageName": "assert",
57+
"rangeStyle": "tilde",
58+
"version": undefined,
59+
},
60+
]
61+
`);
5362
});
5463
});
5564

@@ -72,10 +81,19 @@ describe(AddAction.name, () => {
7281

7382
await expect(parser.execute()).resolves.toEqual(true);
7483
expect(doRushAddMock).toHaveBeenCalledTimes(1);
75-
expect(doRushAddMock.mock.calls[0][0].projects).toHaveLength(2);
76-
expect(doRushAddMock.mock.calls[0][0].projects[0].packageName).toEqual('a');
77-
expect(doRushAddMock.mock.calls[0][0].projects[1].packageName).toEqual('b');
78-
expect(doRushAddMock.mock.calls[0][0].packageNames).toContain('assert');
84+
const doRushAddOptions: IPackageJsonUpdaterRushAddOptions = doRushAddMock.mock.calls[0][0];
85+
expect(doRushAddOptions.projects).toHaveLength(2);
86+
expect(doRushAddOptions.projects[0].packageName).toEqual('a');
87+
expect(doRushAddOptions.projects[1].packageName).toEqual('b');
88+
expect(doRushAddOptions.packagesToAdd).toMatchInlineSnapshot(`
89+
Array [
90+
Object {
91+
"packageName": "assert",
92+
"rangeStyle": "tilde",
93+
"version": undefined,
94+
},
95+
]
96+
`);
7997
});
8098
});
8199
});

0 commit comments

Comments
 (0)
0