Fixed an issue with parsing --json output when publishing (#676)

* Fixed an issue with parsing `--json` output when publishing

* Be more defensive about the `getLastJsonObjectFromString` return value

.changeset/chilly-hounds-break.md

@@ -0,0 +1,5 @@
+---
+"@changesets/cli": patch
+---
+
+Improved compatibility with npm 7+ since they've started to print errors to the `stderr` (where previously they were printed to `stdout`) when using `npm publish --json`.

.changeset/chilly-hounds-return.md

@@ -0,0 +1,5 @@
+---
+"@changesets/cli": patch
+---
+
+Fixed an internal issue that prevented `npm publish --json`'s output to be handled properly. This makes sure that unrelated JSONs printed by lifecycle scripts don't interfere with our logic.

packages/cli/src/commands/publish/npm-utils.ts

@@ -9,6 +9,7 @@ import semver from "semver";
 import { askQuestion } from "../../utils/cli-utilities";
 import isCI from "../../utils/isCI";
 import { TwoFactorState } from "../../utils/types";
+import { getLastJsonObjectFromString } from "../../utils/getLastJsonObjectFromString";
 
 const npmRequestLimit = pLimit(40);
 const npmPublishLimit = pLimit(10);
@@ -166,40 +167,47 @@ async function internalPublish(
   const envOverride = {
     npm_config_registry: getCorrectRegistry()
   };
-  let { stdout } = await spawn(
+  let { code, stdout, stderr } = await spawn(
     publishTool.name,
     ["publish", opts.cwd, "--json", ...publishFlags],
     {
       env: Object.assign({}, process.env, envOverride)
     }
   );
-  // New error handling. NPM's --json option is included alongside the `prepublish and
-  // `postpublish` contents in terminal. We want to handle this as best we can but it has
-  // some struggles
-  // Note that both pre and post publish hooks are printed before the json out, so this works.
-  let json = jsonParse(stdout.toString().replace(/[^{]*/, ""));
-
-  if (json.error) {
-    // The first case is no 2fa provided, the second is when the 2fa is wrong (timeout or wrong words)
-    if (
-      (json.error.code === "EOTP" ||
-        (json.error.code === "E401" &&
-          json.error.detail.includes("--otp=<code>"))) &&
-      !isCI
-    ) {
-      if (twoFactorState.token !== null) {
-        // the current otp code must be invalid since it errored
-        twoFactorState.token = null;
+  if (code !== 0) {
+    // NPM's --json output is included alongside the `prepublish` and `postpublish` output in terminal
+    // We want to handle this as best we can but it has some struggles:
+    // - output of those lifecycle scripts can contain JSON
+    // - npm7 has switched to printing `--json` errors to stderr (https://github.com/npm/cli/commit/1dbf0f9bb26ba70f4c6d0a807701d7652c31d7d4)
+    // Note that the `--json` output is always printed at the end so this should work
+    let json =
+      getLastJsonObjectFromString(stderr.toString()) ||
+      getLastJsonObjectFromString(stdout.toString());
+
+    if (json?.error) {
+      // The first case is no 2fa provided, the second is when the 2fa is wrong (timeout or wrong words)
+      if (
+        (json.error.code === "EOTP" ||
+          (json.error.code === "E401" &&
+            json.error.detail.includes("--otp=<code>"))) &&
+        !isCI
+      ) {
+        if (twoFactorState.token !== null) {
+          // the current otp code must be invalid since it errored
+          twoFactorState.token = null;
+        }
+        // just in case this isn't already true
+        twoFactorState.isRequired = Promise.resolve(true);
+        return internalPublish(pkgName, opts, twoFactorState);
       }
-      // just in case this isn't already true
-      twoFactorState.isRequired = Promise.resolve(true);
-      return internalPublish(pkgName, opts, twoFactorState);
+      error(
+        `an error occurred while publishing ${pkgName}: ${json.error.code}`,
+        json.error.summary,
+        json.error.detail ? "\n" + json.error.detail : ""
+      );
     }
-    error(
-      `an error occurred while publishing ${pkgName}: ${json.error.code}`,
-      json.error.summary,
-      json.error.detail ? "\n" + json.error.detail : ""
-    );
+
+    error(stderr);
     return { published: false };
   }
   return { published: true };

packages/cli/src/utils/getLastJsonObjectFromString.test.ts

@@ -0,0 +1,70 @@
+import { getLastJsonObjectFromString } from "./getLastJsonObjectFromString";
+
+describe("getLastJsonObjectFromString", () => {
+  it("should handle stringified object", () => {
+    expect(
+      getLastJsonObjectFromString(JSON.stringify({ test: "foo" }))
+    ).toEqual({ test: "foo" });
+  });
+
+  it("should handle stringified deep object", () => {
+    expect(
+      getLastJsonObjectFromString(
+        JSON.stringify({
+          test: "foo",
+          bar: { baz: { qwe: "rty" }, arr: [1, 2, 3, 4] }
+        })
+      )
+    ).toEqual({
+      test: "foo",
+      bar: { baz: { qwe: "rty" }, arr: [1, 2, 3, 4] }
+    });
+  });
+
+  it("should handle leading whitespace", () => {
+    expect(
+      getLastJsonObjectFromString(
+        `   \n\n  ${JSON.stringify({ test: "foo", baz: { qwe: "rty" } })}`
+      )
+    ).toEqual({ test: "foo", baz: { qwe: "rty" } });
+  });
+
+  it("should handle trailing whitespace", () => {
+    expect(
+      getLastJsonObjectFromString(
+        `${JSON.stringify({ test: "foo", baz: { qwe: "rty" } })}   \n\n  `
+      )
+    ).toEqual({ test: "foo", baz: { qwe: "rty" } });
+  });
+
+  it("should handle trailing text", () => {
+    expect(
+      getLastJsonObjectFromString(
+        `${JSON.stringify({ test: "foo", baz: { qwe: "rty" } })}   \n\n  test`
+      )
+    ).toEqual({ test: "foo", baz: { qwe: "rty" } });
+  });
+
+  it("should handle string with multiple objects", () => {
+    expect(
+      getLastJsonObjectFromString(
+        `${JSON.stringify({
+          test: "foo",
+          baz: { qwe: "rty" }
+        })}   \n\n  ${JSON.stringify({ much: "awesome" })}`
+      )
+    ).toEqual({ much: "awesome" });
+  });
+
+  it("should return `null` for an empty string", () => {
+    expect(getLastJsonObjectFromString("")).toEqual(null);
+  });
+
+  it("should return `null` for a string with a broken object", () => {
+    expect(getLastJsonObjectFromString(`{"bar:"`)).toEqual(null);
+  });
+
+  it("should return `null` for a string without an object", () => {
+    expect(getLastJsonObjectFromString(`qwerty`)).toEqual(null);
+  });
+});

packages/cli/src/utils/getLastJsonObjectFromString.ts

@@ -0,0 +1,15 @@
+export const getLastJsonObjectFromString = (str: string) => {
+  str = str.replace(/[^}]*$/, "");
+
+  while (str) {
+    str = str.replace(/[^{]*/, "");
+
+    try {
+      return JSON.parse(str);
+    } catch (err) {
+      // move past the potentially leading `{` so the regexp in the loop can try to match for the next `{`
+      str = str.slice(1);
+    }
+  }
+  return null;
+};