[rush-lib] Fix PreferredVersions being ignored when projects have overlapping dependencies

[ace-n-msft]

Summary

Fixes #3205

This PR changes rush-lib to prefer the most-restrictive SemVer range available across both preferredVersions and Rush projects.

Details

Motivation

The repos my team works with specify some dependencies on a repo-wide basis. We want these dependencies to be included in each project and the ability to bump repo-wide dependency pins with a single file change.

Setting ensureConsistentVersions = true isn't sufficient, as (AFAICT) that would still require updating each package.json file to bump its dependency pins.

We tried to solve this by setting the individual project package.json files to SemVer ranges (e.g. 8.x or *) and providing pinned dependency versions in the preferredVersions section of common-versions.json.

In the course of testing this approach, we ran into #3205 - hence this PR.

Alternate approach: test isolation

Right now, this PR applies common/config/rush/* to all the tests in the libraries/rush-lib/src/logic/pnpm/test. This isn't causing any issues right now, but it may need to be refactored if other tests start depending on the common/config/rush directory.

If it's worth refactoring this, please let me know.
 

Alternate approach: conditional Map overwriting

For this PR, I needed a mechanism to conditionally overwrite the keys in a Map.

I was considering modifying the MapExtensions.mergeFromMap() function to accept a third parameter shouldOverwrite that would allow an end-user to control when existing keys should be overwritten.

Instead, I chose to do a "manual" merge in PnpmfileConfiguration.ts using a forEach() loop.

Possible breaking change: conflicting dependency versions across projects

According to the Implicitly Preferred Versions section in the Rush docs, Rush has no specific convention for resolving dependency conflicts between projects.

As such, I wasn't sure if I should add automated tests for that case. Please let me know if I should change this, and what those test cases might look like.

Here are a few (additional) test cases that I thought might be worth implementing:

Repo structure

# project-1/package.json
"dependencies": {
    "library-a": "^1.0.0"
}

# project-2/package.json
"dependencies": {
    "library-a": "^1.2.0"
}

Possible test case 1: should we check that this resolves to only 1.2.0

# common-versions.json
"preferredVersions": {
    "library-a": "1.2.0"
}

# expected version set
{ "library-a": "1.2.0" }

Possible test case 2: should we do anything specific here?

# common-versions.json
"preferredVersions": {
    "library-a": "1.1.0"
}

# expected version set
{ "library-a": ["1.0.0"?, "1.1.0"?, "1.2.0"] }

How it was tested

I've added automated tests for the common cases I could think of and documented some potential edge cases above.

(If those edge cases are things we should test for, I'm 100% onboard with adding automated tests for them as well.)

Impacted documentation

We should consider adding a note to this page explaining that Rush (now) prefers the most specific version of a dependency available.

[D4N14L]

Regarding the test cases you mentioned, I do believe we should add these to the test suite, they're valid scenarios. In terms of specifics for each case:

Possible test case 1: Yes, this seems like a good test case to implement. Seems like exactly the scenario you're trying to solve here.

Possible test case 2: Assuming that ensureConsistentVersions = false, then I would imagine that the expected version sets would be { "library-a": ["1.1.0", "^1.2.0"] }, since the latter range lies outside of the preferred version range of 1.1.0

[ace-n-msft]

@D4N14L thanks for the review!

Some news regarding the extra test cases - looks like there was a misunderstanding on my part. It turns out that the getImplicitlyPreferredVersions function only returns versions for packages that have a single SemVer range within the entire repository. (So, by definition, packages with multiple unique SemVer ranges - even overlapping ones! - should be excluded.)

The getImplicitlyPreferredVersions() function ultimately calls out to the DependencyAnalyzer, which uses this logic to identify implicitly-preferred versions.

If we wanted to support overlapping SemVer ranges with implicitlyPreferredVersions (the behavior I discussed in the additional test cases), we could do that by adding a step to the DependencyAnalyzer here.

---

Based on #1466, it seems like we do want that support. The changes for that might look something like this:

--- a/libraries/rush-lib/src/logic/DependencyAnalyzer.ts
+++ b/libraries/rush-lib/src/logic/DependencyAnalyzer.ts
@@ -147,10 +147,15 @@ export class DependencyAnalyzer {
             if (implicitlyPreferredVersion === undefined) {
               // There isn't a candidate for an implicitly preferred version yet. Set this value as a candidate.
               implicitlyPreferredVersion = version;
+            } else if (semver.subset(version, implicitlyPreferredVersion)) {
+              // The previous candidate is more restrictive than this value. Set the candidate to this value.
+              implicitlyPreferredVersion = version;
+            } else if (semver.subset(implicitlyPreferredVersion, version)) {
+              // This value is less restrictive than the previous candidate. Do nothing.
             } else {
-              // There was already another version that was a candidate. Clear that out and break.
+              // The current value conflicts with the candidate. Clear the candidate out and break.
               // This dependency does not have an implicitly preferred version because there are at least
-              // two candidates.
+              // two conflicting candidates.
               implicitlyPreferredVersion = undefined;
               break;
             }

Maintainers - LMK if that change is something you'd want added to this PR. 🙂

[dmichon-msft]

Maintainers - LMK if that change is something you'd want added to this PR. 🙂
I'm a fan of version deduplication via constraints; I'm also a fan of having the preferredVersion override allowedAlternativeVersions if the preferredVersion is a subset, but that might be a breaking change.

For a static validator a while back I wrote a semver-compatible range-intersection library, though I didn't have occasion to open-source it. If we end up needing that functionality I can contribute it to rushstack.

[ace-n-msft]

Circling back on this - @D4N14L I met with @octogonz and @iclanton today; they're broadly on board with this PR save for one question.

When preferredVersions and implicitPreferredVersions have overlaps, should we:

1. have implicitPreferredVersions narrow down preferredVersions' constraints, or
2. have preferredVersions overwrite any implicitlyPreferredVersions entirely?

In practice, I feel like this is a tradeoff between package freshness and disk space.

• Narrowing down preferredVersions with the implicitlyPreferredVersions ranges would allow us to reuse package versions that are already used elsewhere within the repo.
• Overwriting implicitlyPreferredVersions would (in theory) allow Rush and the underlying package manager to the newest-possible package within the preferredVersions range.

Thoughts?

[D4N14L]

@ace-n-msft Hmm. So let me write a few things out to help frame this discussion (and to be honest, for myself to re-acclimate to the question at hand 😊):

• The purpose of preferred versions is meant to be used to enforce that the same version of a package is used across the repo, which is specifically important when bundling packages and ensuring you don't pull multiple versions of a package down when (for example) browsing to a website
  • allowedAlternateVersions is used specifically to break this enforcement for a specific version range. It is intended to be an "exception" to the rule
• Implicitly preferred versions are intended to be used for the same purpose, but once a second version is specified in the repo, they can no longer be "implicitly preferred"
• Both of these are used to attempt to constrain the versions of packages consumed by internal and external (non-repo) dependencies. If the preferred version is fully within the range specified by the external dependency, the preferred version range will be substituted. Otherwise it will be left as-is. This can be seen here:

  rushstack/libraries/rush-lib/src/logic/pnpm/PnpmfileShim.ts

  Line 82 in 7e16111

  function setPreferredVersions(dependencies: { [dependencyName: string]: string } | undefined): void {

Referring to the above, the implicitlyPreferredVersion for a given package is either the existing package version across the repo or undefined. Given this, we should probably follow the existing behaviour used when replacing external package dependency versions (again, seen here:

rushstack/libraries/rush-lib/src/logic/pnpm/PnpmfileShim.ts

Line 82 in 7e16111

function setPreferredVersions(dependencies: { [dependencyName: string]: string } | undefined): void {

) and override the implicitlyPreferredVersion with the explicit preferredVersion if and only if the explicit preferredVersion is entirely a subset of the implicitlyPreferredVersion. Essentially, this means that:

• implicitlyPreferredVersions are a no-op to repo packages, but continue to impact external packages
• Explicit preferredVersions now impact the repo, regardless of the state of ensureConsistentVersions
• The explicit version takes the priority in cases where there is both an explicit and an implicit version, which feels correct
• We are performing the same expected logic on repo dependencies as we do on transitive dependencies

One side-effect of this is that ensureConsistentVersions basically becomes a linting tool to enforce that the specified version of a package in "package.json" is the version you get on install. One big note about this is that it means that during rush publish, the version of the dependencies in the packaged/published tarball will be updated to the version in the common-versions.json file if the version applies. This would be a bit opaque... but could also be an intended concequence of the changes, depending on how you look at it... I think I'm okay with this, but @octogonz should confirm that this is okay.

[octogonz]

@D4N14L a question we considered in Rush Hour: Should an implicitly preferred version be used to /narrow/ a preferred version? Or should an explicit preferred version always /replace/ it?

[D4N14L]

@octogonz I think it should always replace it, as long as it is entirely a subset of the implicit version. This is the logic we follow in the pnpmfile shim, and this would make it consistent. If we want to limit it using the implicit version if it's a partial subset, that could be a future feature, but the base feature should align with what we already do IMO