Skip to content

Commit 9d43f90

Browse files
bmishbmish
authored
Improve handling of local package version inconsistencies (#347)
1 parent d9b9e58 commit 9d43f90

File tree

3 files changed

+92
-37
lines changed

3 files changed

+92
-37
lines changed

‎lib/dependency-versions.ts‎

Lines changed: 52 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -119,38 +119,40 @@ export function calculateMismatchingVersions(
119119
return [];
120120
}
121121

122-
// Calculate unique versions seen for this dependency.
123-
const uniqueVersions = [
124-
...new Set(
125-
versionObjectsForDep
126-
.filter((versionObject) => !versionObject.isLocalPackageVersion)
127-
.map((versionObject) => versionObject.version)
128-
),
129-
].sort();
130-
131-
// If we saw more than one unique version for this dependency, we found an inconsistency.
132-
if (uniqueVersions.length > 1) {
133-
const uniqueVersionsWithInfo = versionsObjectsWithSortedPackages(
134-
uniqueVersions,
135-
versionObjectsForDep
136-
);
137-
return { dependency, versions: uniqueVersionsWithInfo };
138-
}
122+
// Check what versions we have seen for this dependency.
123+
let versions = versionObjectsForDep
124+
.filter((versionObject) => !versionObject.isLocalPackageVersion)
125+
.map((versionObject) => versionObject.version);
139126

140-
// If we saw a local package version that isn't compatible with the local package's actual version, we found an inconsistency.
127+
// Check if this dependency is a local package.
141128
const localPackageVersions = versionObjectsForDep
142-
.filter((object) => object.isLocalPackageVersion)
143-
.map((object) => object.version);
129+
.filter((versionObject) => versionObject.isLocalPackageVersion)
130+
.map((versionObject) => versionObject.version);
131+
144132
if (
145133
localPackageVersions.length === 1 &&
146-
uniqueVersions.length === 1 &&
147-
!semver.satisfies(localPackageVersions[0], uniqueVersions[0])
134+
versions.some(
135+
(uniqueVersion) =>
136+
!semver.satisfies(localPackageVersions[0], uniqueVersion)
137+
)
148138
) {
139+
// If we saw a version for this dependency that isn't compatible with its actual local package version, add the local package version to the list of versions seen.
140+
versions = [...versions, ...localPackageVersions];
141+
}
142+
143+
// Calculate unique versions seen for this dependency.
144+
const uniqueVersions = [...new Set(versions)].sort(compareRangesSafe);
145+
146+
// If we saw more than one unique version for this dependency, we found an inconsistency.
147+
if (uniqueVersions.length > 1) {
149148
const uniqueVersionsWithInfo = versionsObjectsWithSortedPackages(
150-
[...uniqueVersions, ...localPackageVersions],
149+
uniqueVersions,
151150
versionObjectsForDep
152151
);
153-
return { dependency, versions: uniqueVersionsWithInfo };
152+
return {
153+
dependency,
154+
versions: uniqueVersionsWithInfo,
155+
};
154156
}
155157

156158
return [];
@@ -254,7 +256,9 @@ export function fixMismatchingVersions(
254256
} {
255257
const fixed = [];
256258
const notFixed = [];
259+
// Loop through each dependency that has a mismatching versions.
257260
for (const mismatchingVersion of mismatchingVersions) {
261+
// Decide what version we should fix to.
258262
const versions = mismatchingVersion.versions.map(
259263
(object) => object.version
260264
);
@@ -267,6 +271,21 @@ export function fixMismatchingVersions(
267271
continue;
268272
}
269273

274+
// If this dependency is from a local package and the version we want to fix to is higher than the actual package version, skip it.
275+
const localPackage = packages.find(
276+
(package_) => package_.name === mismatchingVersion.dependency
277+
);
278+
if (
279+
localPackage &&
280+
localPackage.packageJson.version &&
281+
compareRanges(fixedVersion, localPackage.packageJson.version) > 0
282+
) {
283+
// Skip this dependency.
284+
notFixed.push(mismatchingVersion);
285+
continue;
286+
}
287+
288+
// Update the dependency version in each package.json.
270289
let isFixed = false;
271290
for (const package_ of packages) {
272291
if (
@@ -304,8 +323,6 @@ export function fixMismatchingVersions(
304323

305324
if (isFixed) {
306325
fixed.push(mismatchingVersion);
307-
} else {
308-
notFixed.push(mismatchingVersion);
309326
}
310327
}
311328

@@ -315,6 +332,15 @@ export function fixMismatchingVersions(
315332
};
316333
}
317334

335+
// This version doesn't throw for when we want to ignore invalid versions that might be present.
336+
export function compareRangesSafe(a: string, b: string): 0 | -1 | 1 {
337+
try {
338+
return compareRanges(a, b);
339+
} catch {
340+
return 0;
341+
}
342+
}
343+
318344
export function compareRanges(a: string, b: string): 0 | -1 | 1 {
319345
// Strip range and coerce to normalized version.
320346
const aVersion = semver.coerce(a.replace(/^[\^~]/, ''));

‎lib/output.ts‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import chalk from 'chalk';
22
import type { MismatchingDependencyVersions } from './dependency-versions.js';
3-
import { compareRanges, getLatestVersion } from './dependency-versions.js';
3+
import { compareRangesSafe, getLatestVersion } from './dependency-versions.js';
44
import { table } from 'table';
55

66
export function mismatchingVersionsToOutput(
@@ -23,13 +23,7 @@ export function mismatchingVersionsToOutput(
2323
);
2424

2525
const rows = object.versions
26-
.sort((a, b) => {
27-
try {
28-
return compareRanges(b.version, a.version);
29-
} catch {
30-
return 0;
31-
}
32-
})
26+
.sort((a, b) => compareRangesSafe(b.version, a.version))
3327
.map((versionObject) => {
3428
const usageCount = versionObject.packages.length;
3529
const packageNames = versionObject.packages.map(

‎test/lib/dependency-versions-test.ts‎

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
filterOutIgnoredDependencies,
55
fixMismatchingVersions,
66
compareRanges,
7+
compareRangesSafe,
78
getLatestVersion,
89
} from '../../lib/dependency-versions.js';
910
import { getPackages } from '../../lib/workspace.js';
@@ -572,7 +573,7 @@ describe('Utils | dependency-versions', function () {
572573
name: 'package1',
573574
version: '1.0.0',
574575
dependencies: {
575-
package2: '^1.0.0',
576+
package2: '^1.0.0', // Lower than actual version of this local package.
576577
},
577578
}),
578579
},
@@ -585,6 +586,14 @@ describe('Utils | dependency-versions', function () {
585586
},
586587
}),
587588
},
589+
package3: {
590+
'package.json': JSON.stringify({
591+
name: 'package3',
592+
dependencies: {
593+
package1: '^0.0.0', // Lower than actual version of this local package.
594+
},
595+
}),
596+
},
588597
});
589598
});
590599

@@ -611,8 +620,13 @@ describe('Utils | dependency-versions', function () {
611620
'package2/package.json',
612621
'utf-8'
613622
);
623+
const packageJson3Contents = readFileSync(
624+
'package3/package.json',
625+
'utf-8'
626+
);
614627
const packageJson1: PackageJson = JSON.parse(packageJson1Contents);
615628
const packageJson2: PackageJson = JSON.parse(packageJson2Contents);
629+
const packageJson3: PackageJson = JSON.parse(packageJson3Contents);
616630

617631
expect(
618632
packageJson1.dependencies && packageJson1.dependencies['package2']
@@ -622,19 +636,27 @@ describe('Utils | dependency-versions', function () {
622636
packageJson2.dependencies && packageJson2.dependencies['package1']
623637
).toStrictEqual('^2.0.0');
624638

639+
expect(
640+
packageJson3.dependencies && packageJson3.dependencies['package1']
641+
).toStrictEqual('^0.0.0');
642+
625643
expect(notFixed).toStrictEqual([
626644
{
627645
// Not fixed since found version higher than actual version of this local package.
628646
dependency: 'package1',
629647
versions: [
630648
{
631-
version: '^2.0.0',
632-
packages: [expect.objectContaining({ path: 'package2' })],
649+
version: '^0.0.0',
650+
packages: [expect.objectContaining({ path: 'package3' })],
633651
},
634652
{
635653
version: '1.0.0',
636654
packages: [expect.objectContaining({ path: 'package1' })],
637655
},
656+
{
657+
version: '^2.0.0',
658+
packages: [expect.objectContaining({ path: 'package2' })],
659+
},
638660
],
639661
},
640662
]);
@@ -706,6 +728,19 @@ describe('Utils | dependency-versions', function () {
706728
});
707729
});
708730

731+
describe('#compareRangesSafe', function () {
732+
it('behaves correctly', function () {
733+
expect(compareRangesSafe('1.2.3', '1.2.2')).toStrictEqual(1);
734+
expect(compareRangesSafe('4.0.0', '5.0.0')).toStrictEqual(-1);
735+
expect(compareRangesSafe('6', '6')).toStrictEqual(0);
736+
});
737+
738+
it('does not throw with invalid ranges', function () {
739+
expect(compareRangesSafe('foo', '~6.0.0')).toStrictEqual(0);
740+
expect(compareRangesSafe('~6.0.0', 'foo')).toStrictEqual(0);
741+
});
742+
});
743+
709744
describe('#getLatestVersion', function () {
710745
it('correctly chooses the higher range', function () {
711746
// Just basic sanity checks to ensure the data is passed through to `compareRanges` which has extensive tests.

0 commit comments

Comments
 (0)