Update dependency correctly when it's declared multiple times with di…

…fferent ranges (#412)

* update a dependency correctly when it's declared multiple times with differen range

* Create nice-eels-study.md

* fix lint issue

* fix test

* Update package.json

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* Update packages/cli/src/commands/version/version.test.ts

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* update comment

* Update .changeset/nice-eels-study.md

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

* address comments

* fix lint

* update test description

* update test to use more realistic scenario

* tweak test title

* tweak changeset content

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

.changeset/nice-eels-study.md

@@ -0,0 +1,6 @@
+---
+"@changesets/assemble-release-plan": patch
+"@changesets/cli": patch
+---
+
+Fixed an issue with the same package specified as a different dependency type with different range types not being updated correctly for all of them.

__fixtures__/simple-project-same-dep-diff-range/.changeset/README.md

@@ -0,0 +1,3 @@
+# We just want a file in here so git collects it
+
+For this we have deliberately not included a config file, as we want to test the defaults

__fixtures__/simple-project-same-dep-diff-range/.changeset/config.json

@@ -0,0 +1 @@
+{}

__fixtures__/simple-project-same-dep-diff-range/package.json

@@ -0,0 +1,11 @@
+{
+  "private": true,
+  "name": "simple-project",
+  "description": "three projects, each depending on one other",
+  "version": "1.0.0",
+  "bolt": {
+    "workspaces": [
+      "packages/*"
+    ]
+  }
+}

__fixtures__/simple-project-same-dep-diff-range/packages/pkg-a/package.json

@@ -0,0 +1,10 @@
+{
+  "name": "pkg-a",
+  "version": "1.0.0",
+  "peerDependencies": {
+    "pkg-b": "^1.0.0"
+  },
+  "devDependencies": {
+    "pkg-b": "1.0.0"
+  }
+}

__fixtures__/simple-project-same-dep-diff-range/packages/pkg-b/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "pkg-b",
+  "version": "1.0.0"
+}

packages/apply-release-plan/src/utils.ts

@@ -47,6 +47,5 @@ export function shouldUpdateDependencyBasedOnConfig(
   if (depType === "peerDependencies") {
     shouldUpdate = !onlyUpdatePeerDependentsWhenOutOfRange;
   }
-
   return shouldUpdate;
 }

packages/assemble-release-plan/src/determine-dependents.ts

