Skip to content

Commit 4f20905

Browse files
author
Nicholas Pape
authored
Rush: fix an issue where after running "rush add", "rush update" was not detecting changes (microsoft#853)
* Update InstallManager to use the new PackageJsonEditor API * Changefile * Dont use packageJson API in LinkManager * Rename changefile * Publish as patch
1 parent ea79bda commit 4f20905

File tree

7 files changed

+80
-64
lines changed

7 files changed

+80
-64
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,11 +93,11 @@ export class PackageJsonEditor {
9393
return this._filePath;
9494
}
9595

96-
public get dependencyList(): Array<PackageJsonDependency> {
96+
public get dependencyList(): ReadonlyArray<PackageJsonDependency> {
9797
return [...this._dependencies.values()];
9898
}
9999

100-
public get devDependencyList(): Array<PackageJsonDependency> {
100+
public get devDependencyList(): ReadonlyArray<PackageJsonDependency> {
101101
return [...this._devDependencies.values()];
102102
}
103103

‎apps/rush-lib/src/logic/InstallManager.ts‎

Lines changed: 60 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import { ShrinkwrapFileFactory } from '../logic/ShrinkwrapFileFactory';
3737
import { Stopwatch } from '../utilities/Stopwatch';
3838
import { Utilities } from '../utilities/Utilities';
3939
import { Rush } from '../api/Rush';
40+
import { PackageJsonEditor, DependencyType, PackageJsonDependency } from '../api/PackageJsonEditor';
4041
import { AlreadyReportedError } from '../utilities/AlreadyReportedError';
4142

4243
const MAX_INSTALL_ATTEMPTS: number = 2;
@@ -46,6 +47,7 @@ const MAX_INSTALL_ATTEMPTS: number = 2;
4647
* As a temporary workaround, augment the type.
4748
*/
4849
import { CreateOptions } from 'tar';
50+
4951
export interface CreateOptions { // tslint:disable-line:interface-name
5052
/**
5153
* "Set to true to omit writing mtime values for entries. Note that this prevents using other
@@ -117,10 +119,15 @@ export class InstallManager {
117119
const versionsForDependencies: Map<string, Set<string>> = new Map<string, Set<string>>();
118120

119121
rushConfiguration.projects.forEach((project: RushConfigurationProject) => {
120-
InstallManager._collectVersionsForDependencies(versionsForDependencies, project.packageJson.dependencies,
121-
project.cyclicDependencyProjects, rushConfiguration);
122-
InstallManager._collectVersionsForDependencies(versionsForDependencies, project.packageJson.devDependencies,
123-
project.cyclicDependencyProjects, rushConfiguration);
122+
InstallManager._collectVersionsForDependencies(versionsForDependencies,
123+
project.packageJsonEditor.dependencyList,
124+
project.cyclicDependencyProjects,
125+
rushConfiguration);
126+
127+
InstallManager._collectVersionsForDependencies(versionsForDependencies,
128+
project.packageJsonEditor.devDependencyList,
129+
project.cyclicDependencyProjects,
130+
rushConfiguration);
124131
});
125132

126133
// If any dependency has more than one version, then filter it out (since we don't know which version
@@ -147,47 +154,45 @@ export class InstallManager {
147154

148155
// Helper for collectImplicitlyPreferredVersions()
149156
private static _collectVersionsForDependencies(versionsForDependencies: Map<string, Set<string>>,
150-
dependencies: { [dep: string]: string } | undefined,
157+
dependencies: ReadonlyArray<PackageJsonDependency>,
151158
cyclicDependencies: Set<string>, rushConfiguration: RushConfiguration): void {
152159

153160
const allowedAlternativeVersions: Map<string, ReadonlyArray<string>>
154161
= rushConfiguration.commonVersions.allowedAlternativeVersions;
155162

156-
if (dependencies) {
157-
Object.keys(dependencies).forEach((dependency: string) => {
158-
const versionSpecifier: string = dependencies[dependency];
159-
const alternativesForThisDependency: ReadonlyArray<string> = allowedAlternativeVersions.get(dependency) || [];
160-
161-
// For each dependency, collectImplicitlyPreferredVersions() is collecting the set of all version specifiers
162-
// that appear across the repo. If there is only one version specifier, then that's the "preferred" one.
163-
// However, there are a few cases where additional version specifiers can be safely ignored.
164-
let ignoreVersion: boolean = false;
165-
166-
// 1. If the version specifier was listed in "allowedAlternativeVersions", then it's never a candidate.
167-
// (Even if it's the only version specifier anywhere in the repo, we still ignore it, because
168-
// otherwise the rule would be difficult to explain.)
169-
if (alternativesForThisDependency.indexOf(versionSpecifier) > 0) {
170-
ignoreVersion = true;
171-
} else {
172-
// Is it a local project?
173-
const localProject: RushConfigurationProject | undefined = rushConfiguration.getProjectByName(dependency);
174-
if (localProject) {
175-
// 2. If it's a symlinked local project, then it's not a candidate, because the package manager will
176-
// never even see it.
177-
// However there are two ways that a local project can NOT be symlinked:
178-
// - if the local project doesn't satisfy the referenced semver specifier; OR
179-
// - if the local project was specified in "cyclicDependencyProjects" in rush.json
180-
if (semver.satisfies(localProject.packageJson.version, versionSpecifier)
181-
&& !cyclicDependencies.has(dependency)) {
182-
ignoreVersion = true;
183-
}
163+
for (const dependency of dependencies) {
164+
const alternativesForThisDependency: ReadonlyArray<string>
165+
= allowedAlternativeVersions.get(dependency.name) || [];
166+
167+
// For each dependency, collectImplicitlyPreferredVersions() is collecting the set of all version specifiers
168+
// that appear across the repo. If there is only one version specifier, then that's the "preferred" one.
169+
// However, there are a few cases where additional version specifiers can be safely ignored.
170+
let ignoreVersion: boolean = false;
171+
172+
// 1. If the version specifier was listed in "allowedAlternativeVersions", then it's never a candidate.
173+
// (Even if it's the only version specifier anywhere in the repo, we still ignore it, because
174+
// otherwise the rule would be difficult to explain.)
175+
if (alternativesForThisDependency.indexOf(dependency.version) > 0) {
176+
ignoreVersion = true;
177+
} else {
178+
// Is it a local project?
179+
const localProject: RushConfigurationProject | undefined = rushConfiguration.getProjectByName(dependency.name);
180+
if (localProject) {
181+
// 2. If it's a symlinked local project, then it's not a candidate, because the package manager will
182+
// never even see it.
183+
// However there are two ways that a local project can NOT be symlinked:
184+
// - if the local project doesn't satisfy the referenced semver specifier; OR
185+
// - if the local project was specified in "cyclicDependencyProjects" in rush.json
186+
if (semver.satisfies(localProject.packageJsonEditor.version, dependency.version)
187+
&& !cyclicDependencies.has(dependency.name)) {
188+
ignoreVersion = true;
184189
}
185190
}
186191

187192
if (!ignoreVersion) {
188-
InstallManager._updateVersionsForDependencies(versionsForDependencies, dependency, versionSpecifier);
193+
InstallManager._updateVersionsForDependencies(versionsForDependencies, dependency.name, dependency.version);
189194
}
190-
});
195+
}
191196
}
192197
}
193198

@@ -445,7 +450,7 @@ export class InstallManager {
445450
);
446451

447452
for (const rushProject of sortedRushProjects) {
448-
const packageJson: IPackageJson = rushProject.packageJson;
453+
const packageJson: PackageJsonEditor = rushProject.packageJsonEditor;
449454

450455
// Example: "C:\MyRepo\common\temp\projects\my-project-2.tgz"
451456
const tarballFile: string = this._getTarballFilePath(rushProject);
@@ -464,30 +469,30 @@ export class InstallManager {
464469
dependencies: {}
465470
};
466471

467-
// If there are any optional dependencies, copy them over directly
468-
if (packageJson.optionalDependencies) {
469-
tempPackageJson.optionalDependencies = packageJson.optionalDependencies;
470-
}
471-
472472
// Collect pairs of (packageName, packageVersion) to be added as temp package dependencies
473473
const pairs: { packageName: string, packageVersion: string }[] = [];
474474

475-
// If there are devDependencies, we need to merge them with the regular
476-
// dependencies. If the same library appears in both places, then the
477-
// regular dependency takes precedence over the devDependency.
478-
// It also takes precedence over a duplicate in optionalDependencies,
479-
// but NPM will take care of that for us. (Frankly any kind of duplicate
480-
// should be an error, but NPM is pretty lax about this.)
481-
if (packageJson.devDependencies) {
482-
for (const packageName of Object.keys(packageJson.devDependencies)) {
483-
pairs.push({ packageName: packageName, packageVersion: packageJson.devDependencies[packageName] });
475+
for (const dependency of packageJson.dependencyList) {
476+
477+
// If there are any optional dependencies, copy them over directly
478+
if (dependency.dependencyType === DependencyType.Optional) {
479+
if (!tempPackageJson.optionalDependencies) {
480+
tempPackageJson.optionalDependencies = {};
481+
}
482+
tempPackageJson.optionalDependencies[dependency.name] = dependency.version;
483+
} else {
484+
pairs.push({ packageName: dependency.name, packageVersion: dependency.version });
484485
}
485486
}
486487

487-
if (packageJson.dependencies) {
488-
for (const packageName of Object.keys(packageJson.dependencies)) {
489-
pairs.push({ packageName: packageName, packageVersion: packageJson.dependencies[packageName] });
490-
}
488+
for (const dependency of packageJson.devDependencyList) {
489+
// If there are devDependencies, we need to merge them with the regular
490+
// dependencies. If the same library appears in both places, then the
491+
// regular dependency takes precedence over the devDependency.
492+
// It also takes precedence over a duplicate in optionalDependencies,
493+
// but NPM will take care of that for us. (Frankly any kind of duplicate
494+
// should be an error, but NPM is pretty lax about this.)
495+
pairs.push({ packageName: dependency.name, packageVersion: dependency.version });
491496
}
492497

493498
for (const pair of pairs) {
@@ -503,7 +508,7 @@ export class InstallManager {
503508
if (!rushProject.cyclicDependencyProjects.has(pair.packageName)) {
504509

505510
// Also, don't locally link if the SemVer doesn't match
506-
const localProjectVersion: string = localProject.packageJson.version;
511+
const localProjectVersion: string = localProject.packageJsonEditor.version;
507512
if (semver.satisfies(localProjectVersion, pair.packageVersion)) {
508513

509514
// We will locally link this package

‎apps/rush-lib/src/logic/npm/NpmLinkManager.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ export class NpmLinkManager extends BaseLinkManager {
137137

138138
// TODO: Validate that the project's package.json still matches the common folder
139139
const localProjectPackage: NpmPackage = NpmPackage.createLinkedNpmPackage(
140-
project.packageJson.name,
140+
project.packageJsonEditor.name,
141141
commonProjectPackage.version,
142142
commonProjectPackage.dependencies,
143143
project.projectFolder
@@ -180,7 +180,7 @@ export class NpmLinkManager extends BaseLinkManager {
180180
this._rushConfiguration.getProjectByName(dependency.name);
181181

182182
if (matchedRushPackage) {
183-
const matchedVersion: string = matchedRushPackage.packageJson.version;
183+
const matchedVersion: string = matchedRushPackage.packageJsonEditor.version;
184184

185185
// The dependency name matches an Rush project, but are there any other reasons not
186186
// to create a local link?

‎apps/rush-lib/src/logic/pnpm/PnpmLinkManager.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ export class PnpmLinkManager extends BaseLinkManager {
7878
const commonPackage: BasePackage = BasePackage.createVirtualTempPackage(packageJsonFilename, installFolderName);
7979

8080
const localPackage: BasePackage = BasePackage.createLinkedPackage(
81-
project.packageJson.name,
81+
project.packageJsonEditor.name,
8282
commonPackage.version,
8383
project.projectFolder
8484
);
@@ -94,7 +94,7 @@ export class PnpmLinkManager extends BaseLinkManager {
9494
if (matchedRushPackage) {
9595
// We found a suitable match, so place a new local package that
9696
// symlinks to the Rush project
97-
const matchedVersion: string = matchedRushPackage.packageJson.version;
97+
const matchedVersion: string = matchedRushPackage.packageJsonEditor.version;
9898

9999
let localLinks: string[] = rushLinkJson.localLinks[localPackage.name];
100100
if (!localLinks) {

‎common/changes/@microsoft/rush/nickpape-fix-rush-update-bug_2018-10-02-23-05.json‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
{
2+
"changes": [
3+
{
4+
"comment": "Fix an issue where after running \"rush add\" (after successfully running \"rush install\"), the new package was not being installed or linked.",
5+
"packageName": "@microsoft/rush",
6+
"type": "none"
7+
}
8+
],
9+
"packageName": "@microsoft/rush",
10+
"email": "nickpape-msft@users.noreply.github.com"
11+
}

‎common/config/rush/version-policies.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"policyName": "rush",
44
"definitionName": "lockStepVersion",
55
"version": "5.3.0",
6-
"nextBump": "minor",
6+
"nextBump": "patch",
77
"mainProject": "@microsoft/rush"
88
}
99
]

‎common/reviews/api/rush-lib.api.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,9 +155,9 @@ class PackageJsonEditor {
155155
// (undocumented)
156156
addOrUpdateDependency(packageName: string, newVersion: string, dependencyType: DependencyType): void;
157157
// (undocumented)
158-
readonly dependencyList: Array<PackageJsonDependency>;
158+
readonly dependencyList: ReadonlyArray<PackageJsonDependency>;
159159
// (undocumented)
160-
readonly devDependencyList: Array<PackageJsonDependency>;
160+
readonly devDependencyList: ReadonlyArray<PackageJsonDependency>;
161161
// (undocumented)
162162
readonly filePath: string;
163163
// (undocumented)

0 commit comments

Comments
 (0)