`bun install` correctly join dependency URLs (#4421)

* use WTF to join registry strings

* show dependency error messages, better join error

We actually report errors when enqueuing dependencies now. I also made
the join URLs error message read better. It'd be cleaner to handle it
all in one place, but there's currently no way to propagate the data up.

* starting on registry URL tests

* added more registry URL tests

* [install] prevent optional/peer deps from failing builds

Couldn't get the peer dependency test to work, but the code is there.

* ran prettier

* changed error note to use realname, updated tests

* ran prettier again...

2 files changed, 156 insertions, 11 deletions

diff --git a/test/cli/install/bun-add.test.ts b/test/cli/install/bun-add.test.ts
index 589b7a87e..4461584e4 100644
--- a/test/cli/install/bun-add.test.ts
+++ b/test/cli/install/bun-add.test.ts
@@ -102,11 +102,10 @@ it("should reject missing package", async () => {
   });
   expect(stderr).toBeDefined();
   const err = await new Response(stderr).text();
-  expect(err.replace(/^(.*?) v[^\n]+/, "$1").split(/\r?\n/)).toEqual([
-    "bun add",
-    `error: file:${add_path} failed to resolve`,
-    "",
-  ]);
+  expect(err.includes("bun add")).toBeTrue();
+  expect(err.includes("error: MissingPackageJSON")).toBeTrue();
+  expect(err.includes(`note: error occured while resolving file:${add_path}`)).toBeTrue();
+
   expect(stdout).toBeDefined();
   const out = await new Response(stdout).text();
   expect(out).toBe("");
@@ -145,11 +144,10 @@ it("should reject invalid path without segfault", async () => {
   });
   expect(stderr).toBeDefined();
   const err = await new Response(stderr).text();
-  expect(err.replace(/^(.*?) v[^\n]+/, "$1").split(/\r?\n/)).toEqual([
-    "bun add",
-    `error: file://${add_path} failed to resolve`,
-    "",
-  ]);
+  expect(err.includes("bun add")).toBeTrue();
+  expect(err.includes("error: MissingPackageJSON")).toBeTrue();
+  expect(err.includes(`note: error occured while resolving file://${add_path}`)).toBeTrue();
+
   expect(stdout).toBeDefined();
   const out = await new Response(stdout).text();
   expect(out).toBe("");
diff --git a/test/cli/install/bun-install.test.ts b/test/cli/install/bun-install.test.ts
index 28edac092..09da0bbd4 100644
--- a/test/cli/install/bun-install.test.ts
+++ b/test/cli/install/bun-install.test.ts
@@ -1,5 +1,5 @@
 import { file, listen, Socket, spawn } from "bun";
-import { afterAll, afterEach, beforeAll, beforeEach, expect, it } from "bun:test";
+import { afterAll, afterEach, beforeAll, beforeEach, expect, it, describe, test } from "bun:test";
 import { bunExe, bunEnv as env } from "harness";
 import { access, mkdir, readlink, realpath, rm, writeFile } from "fs/promises";
 import { join } from "path";
@@ -6006,3 +6006,150 @@ it("should handle `workspace:*` on both root & child", async () => {
   expect(await file(join(package_dir, "node_modules", "baz", "package.json")).text()).toEqual(baz_package);
   await access(join(package_dir, "bun.lockb"));
 });
+
+describe("Registry URLs", () => {
+  // Some of the non failing URLs are invalid, but bun's URL parser ignores
+  // the validation error and returns a valid serialized URL anyway.
+  const registryURLs: [url: string, fails: boolean][] = [
+    ["asdfghjklqwertyuiop", true],
+    ["                ", true],
+    ["::::::::::::::::", true],
+    ["https://ex ample.org/", true],
+    ["example", true],
+    ["https://example.com:demo", true],
+    ["http://[www.example.com]/", true],
+    ["c:", true],
+    ["c:a", true],
+    ["https://registry.npmjs.org/", false],
+    ["https://artifactory.xxx.yyy/artifactory/api/npm/my-npm/", false], // https://github.com/oven-sh/bun/issues/3899
+    ["", false],
+    ["https:example.org", false],
+    ["https://////example.com///", false],
+    ["https://example.com/https:example.org", false],
+    ["https://example.com/[]?[]#[]", false],
+    ["https://example/%?%#%", false],
+    ["c:/", false],
+    ["https://點看", false], // gets converted to punycode
+    ["https://xn--c1yn36f/", false],
+  ];
+
+  for (const entry of registryURLs) {
+    const regURL = entry[0];
+    const fails = entry[1];
+
+    it(`should ${fails ? "fail" : "handle"} joining registry and package URLs (${regURL})`, async () => {
+      await writeFile(join(package_dir, "bunfig.toml"), `[install]\ncache = false\nregistry = "${regURL}"`);
+
+      await writeFile(
+        join(package_dir, "package.json"),
+        JSON.stringify({
+          name: "foo",
+          version: "0.0.1",
+          dependencies: {
+            notapackage: "0.0.2",
+          },
+        }),
+      );
+
+      const { stdout, stderr, exited } = spawn({
+        cmd: [bunExe(), "install"],
+        cwd: package_dir,
+        stdout: null,
+        stdin: "pipe",
+        stderr: "pipe",
+        env,
+      });
+      expect(stdout).toBeDefined();
+      expect(await new Response(stdout).text()).toBeEmpty();
+
+      expect(stderr).toBeDefined();
+      const err = await new Response(stderr).text();
+
+      if (fails) {
+        expect(err.includes(`Failed to join registry \"${regURL}\" and package \"notapackage\" URLs`)).toBeTrue();
+        expect(err.includes("error: InvalidURL")).toBeTrue();
+      } else {
+        expect(err.includes("error: notapackage@0.0.2 failed to resolve")).toBeTrue();
+      }
+      // fails either way, since notapackage is, well, not a real package.
+      expect(await exited).not.toBe(0);
+    });
+  }
+
+  it("shouldn't fail joining invalid registry and package URLs for optional dependencies", async () => {
+    const regURL = "asdfghjklqwertyuiop";
+
+    await writeFile(join(package_dir, "bunfig.toml"), `[install]\ncache = false\nregistry = "${regURL}"`);
+
+    await writeFile(
+      join(package_dir, "package.json"),
+      JSON.stringify({
+        name: "foo",
+        version: "0.0.1",
+        optionalDependencies: {
+          notapackage: "0.0.2",
+        },
+      }),
+    );
+
+    const { stdout, stderr, exited } = spawn({
+      cmd: [bunExe(), "install"],
+      cwd: package_dir,
+      stdout: null,
+      stdin: "pipe",
+      stderr: "pipe",
+      env,
+    });
+    expect(stdout).toBeDefined();
+    expect(await new Response(stdout).text()).not.toBeEmpty();
+
+    expect(stderr).toBeDefined();
+    const err = await new Response(stderr).text();
+
+    expect(err.includes(`Failed to join registry \"${regURL}\" and package \"notapackage\" URLs`)).toBeTrue();
+    expect(err.includes("warn: InvalidURL")).toBeTrue();
+
+    expect(await exited).toBe(0);
+  });
+
+  // TODO: This test should fail if the param `warn_on_error` is true in
+  // `(install.zig).NetworkTask.forManifest()`. Unfortunately, that
+  // code never gets run for peer dependencies unless you do some package
+  // manifest magic. I doubt it'd ever fail, but having a dedicated
+  // test would be nice.
+  test.todo("shouldn't fail joining invalid registry and package URLs for peer dependencies", async () => {
+    const regURL = "asdfghjklqwertyuiop";
+
+    await writeFile(join(package_dir, "bunfig.toml"), `[install]\ncache = false\nregistry = "${regURL}"`);
+
+    await writeFile(
+      join(package_dir, "package.json"),
+      JSON.stringify({
+        name: "foo",
+        version: "0.0.1",
+        peerDependencies: {
+          notapackage: "0.0.2",
+        },
+      }),
+    );
+
+    const { stdout, stderr, exited } = spawn({
+      cmd: [bunExe(), "install"],
+      cwd: package_dir,
+      stdout: null,
+      stdin: "pipe",
+      stderr: "pipe",
+      env,
+    });
+    expect(stdout).toBeDefined();
+    expect(await new Response(stdout).text()).not.toBeEmpty();
+
+    expect(stderr).toBeDefined();
+    const err = await new Response(stderr).text();
+
+    expect(err.includes(`Failed to join registry \"${regURL}\" and package \"notapackage\" URLs`)).toBeTrue();
+    expect(err.includes("warn: InvalidURL")).toBeTrue();
+
+    expect(await exited).toBe(0);
+  });
+});