@@ -59,48 +59,59 @@ export default function getDependents({
 
         const dependentPackage = packagesByName.get(dependent);
         if (!dependentPackage) throw new Error("Dependency map is incorrect");
-        const { depTypes, versionRange } = getDependencyVersionRange(
-          dependentPackage.packageJson,
-          nextRelease.name
-        );
 
         // If the dependent is an ignored package, we want to bump its dependencies without a release, so setting type to "none"
         if (ignoredPackages.includes(dependent)) {
           type = "none";
-        }
-        // we check if it is a peerDependency because if it is, our dependent bump type needs to be major.
-        else if (
-          depTypes.includes("peerDependencies") &&
-          nextRelease.type !== "patch" &&
-          (!onlyUpdatePeerDependentsWhenOutOfRange ||
-            !semver.satisfies(
-              incrementVersion(nextRelease, preInfo),
-              versionRange
-            )) &&
-          (!releases.has(dependent) ||
-            (releases.has(dependent) &&
-              releases.get(dependent)!.type !== "major"))
-        ) {
-          type = "major";
         } else {
-          if (
-            // TODO validate this - I don't think it's right anymore
-            (!releases.has(dependent) ||
-              releases.get(dependent)!.type === "none") &&
-            !semver.satisfies(
-              incrementVersion(nextRelease, preInfo),
-              versionRange
-            )
-          ) {
+          const dependencyVersionRanges = getDependencyVersionRanges(
+            dependentPackage.packageJson,
+            nextRelease.name
+          );
+
+          for (const { depType, versionRange } of dependencyVersionRanges) {
             if (
-              depTypes.includes("dependencies") ||
-              depTypes.includes("optionalDependencies") ||
-              depTypes.includes("peerDependencies")
+              shouldBumpMajor({
+                dependent,
+                depType,
+                versionRange,
+                releases,
+                nextRelease,
+                preInfo,
+                onlyUpdatePeerDependentsWhenOutOfRange
+              })
             ) {
-              type = "patch";
+              type = "major";
             } else {
-              // We don't need a version bump if the package is only in the devDependencies of the dependent package
-              type = "none";
+              if (
+                // TODO validate this - I don't think it's right anymore
+                (!releases.has(dependent) ||
+                  releases.get(dependent)!.type === "none") &&
+                !semver.satisfies(
+                  incrementVersion(nextRelease, preInfo),
+                  versionRange
+                )
+              ) {
+                switch (depType) {
+                  case "dependencies":
+                  case "optionalDependencies":
+                  case "peerDependencies":
+                    if (type !== "major" && type !== "minor") {
+                      type = "patch";
+                    }
+                    break;
+                  case "devDependencies": {
+                    // We don't need a version bump if the package is only in the devDependencies of the dependent package
+                    if (
+                      type !== "major" &&
+                      type !== "minor" &&
+                      type !== "patch"
+                    ) {
+                      type = "none";
+                    }
+                  }
+                }
+              }
             }
           }
         }
@@ -145,35 +156,70 @@ export default function getDependents({
 }
 
 /*
-  Returns an object in the shape { depTypes: [], versionRange: '' } with a list of different depTypes
-  matched ('dependencies', 'peerDependencies', etc) and the versionRange itself ('^1.0.0')
+  Returns an array of objects in the shape { depType: DependencyType, versionRange: string }
+  The array can contain more than one elements in case a dependency appears in multiple
+  dependency lists. For example, a package that is both a peerDepenency and a devDependency.
 */
-
-function getDependencyVersionRange(
+function getDependencyVersionRanges(
   dependentPkgJSON: PackageJSON,
   dependencyName: string
-) {
+): {
+  depType: DependencyType;
+  versionRange: string;
+}[] {
   const DEPENDENCY_TYPES = [
     "dependencies",
     "devDependencies",
     "peerDependencies",
     "optionalDependencies"
   ] as const;
-  const dependencyVersionRange: {
-    depTypes: DependencyType[];
+  const dependencyVersionRanges: {
+    depType: DependencyType;
     versionRange: string;
-  } = {
-    depTypes: [],
-    versionRange: ""
-  };
+  }[] = [];
   for (const type of DEPENDENCY_TYPES) {
     const deps = dependentPkgJSON[type];
     if (!deps) continue;
     if (deps[dependencyName]) {
-      dependencyVersionRange.depTypes.push(type);
-      // We'll just override this each time, *hypothetically* it *should* be the same...
-      dependencyVersionRange.versionRange = deps[dependencyName];
+      dependencyVersionRanges.push({
+        depType: type,
+        versionRange: deps[dependencyName]
+      });
     }
   }
-  return dependencyVersionRange;
+  return dependencyVersionRanges;
+}
+
+function shouldBumpMajor({
+  dependent,
+  depType,
+  versionRange,
+  releases,
+  nextRelease,
+  preInfo,
+  onlyUpdatePeerDependentsWhenOutOfRange
+}: {
+  dependent: string;
+  depType: DependencyType;
+  versionRange: string;
+  releases: Map<string, InternalRelease>;
+  nextRelease: InternalRelease;
+  preInfo: PreInfo | undefined;
+  onlyUpdatePeerDependentsWhenOutOfRange: boolean;
+}) {
+  // we check if it is a peerDependency because if it is, our dependent bump type might need to be major.
+  return (
+    depType === "peerDependencies" &&
+    nextRelease.type !== "patch" &&
+    // 1. If onlyUpdatePeerDependentsWhenOutOfRange set to true, bump major if the version is leaving the range.
+    // 2. If onlyUpdatePeerDependentsWhenOutOfRange set to false, bump major regardless whether or not the version is leaving the range.
+    (!onlyUpdatePeerDependentsWhenOutOfRange ||
+      !semver.satisfies(
+        incrementVersion(nextRelease, preInfo),
+        versionRange
+      )) &&
+    // bump major only if the dependent doesn't already has a major release.
+    (!releases.has(dependent) ||
+      (releases.has(dependent) && releases.get(dependent)!.type !== "major"))
+  );
 }

packages/cli/src/commands/version/version.test.ts

@@ -356,6 +356,44 @@ describe("running version in a simple project with workspace range", () => {
   });
 });
 
+describe("same package in different dependency types", () => {
+  it("should update different range types correctly", async () => {
+    let cwd = f.copy("simple-project-same-dep-diff-range");
+    await writeChangeset(
+      {
+        releases: [
+          {
+            name: "pkg-b",
+            type: "patch"
+          }
+        ],
+        summary: "a very useful summary for the first change"
+      },
+      cwd
+    );
+
+    await version(cwd, defaultOptions, modifiedDefaultConfig);
+
+    let packages = (await getPackages(cwd))!;
+    expect(packages.packages.map(x => x.packageJson)).toEqual([
+      {
+        devDependencies: {
+          "pkg-b": "1.0.1"
+        },
+        peerDependencies: {
+          "pkg-b": "^1.0.1"
+        },
+        name: "pkg-a",
+        version: "1.0.0"
+      },
+      {
+        name: "pkg-b",
+        version: "1.0.1"
+      }
+    ]);
+  });
+});
+
 describe("snapshot release", () => {
   it("should update the packge to unique version no matter the kind of version bump it is", async () => {
     let cwd = f.copy("simple-project");