Fixed an issue with incorrect dependent updates on none changesets wi…

…th `updateInternalDependents: "always"` (changesets#949)

* Add tests to showcase buggy behaviour

Test that when `updateInternalDependents:'always'` is set then
transitive dependencies of devDependencies should not be part of the
release plan.

* Fixed the issue

* Tweak test title

* Check first if the `nextRelease` is `none`

* Remove the redundant indentation

* Tweak `launch.json`

* Add changeset

* Add test case and juggle some tests into their appropriate blocks

* Add a simpler test case too

Co-authored-by: Mateusz Burzyński <[email protected]>

.changeset/silent-ants-smell.md

@@ -0,0 +1,17 @@
+---
+"@changesets/assemble-release-plan": patch
+"@changesets/cli": patch
+---
+
+author: @Andarist
+author: @BPScott
+
+Fixed the issue that caused transitive dependents of dev dependents to be bumped when a package got bumped and when using `___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH.updateInternalDependents: "always"`. To illustrate this with an example:
+
+```
+pkg-a - version: 1.0.0
+pkg-b - devDependencies['pkg-a']: 1.0.0
+pkg-c - dependencies['pkg-b']: 1.0.0
+```
+
+With a changeset for `pkg-a` the `pkg-c` could have been sometimes incorrectly released.

.vscode/launch.json

@@ -1,30 +1,12 @@
 {
   "version": "0.2.0",
   "configurations": [
-    {
-      "type": "node",
-      "request": "launch",
-      "name": "Jest All",
-      "program": "${workspaceFolder}/node_modules/.bin/jest",
-      "args": ["--runInBand"],
-      "console": "integratedTerminal",
-      "internalConsoleOptions": "neverOpen",
-      "disableOptimisticBPs": true,
-      "windows": {
-        "program": "${workspaceFolder}/node_modules/jest/bin/jest"
-      }
-    },
     {
       "type": "node",
       "request": "launch",
       "name": "Jest Current File",
       "program": "${workspaceFolder}/node_modules/.bin/jest",
-      "args": [
-        "${fileBasenameNoExtension}",
-        "--config",
-        "jest.config.js",
-        "--no-cache"
-      ],
+      "args": ["${relativeFile}", "--config", "jest.config.js", "--no-cache"],
       "console": "integratedTerminal",
       "internalConsoleOptions": "neverOpen",
       "disableOptimisticBPs": true,

__fixtures__/simplest-transitive-devdependent/.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__/simplest-transitive-devdependent/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__/simplest-transitive-devdependent/packages/pkg-a/package.json

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

__fixtures__/simplest-transitive-devdependent/packages/pkg-b/package.json

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

__fixtures__/simplest-transitive-devdependent/packages/pkg-c/package.json

@@ -0,0 +1,5 @@
+{
+  "name": "pkg-c",
+  "version": "1.0.0",
+  "dependencies": { "pkg-b": "1.0.0" }
+}

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

@@ -66,7 +66,9 @@ export default function determineDependents({
           );
 
           for (const { depType, versionRange } of dependencyVersionRanges) {
-            if (
+            if (nextRelease.type === "none") {
+              continue;
+            } else if (
               shouldBumpMajor({
                 dependent,
                 depType,
@@ -80,35 +82,32 @@ export default function determineDependents({
               })
             ) {
               type = "major";
-            } else {
-              if (
-                // TODO validate this - I don't think it's right anymore
-                (!releases.has(dependent) ||
-                  releases.get(dependent)!.type === "none") &&
-                (config.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH
-                  .updateInternalDependents === "always" ||
-                  !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";
-                    }
+            } else if (
+              (!releases.has(dependent) ||
+                releases.get(dependent)!.type === "none") &&
+              (config.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH
+                .updateInternalDependents === "always" ||
+                !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";
                   }
                 }
               }

packages/assemble-release-plan/src/index.test.ts

@@ -542,6 +542,24 @@ Mixed changesets that contain both ignored and not ignored packages are not allo
 `);
   });
 
+  it("should not bump a dev dependent nor its dependent when a package gets bumped", () => {
+    setup.updateDevDependency("pkg-b", "pkg-a", "1.0.0");
+    setup.updateDependency("pkg-c", "pkg-b", "1.0.0");
+
+    let { releases } = assembleReleasePlan(
+      setup.changesets,
+      setup.packages,
+      defaultConfig,
+      undefined
+    );
+
+    expect(releases.length).toBe(2);
+    expect(releases[0].name).toEqual("pkg-a");
+    expect(releases[0].newVersion).toEqual("1.0.1");
+    expect(releases[1].name).toEqual("pkg-b");
+    expect(releases[1].newVersion).toEqual("1.0.0");
+  });
+
   describe("fixed packages", () => {
     it("should assemble release plan for fixed packages", () => {
       setup.addChangeset({
@@ -1038,6 +1056,108 @@ Mixed changesets that contain both ignored and not ignored packages are not allo
       expect(releases[1].newVersion).toEqual("1.0.1");
     });
   });
+
+  describe("updateInternalDependents: always", () => {
+    it("should bump a direct dependent when a dependency package gets bumped", () => {
+      setup.updateDependency("pkg-b", "pkg-a", "^1.0.0");
+
+      let { releases } = assembleReleasePlan(
+        setup.changesets,
+        setup.packages,
+        {
+          ...defaultConfig,
+          ___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH: {
+            ...defaultConfig.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH,
+            updateInternalDependents: "always",
+          },
+        },
+        undefined
+      );
+
+      expect(releases.length).toBe(2);
+      expect(releases[0].name).toEqual("pkg-a");
+      expect(releases[0].newVersion).toEqual("1.0.1");
+      expect(releases[1].name).toEqual("pkg-b");
+      expect(releases[1].newVersion).toEqual("1.0.1");
+    });
+
+    it("should bump a transitive dependent when a dependency package gets bumped", () => {
+      setup.updateDependency("pkg-b", "pkg-a", "^1.0.0");
+      setup.updateDependency("pkg-c", "pkg-b", "^1.0.0");
+
+      let { releases } = assembleReleasePlan(
+        setup.changesets,
+        setup.packages,
+        {
+          ...defaultConfig,
+          ___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH: {
+            ...defaultConfig.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH,
+            updateInternalDependents: "always",
+          },
+        },
+        undefined
+      );
+
+      expect(releases.length).toBe(3);
+      expect(releases[0].name).toEqual("pkg-a");
+      expect(releases[0].newVersion).toEqual("1.0.1");
+      expect(releases[1].name).toEqual("pkg-b");
+      expect(releases[1].newVersion).toEqual("1.0.1");
+      expect(releases[2].name).toEqual("pkg-c");
+      expect(releases[2].newVersion).toEqual("1.0.1");
+    });
+
+    it("not bump a dependent package when a dependency has `none` changeset", () => {
+      setup.updateDependency("pkg-b", "pkg-c", "^1.0.0");
+      setup.addChangeset({
+        id: "stuff-and-nonsense",
+        releases: [{ name: "pkg-c", type: "none" }],
+      });
+
+      let { releases } = assembleReleasePlan(
+        setup.changesets,
+        setup.packages,
+        {
+          ...defaultConfig,
+          ___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH: {
+            ...defaultConfig.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH,
+            updateInternalDependents: "always",
+          },
+        },
+        undefined
+      );
+
+      expect(releases.length).toBe(2);
+      expect(releases[0].name).toEqual("pkg-a");
+      expect(releases[0].newVersion).toEqual("1.0.1");
+      expect(releases[1].name).toEqual("pkg-c");
+      expect(releases[1].newVersion).toEqual("1.0.0");
+    });
+
+    it("should not bump a dev dependent nor its dependent when a package gets bumped", () => {
+      setup.updateDevDependency("pkg-b", "pkg-a", "^1.0.0");
+      setup.updateDependency("pkg-c", "pkg-b", "^1.0.0");
+
+      let { releases } = assembleReleasePlan(
+        setup.changesets,
+        setup.packages,
+        {
+          ...defaultConfig,
+          ___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH: {
+            ...defaultConfig.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH,
+            updateInternalDependents: "always",
+          },
+        },
+        undefined
+      );
+
+      expect(releases.length).toBe(2);
+      expect(releases[0].name).toEqual("pkg-a");
+      expect(releases[0].newVersion).toEqual("1.0.1");
+      expect(releases[1].name).toEqual("pkg-b");
+      expect(releases[1].newVersion).toEqual("1.0.0");
+    });
+  });
 });
 
 describe("version update thoroughness", () => {
@@ -1335,11 +1455,13 @@ describe("bumping peerDeps", () => {
       expect(releases[0].name).toEqual("pkg-a");
       expect(releases[0].newVersion).toEqual("1.1.0");
     });
-  });
 
-  describe("updateInternalDependents: always", () => {
-    it("should bump a direct dependent when a dependency package gets bumped", () => {
-      setup.updateDependency("pkg-b", "pkg-a", "^1.0.0");
+    it("should major bump dependent when leaving range", () => {
+      setup.updatePeerDependency("pkg-b", "pkg-a", "~1.0.0");
+      setup.addChangeset({
+        id: "anyway-the-windblows",
+        releases: [{ name: "pkg-a", type: "minor" }],
+      });
 
       let { releases } = assembleReleasePlan(
         setup.changesets,
@@ -1348,72 +1470,18 @@ describe("bumping peerDeps", () => {
           ...defaultConfig,
           ___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH: {
             ...defaultConfig.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH,
-            updateInternalDependents: "always",
+            onlyUpdatePeerDependentsWhenOutOfRange: true,
           },
         },
         undefined
       );
 
       expect(releases.length).toBe(2);
       expect(releases[0].name).toEqual("pkg-a");
-      expect(releases[0].newVersion).toEqual("1.0.1");
-      expect(releases[1].name).toEqual("pkg-b");
-      expect(releases[1].newVersion).toEqual("1.0.1");
-    });
-
-    it("should bump a transitive dependent when a dependency package gets bumped", () => {
-      setup.addPackage("pkg-c", "1.0.0");
-      setup.updateDependency("pkg-b", "pkg-a", "^1.0.0");
-      setup.updateDependency("pkg-c", "pkg-b", "^1.0.0");
-
-      let { releases } = assembleReleasePlan(
-        setup.changesets,
-        setup.packages,
-        {
-          ...defaultConfig,
-          ___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH: {
-            ...defaultConfig.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH,
-            updateInternalDependents: "always",
-          },
-        },
-        undefined
-      );
-
-      expect(releases.length).toBe(3);
-      expect(releases[0].name).toEqual("pkg-a");
-      expect(releases[0].newVersion).toEqual("1.0.1");
+      expect(releases[0].newVersion).toEqual("1.1.0");
       expect(releases[1].name).toEqual("pkg-b");
-      expect(releases[1].newVersion).toEqual("1.0.1");
-      expect(releases[2].name).toEqual("pkg-c");
-      expect(releases[2].newVersion).toEqual("1.0.1");
-    });
-  });
-
-  it("should major bump dependent when leaving range", () => {
-    setup.updatePeerDependency("pkg-b", "pkg-a", "~1.0.0");
-    setup.addChangeset({
-      id: "anyway-the-windblows",
-      releases: [{ name: "pkg-a", type: "minor" }],
+      expect(releases[1].newVersion).toEqual("2.0.0");
     });
-
-    let { releases } = assembleReleasePlan(
-      setup.changesets,
-      setup.packages,
-      {
-        ...defaultConfig,
-        ___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH: {
-          ...defaultConfig.___experimentalUnsafeOptions_WILL_CHANGE_IN_PATCH,
-          onlyUpdatePeerDependentsWhenOutOfRange: true,
-        },
-      },
-      undefined
-    );
-
-    expect(releases.length).toBe(2);
-    expect(releases[0].name).toEqual("pkg-a");
-    expect(releases[0].newVersion).toEqual("1.1.0");
-    expect(releases[1].name).toEqual("pkg-b");
-    expect(releases[1].newVersion).toEqual("2.0.0");
   });
 });