Skip to content

Commit dbe7f77

Browse files
pgonzalpgonzal
authored
Merge pull request microsoft#902 from Microsoft/pgonzal/yarn-bug
[rush] Fix issue where "rush install" sometimes would incorrectly ask for "rush update" when using Yarn
2 parents e3af166 + 43aea24 commit dbe7f77

9 files changed

Lines changed: 215 additions & 33 deletions

File tree

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

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ import * as semver from 'semver';
55

66
import {
77
IPackageJson,
8-
JsonFile
8+
JsonFile,
9+
Sort
910
} from '@microsoft/node-core-library';
1011

1112
/**
@@ -93,10 +94,16 @@ export class PackageJsonEditor {
9394
return this._filePath;
9495
}
9596

97+
/**
98+
* The list of dependencies of type DependencyType.Regular, DependencyType.Optional, or DependencyType.Peer.
99+
*/
96100
public get dependencyList(): ReadonlyArray<PackageJsonDependency> {
97101
return [...this._dependencies.values()];
98102
}
99103

104+
/**
105+
* The list of dependencies of type DependencyType.Dev.
106+
*/
100107
public get devDependencyList(): ReadonlyArray<PackageJsonDependency> {
101108
return [...this._devDependencies.values()];
102109
}
@@ -148,11 +155,11 @@ export class PackageJsonEditor {
148155

149156
try {
150157
Object.keys(dependencies || {}).forEach((packageName: string) => {
151-
if (optionalDependencies[packageName]) {
158+
if (Object.prototype.hasOwnProperty.call(optionalDependencies, packageName)) {
152159
throw new Error(`The package "${packageName}" cannot be listed in both `
153160
+ `"dependencies" and "optionalDependencies"`);
154161
}
155-
if (peerDependencies[packageName]) {
162+
if (Object.prototype.hasOwnProperty.call(peerDependencies, packageName)) {
156163
throw new Error(`The package "${packageName}" cannot be listed in both `
157164
+ `"dependencies" and "peerDependencies"`);
158165
}
@@ -162,7 +169,7 @@ export class PackageJsonEditor {
162169
});
163170

164171
Object.keys(optionalDependencies || {}).forEach((packageName: string) => {
165-
if (peerDependencies[packageName]) {
172+
if (Object.prototype.hasOwnProperty.call(peerDependencies, packageName)) {
166173
throw new Error(`The package "${packageName}" cannot be listed in both `
167174
+ `"optionalDependencies" and "peerDependencies"`);
168175
}
@@ -181,6 +188,9 @@ export class PackageJsonEditor {
181188
new PackageJsonDependency(packageName, devDependencies[packageName], DependencyType.Dev, _onChange));
182189
});
183190

191+
Sort.sortMapKeys(this._dependencies);
192+
Sort.sortMapKeys(this._devDependencies);
193+
184194
} catch (e) {
185195
throw new Error(`Error loading "${filepath}": ${e.message}`);
186196
}
@@ -236,4 +246,4 @@ export class PackageJsonEditor {
236246

237247
return this._data;
238248
}
239-
}
249+
}

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

Lines changed: 30 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@ import {
1818
IPackageJson,
1919
MapExtensions,
2020
FileSystem,
21-
FileConstants
21+
FileConstants,
22+
Sort
2223
} from '@microsoft/node-core-library';
2324

2425
import { ApprovedPackagesChecker } from '../logic/ApprovedPackagesChecker';
@@ -111,7 +112,8 @@ export class InstallManager {
111112
private _commonTempFolderRecycler: AsyncRecycler;
112113

113114
/**
114-
* Returns a map of all direct dependencies that only have a single semantic version specifier
115+
* Returns a map of all direct dependencies that only have a single semantic version specifier.
116+
* Returns a map: dependency name --> version specifier
115117
*/
116118
public static collectImplicitlyPreferredVersions(rushConfiguration: RushConfiguration): Map<string, string> {
117119
// First, collect all the direct dependencies of all local projects, and their versions:
@@ -381,6 +383,7 @@ export class InstallManager {
381383
shrinkwrapIsUpToDate = false;
382384
}
383385

386+
// dependency name --> version specifier
384387
const allExplicitPreferredVersions: Map<string, string> = this._rushConfiguration.commonVersions
385388
.getAllPreferredVersions();
386389

@@ -429,6 +432,8 @@ export class InstallManager {
429432
// Find the implicitly preferred versions
430433
// These are any first-level dependencies for which we only consume a single version range
431434
// (e.g. every package that depends on react uses an identical specifier)
435+
436+
// dependency name --> version specifier
432437
const allPreferredVersions: Map<string, string> =
433438
InstallManager.collectImplicitlyPreferredVersions(this._rushConfiguration);
434439

@@ -445,9 +450,7 @@ export class InstallManager {
445450
// To make the common/package.json file more readable, sort alphabetically
446451
// according to rushProject.tempProjectName instead of packageName.
447452
const sortedRushProjects: RushConfigurationProject[] = this._rushConfiguration.projects.slice(0);
448-
sortedRushProjects.sort(
449-
(a: RushConfigurationProject, b: RushConfigurationProject) => a.tempProjectName.localeCompare(b.tempProjectName)
450-
);
453+
Sort.sortBy(sortedRushProjects, x => x.tempProjectName);
451454

452455
for (const rushProject of sortedRushProjects) {
453456
const packageJson: PackageJsonEditor = rushProject.packageJsonEditor;
@@ -469,65 +472,66 @@ export class InstallManager {
469472
dependencies: {}
470473
};
471474

472-
// Collect pairs of (packageName, packageVersion) to be added as temp package dependencies
473-
const pairs: { packageName: string, packageVersion: string }[] = [];
475+
// Collect pairs of (packageName, packageVersion) to be added as dependencies of the @rush-temp package.json
476+
const tempDependencies: Map<string, string> = new Map<string, string>();
474477

478+
// These can be regular, optional, or peer dependencies (but NOT dev dependencies).
479+
// (A given packageName will never appear more than once in this list.)
475480
for (const dependency of packageJson.dependencyList) {
476481

477-
// If there are any optional dependencies, copy them over directly
482+
// If there are any optional dependencies, copy directly into the optionalDependencies field.
478483
if (dependency.dependencyType === DependencyType.Optional) {
479484
if (!tempPackageJson.optionalDependencies) {
480485
tempPackageJson.optionalDependencies = {};
481486
}
482487
tempPackageJson.optionalDependencies[dependency.name] = dependency.version;
483488
} else {
484-
pairs.push({ packageName: dependency.name, packageVersion: dependency.version });
489+
tempDependencies.set(dependency.name, dependency.version);
485490
}
486491
}
487492

488493
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 });
494+
// If there are devDependencies, we need to merge them with the regular dependencies. If the same
495+
// library appears in both places, then the dev dependency wins (because presumably it's saying what you
496+
// want right now for development, not the range that you support for consumers).
497+
tempDependencies.set(dependency.name, dependency.version);
496498
}
499+
Sort.sortMapKeys(tempDependencies);
497500

498-
for (const pair of pairs) {
501+
for (const [packageName, packageVersion] of tempDependencies.entries()) {
499502
// Is there a locally built Rush project that could satisfy this dependency?
500503
// If so, then we will symlink to the project folder rather than to common/temp/node_modules.
501504
// In this case, we don't want "npm install" to process this package, but we do need
502505
// to record this decision for "rush link" later, so we add it to a special 'rushDependencies' field.
503506
const localProject: RushConfigurationProject | undefined =
504-
this._rushConfiguration.getProjectByName(pair.packageName);
505-
if (localProject) {
507+
this._rushConfiguration.getProjectByName(packageName);
506508

509+
if (localProject) {
507510
// Don't locally link if it's listed in the cyclicDependencyProjects
508-
if (!rushProject.cyclicDependencyProjects.has(pair.packageName)) {
511+
if (!rushProject.cyclicDependencyProjects.has(packageName)) {
509512

510513
// Also, don't locally link if the SemVer doesn't match
511514
const localProjectVersion: string = localProject.packageJsonEditor.version;
512-
if (semver.satisfies(localProjectVersion, pair.packageVersion)) {
515+
if (semver.satisfies(localProjectVersion, packageVersion)) {
513516

514-
// We will locally link this package
517+
// We will locally link this package, so instead add it to our special "rushDependencies"
518+
// field in the package.json file.
515519
if (!tempPackageJson.rushDependencies) {
516520
tempPackageJson.rushDependencies = {};
517521
}
518-
tempPackageJson.rushDependencies[pair.packageName] = pair.packageVersion;
522+
tempPackageJson.rushDependencies[packageName] = packageVersion;
519523
continue;
520524
}
521525
}
522526
}
523527

524528
// We will NOT locally link this package; add it as a regular dependency.
525-
tempPackageJson.dependencies![pair.packageName] = pair.packageVersion;
529+
tempPackageJson.dependencies![packageName] = packageVersion;
526530

527531
if (shrinkwrapFile) {
528-
if (!shrinkwrapFile.tryEnsureCompatibleDependency(pair.packageName, pair.packageVersion,
532+
if (!shrinkwrapFile.tryEnsureCompatibleDependency(packageName, packageVersion,
529533
rushProject.tempProjectName)) {
530-
shrinkwrapWarnings.push(`"${pair.packageName}" (${pair.packageVersion}) required by`
534+
shrinkwrapWarnings.push(`"${packageName}" (${packageVersion}) required by`
531535
+ ` "${rushProject.packageName}"`);
532536
shrinkwrapIsUpToDate = false;
533537
}

‎common/changes/@microsoft/node-core-library/pgonzal-yarn-bug_2018-10-25-07-31.json‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
{
2+
"changes": [
3+
{
4+
"packageName": "@microsoft/node-core-library",
5+
"comment": "Add Sort API",
6+
"type": "minor"
7+
}
8+
],
9+
"packageName": "@microsoft/node-core-library",
10+
"email": "pgonzal@users.noreply.github.com"
11+
}

‎common/changes/@microsoft/rush/pgonzal-yarn-bug_2018-10-25-03-42.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 \"rush install\" sometimes would incorrectly ask for \"rush update\", when using the Yarn package manager",
5+
"packageName": "@microsoft/rush",
6+
"type": "none"
7+
}
8+
],
9+
"packageName": "@microsoft/rush",
10+
"email": "pgonzal@users.noreply.github.com"
11+
}

‎common/changes/@microsoft/rush/pgonzal-yarn-bug_2018-10-25-07-31.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": "Improve sorting of @rush-temp projects, which may reduce churn of hashes in the shrinkwrap file",
5+
"packageName": "@microsoft/rush",
6+
"type": "none"
7+
}
8+
],
9+
"packageName": "@microsoft/rush",
10+
"email": "pgonzal@users.noreply.github.com"
11+
}

‎common/reviews/api/node-core-library.api.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,15 @@ class ProtectableMap<K, V> {
397397
readonly size: number;
398398
}
399399

400+
// @public
401+
class Sort {
402+
static compareByValue(x: any, y: any): number;
403+
static isSorted<T>(array: T[], comparer?: (x: any, y: any) => number): boolean;
404+
static isSortedBy<T>(array: T[], keySelector: (element: T) => any, comparer?: (x: any, y: any) => number): boolean;
405+
static sortBy<T>(array: T[], keySelector: (element: T) => any, comparer?: (x: any, y: any) => number): void;
406+
static sortMapKeys<K, V>(map: Map<K, V>, keyComparer?: (x: K, y: K) => number): void;
407+
}
408+
400409
// @beta
401410
class StringBuilder {
402411
constructor();

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,7 @@ class PackageJsonDependency {
154154
class PackageJsonEditor {
155155
// (undocumented)
156156
addOrUpdateDependency(packageName: string, newVersion: string, dependencyType: DependencyType): void;
157-
// (undocumented)
158157
readonly dependencyList: ReadonlyArray<PackageJsonDependency>;
159-
// (undocumented)
160158
readonly devDependencyList: ReadonlyArray<PackageJsonDependency>;
161159
// (undocumented)
162160
readonly filePath: string;

‎libraries/node-core-library/src/Sort.ts‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
// Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT license.
2+
// See LICENSE in the project root for license information.
3+
4+
/**
5+
* Operations for sorting collections.
6+
*
7+
* @public
8+
*/
9+
export class Sort {
10+
/**
11+
* Compares `x` and `y` using the JavaScript `>` and `<` operators. This function is suitable for usage as
12+
* the callback for `array.sort()`.
13+
* @returns -1 if `x` is smaller than `y`, 1 if `x` is greater than `y`, or 0 if the values are equal.
14+
*
15+
* @example
16+
*
17+
* ```ts
18+
* let array: number[] = [3, 6, 2];
19+
* array.sort(Sort.compareByValue); // [2, 3, 6]
20+
* ```
21+
*/
22+
// tslint:disable-next-line:no-any
23+
public static compareByValue(x: any, y: any): number {
24+
if (x === y) {
25+
return 0;
26+
}
27+
if (x === undefined) {
28+
return -1;
29+
}
30+
if (x === null) {
31+
return -1;
32+
}
33+
if (x < y) {
34+
return -1;
35+
}
36+
if (x > y) {
37+
return 1;
38+
}
39+
return 0;
40+
}
41+
42+
/**
43+
* Sorts the array according to a key which is obtained from the array elements.
44+
*
45+
* @example
46+
*
47+
* ```ts
48+
* let array: string[] = [ 'ccc', 'bb', 'a' ];
49+
* Sort.sortBy(array, x => x.length); // [ 'a', 'bb', 'ccc' ]
50+
* ```
51+
*/
52+
// tslint:disable-next-line:no-any
53+
public static sortBy<T>(array: T[], keySelector: (element: T) => any, comparer: (x: any, y: any) => number
54+
= Sort.compareByValue): void {
55+
array.sort((x, y) => comparer(keySelector(x), keySelector(y)));
56+
}
57+
58+
/**
59+
* Returns true if the array is already sorted.
60+
*/
61+
// tslint:disable-next-line:no-any
62+
public static isSorted<T>(array: T[], comparer: (x: any, y: any) => number = Sort.compareByValue): boolean {
63+
let previous: T | undefined = undefined;
64+
for (const element of array) {
65+
if (comparer(previous, element) > 0) {
66+
return false;
67+
}
68+
previous = element;
69+
}
70+
return true;
71+
}
72+
73+
/**
74+
* Returns true if the array is already sorted by the specified key.
75+
*
76+
* @example
77+
*
78+
* ```ts
79+
* let array: string[] = [ 'a', 'bb', 'ccc' ];
80+
* Sort.isSortedBy(array, x => x.length); // true
81+
* ```
82+
*/
83+
// tslint:disable-next-line:no-any
84+
public static isSortedBy<T>(array: T[], keySelector: (element: T) => any, comparer: (x: any, y: any) => number
85+
= Sort.compareByValue): boolean {
86+
87+
let previousKey: T | undefined = undefined;
88+
for (const element of array) {
89+
const key: T = keySelector(element);
90+
if (comparer(previousKey, key) > 0) {
91+
return false;
92+
}
93+
previousKey = key;
94+
}
95+
return true;
96+
}
97+
98+
/**
99+
* Sorts the entries in a Map object according to the keys.
100+
*
101+
* @example
102+
*
103+
* ```ts
104+
* let map: Map<string, number> = new Map<string, number>();
105+
* map.set('zebra', 1);
106+
* map.set('goose', 2);
107+
* map.set('aardvark', 3);
108+
* Sort.sortMapKeys(map);
109+
* console.log(JSON.stringify(Array.from(map.keys()))); // ["aardvark","goose","zebra"]
110+
* ```
111+
*/
112+
// tslint:disable-next-line:no-any
113+
public static sortMapKeys<K, V>(map: Map<K, V>, keyComparer: (x: K, y: K) => number = Sort.compareByValue): void {
114+
const pairs: [K, V][] = Array.from(map.entries());
115+
116+
// Sorting a map is expensive, so first check whether it's already sorted.
117+
if (Sort.isSortedBy(pairs, x => x[0], keyComparer)) {
118+
return;
119+
}
120+
121+
Sort.sortBy(pairs, x => x[0], keyComparer);
122+
map.clear();
123+
for (const pair of pairs) {
124+
map.set(pair[0], pair[1]);
125+
}
126+
}
127+
}

0 commit comments

Comments
 (0)